Skip to content
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] Change session activation #1093

Merged
merged 9 commits into from
May 21, 2024
Merged

Conversation

StevenCellist
Copy link
Collaborator

Due to a slight bug introduced in one of my recent patches, I decided after conferring with @HeadBoffin to shift around some of the logic around session restoring / activation.

Restoring the Nonces and Session buffer now immediately restores all the contents of the buffer, instead of waiting for this to happen in begin(). Should make it easier to read and maintain. Now if begin() is called with the force argument set to false (default), it will check if a restored session is available for the current activitation information (keys, plan and the like) and return SESSION_RESTORED if this is the case - if there was none or not a valid session, it will immediately do a join and return (if succeeded) NEW_SESSION. Hence there is no need to set force to true anymore, simplifying logic and being more intuitive. It is still available for rejoining while a session is active, however.

For the examples in this repository there is not much difference (just begin()'s return code and dropping the argument to begin()), but the persistence examples should see nice improvements.

Also, a slight bug has been fixed regarding Nonces not increasing on a failed join (buffer checksum was not recalculated and could result in restoring rejection).

Pending confirmation by @HeadBoffin (already tested to run fine, but slight misuse-testing also welcome), and ready for review.

@StevenCellist StevenCellist changed the title [LoRaWAN] [LoRaWAN] Change session activation May 12, 2024
Copy link
Owner

@jgromes jgromes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again @StevenCellist - looks good from my end! Admittedly I can't really check the complete join/restore logic, but from the POV of code clarity, style, memory handling etc. all seems to be in order :)

@StevenCellist
Copy link
Collaborator Author

The latest commits split the begin() out into begin() and activate().
The reasoning is as follows: to restore the nonces and session, the code should already know what it should expect w.r.t. keys, mode, frequency plan and class. If you first restore and then call begin() with a different key, it must throw away whatever it restored and it is hard to make it clear from return codes that something like that happened.
It is much easier if we first call begin() so the code knows what to expect, then restore the buffers, and finally call activate() - this either finalizes the restored buffers, or does a fresh join.

  • begin() is now a void-function as it just takes the keys, generates their checksums and sets mode and class (latter always A)
  • setBufferNonces() returns ERR_NONE or NONCES_DISCARDED (or CHECKSUM_MISMATCH)
  • setBufferSession() returns ERR_NONE or SESSION_DISCARDED (or CHECKSUM_MISMATCH)
  • activate() returns SESSION_RESTORED or NEW_SESSION (or NO_DOWNLINK or any hardware-related error-code)

Additionally, an optional event is added to activateOTAA() that returns the DevNonce and JoinNonce.

Checked to run fine on deepsleeping ESP32s for both OTAA and ABP on EU868, but waiting to get verified by an independent tester (@HeadBoffin).

@StevenCellist
Copy link
Collaborator Author

Additionally, the user now has access to clearing a session, after which calling activate() results in starting a new session. Thus, the force argument in the activate() functions is now dropped to make the flow more explicit if one wishes to start a new session while one is active.
After calling clearSession(), the function isActivated() evaluates to false which is an easy indication to call activate().

@HeadBoffin
Copy link
Collaborator

Abbrviating th function nams and th rturn cods dosn't mak things straightforward. Fundamntally works but thr is somthing in th saving of th join on boot == 1 that causs a join on boot == 2. Thraftr it sms to procd normally.

@StevenCellist
Copy link
Collaborator Author

@HeadBoffin I think your 'e'-key needs replacement ;)

Comment on lines +567 to +569
*/
void beginOTAA(uint64_t joinEUI, uint64_t devEUI, uint8_t* nwkKey, uint8_t* appKey);

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since begin methods are now void, and new activate methods were added, we need to update the examples to reflect that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup.. Nick or I will follow up with that!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A product of the testing!

@StevenCellist
Copy link
Collaborator Author

Examples rework in progress, via separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants