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 and upgrade persistence handling #1017

Merged
merged 6 commits into from
Mar 18, 2024
Merged

[LoRaWAN] Change and upgrade persistence handling #1017

merged 6 commits into from
Mar 18, 2024

Conversation

StevenCellist
Copy link
Collaborator

Description following soon... yes I know about BuildOpt.h, will resolve.
Ready for review, tested EU868 with OTAA and ABP, tested US915 with OTAA.

@StevenCellist
Copy link
Collaborator Author

Due to the problem reported by @ropg (thank you!), extensive work has been put into remodelling the handling of session persistence. This PR aims to remove any and all support of platform-specific code for handling the required persistence. This will be handled in https://github.com/radiolib-org/radiolib-persistence.

The solution in this PR is as follows - the stack exposes two buffers, namely:

  1. a 'Nonces' buffer (size: 16 bytes) that includes the Nonces of the OTAA sessions, as well as integrity values that keep track of whether these Nonces match the current configuration of the device, and
  2. a 'Session' buffer (size: 396 bytes) that contains all necessary details of the currently active session, such as all counters and the complete MAC state.

Both buffers include checksums, and the session buffer includes the checksum of the Nonces buffer and requires that these two match each other. This makes sure that the buffers cannot easily be (accidentally) tempered with.

@HeadBoffin is working on examples that showcase how these buffers can be used for nicely working persistence of ESP32.

Converting this into some sort of persistence library will come at a later point in time in the other repository and is not the aim of this PR. It can be handled independently in the other mentioned repository.

A relevant changelog will follow later.

This PR also deletes all references to anything related to EEPROM throughout the source files.

@HeadBoffin
Copy link
Collaborator

Converting this into some sort of persistence library will come at a later point in time

If it transpires a persistence library is required - the ESP32 example we are using for testing uses native Arduino-ESP32 APIs with the aim that examples become available for AVR, SAMD, STM32 etc that uses the BSP APIs.

I'm not adverse to a library, but native APIs are looked after by a very large number of eyeballs - if we start creating additional libraries we end up with update work.

I'm aiming to put the ESP32 persistence example in to my WIP PR Sun 16th so we can join up everything.

@ropg
Copy link

ropg commented Mar 16, 2024

Great work! 👍

@StevenCellist
Copy link
Collaborator Author

  • [LoRaWAN] Change session persistence from integrated EEPROM to exposed buffers
  • [LoRaWAN] Make the return codes from beginOTAA() and beginABP() zero again (ERR_NONE)
  • [LoRaWAN] Swap the arguments to beginOTAA()
  • [LoRaWAN] Include frequency plan and device class into integrity checks
  • [LoRaWAN] Fix bugs in MAC state restoring
  • [LoRaWAN] Fix LoRaWAN cppcheck warnings / bugs #1018 incorrect loop
  • [LoRaWAN] Improve checksum calculation by working on pointers instead of creating new buffers
  • [LoRaWAN] Decrease debug output significantly (full restore + uplink now consumes about 100 lines on EU868/OTAA instead of closer to 200)
  • [BuildOpt] Remove EEPROM support code
  • [HAL] Remove EEPROM support code
  • [ArduinoHAL] Remove EEPROM support code
  • [TypeDef] Add checksum mismatch error

@StevenCellist StevenCellist linked an issue Mar 16, 2024 that may be closed by this pull request
@StevenCellist
Copy link
Collaborator Author

@jgromes not sure about your CI checks, but with the removal of EEPROM, it may be the case that you can add LoRaWAN back to some of the platforms for which it was disabled?

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.

@StevenCellist great work as always! I left some comments but none of them are major, more suggestions on improvements ;)

src/protocols/LoRaWAN/LoRaWAN.cpp Outdated Show resolved Hide resolved
src/protocols/LoRaWAN/LoRaWAN.cpp Outdated Show resolved Hide resolved
src/protocols/LoRaWAN/LoRaWAN.cpp Show resolved Hide resolved
src/protocols/LoRaWAN/LoRaWAN.cpp Show resolved Hide resolved
src/protocols/LoRaWAN/LoRaWAN.cpp Outdated Show resolved Hide resolved
src/protocols/LoRaWAN/LoRaWAN.cpp Outdated Show resolved Hide resolved
src/protocols/LoRaWAN/LoRaWAN.cpp Outdated Show resolved Hide resolved
src/protocols/LoRaWAN/LoRaWAN.cpp Show resolved Hide resolved
src/protocols/LoRaWAN/LoRaWAN.cpp Outdated Show resolved Hide resolved
@jgromes
Copy link
Owner

jgromes commented Mar 17, 2024

@StevenCellist

with the removal of EEPROM, it may be the case that you can add LoRaWAN back to some of the platforms for which it was disabled?

Definitely, some of them blocked only the persistent examples. I will try that manually after this is merged. The otehr ones are disabled due to program/data memory limitations (e.g. Arduino Uno), I don't think those will fit unfortuntely, even if we could have saved some memory with this PR.

@jgromes
Copy link
Owner

jgromes commented Mar 17, 2024

@HeadBoffin

native APIs are looked after by a very large number of eyeballs - if we start creating additional libraries we end up with update work.

I agree, perhaps we will end up with a repository of examples for different platforms (instead of a persistence library) - we'll see in due time. In my opinion that is OK as well, since the other option is having those examples in RadioLib, which is probably not the way this should be handled.

Warning: untested - am not at my desk
@StevenCellist
Copy link
Collaborator Author

Warning: untested yet - am not at my desk.

@StevenCellist
Copy link
Collaborator Author

Bugfixed and tested thanks to @HeadBoffin.

@jgromes jgromes merged commit ca2a307 into jgromes:master Mar 18, 2024
30 checks passed
@jgromes
Copy link
Owner

jgromes commented Mar 18, 2024

All looks good to me - good job, big task off the todo list @StevenCellist and @HeadBoffin!

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.

LoRaWAN cppcheck warnings / bugs
4 participants