-
Notifications
You must be signed in to change notification settings - Fork 413
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[LoRaWAN] Improve PHY behaviour, update beginABP, bugfixes #1080
Conversation
This PR has been tested on EU868/OTAA using the Reference and Persistent sketches. Both feedback and additional tests are welcome. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @StevenCellist for another great contribution! I left some comments, but I think most of it can be addressed quite quickly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Random request for a change to prevent a merge to allow someone to test this on US/AU
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Random approval because Steven said so
@jgromes thank you, LGTM. As per @HeadBoffin: tested on US915/OTAA as well. |
@StevenCellist I pushed the changes and also tested using an SX1272 to make sure the overrides are OK so this should be good to go. Thanks! |
Until TTS v3.30.0 (current version), the ABP keys are displayed incorrectly. With the upcoming TTS v3.30.1, the ABP keys are now displayed exactly as per spec. The
beginABP()
function from this PR onwards uses the exact same order of key arguments with exactly the same names. This also makes it non-obvious how to use LW v1.0.x session which are still not properly supported, as the F/SNwkSIntKeys are now required instead of NULL-default. (They can still be set to NULL.)If running LoRaWAN mixed with another protocol sharing the same radio, the physical layer was not reconfigured completely for each uplink. Also, as reminded in Discussions recently, there was a possibility to overrule the Tx power without the LW stack knowing about this. To prevent this, the LW stack now completely configures the PHY layer for each up/downlink.
To accomodate this change and prevent useless configuration of Tx power upon receiving ADR requests or user configuration, the checks for the module's Tx power range in the respective module drivers is separated from their
setOutputPower()
functions intocheckOutputPower()
.Moreover, the persistent buffers address calculations in for the Nonces & Session buffers was prone to user error - in fact, the session buffer even contained 48 useless bytes because I made an error! The improved implementation isn't particularly easier to read, but sure is safer. (And as a bonus, reduces from 396 bytes to 348.)
... and the other bugfixes / changes: