drivers/sx126x add optional CONFIG_SX126X_DEFAULT_SYNC_WORD#21711
drivers/sx126x add optional CONFIG_SX126X_DEFAULT_SYNC_WORD#21711crasbe merged 1 commit intoRIOT-OS:masterfrom
Conversation
github-actions
bot
added
Area: drivers
crasbe
added
Type: enhancement
|
Please add some documentation as to what |
fabian18
force-pushed
the
pr/sx126x_radio_hal_sync_word
branch
from
afe97d8 to
3aa6c33
Compare
fabian18
force-pushed
the
pr/sx126x_radio_hal_sync_word
branch
from
3aa6c33 to
450b653
Compare
crasbe
removed
the
Discussion: RFC
| #if defined(DOXYGEN) | ||
| /** | ||
| * @brief Configure the LoRa sync word | ||
| * | ||
| * The sync word for sx126x is 16 bits long. | ||
| * Private networks should use 0x1424. | ||
| * Public networks should use 0x3444. | ||
| * Using the driver API you only configure 2 nibbles Y and Z to form a sync word 0xY4Z4. | ||
| * The default chip value is 0x12 (0x1424). | ||
| * | ||
| * See https://blog.classycode.com/lora-sync-word-compatibility-between-sx127x-and-sx126x-460324d1787a | ||
| * for more information and a comparison with sx127x. | ||
| */ | ||
| # define CONFIG_SX126X_DEFAULT_SYNC_WORD 0x12 | ||
| #endif |
There was a problem hiding this comment.
Is this macro supposed to be an "internal only" macro or is it expected that a user can (and might want to) change it?
You have to navigate from the SX126x main page...
...to the internal page to find it.
Maybe the sx126x.h or sx126x_params.h would be a better home?
There was a problem hiding this comment.
I would say every CONFIG_ may be set by a user. There are a few CONFIG_ in some c files, which means they do not appear in the Doxygen doc. I moved it to internal because I am going to move all the other CONFIG_ there as well.
There are 2 ways that make sense to me. Either all CONFIG_ go to a public header where it is more likely to be seen by a user. Or some (if not all) CONFIG_ macros go to internal headers because setting them requires more experienced knowledge. There is no strict rule at the moment. The user is likely a (student) developer, so I would say the user is also able to look at internal doc.
There was a problem hiding this comment.
moved to public header
|
Also small nitpick: the headline and commit message are missing the |
|
So the commit message check lacks the check for the |
fabian18
force-pushed
the
pr/sx126x_radio_hal_sync_word
branch
from
450b653 to
7bd175e
Compare
This smells like a NETOPT. |
benpicco
left a comment
benpicco
left a comment
There was a problem hiding this comment.
Adding a NETOPT for the sync word would be a good follow-up, but setting the default value is independent of that.
5 participants
Contribution description
Configure a LoRa sync word if
CONFIG_SX126X_DEFAULT_SYNC_WORDis definedAlternatively, should it be in
params, in case 2 LoRa on a board should use different sync words?Testing procedure
Issues/PRs references
#21675