[LoRaWAN] Revamp internal processing, key checking, new MAC commands, implement DutyCycle & DwellTime

[StevenCellist]

Four days of intense work, but now in a MUCH better state. Most importantly, any internal processing of settings is now delegated to execMacCommand(), which should allow for much better maintainability, because the same setting is not altered at multiple different places. Instead, an internal MAC command is fired in that case and processed like 'normal' MAC commands, except there is no response to the LNS.

Changelog as far as I can remember:

• [LoRaWAN] Revamp internal processing of configuration to use unified MAC commands
• [LoRaWAN] Save MAC session to EEPROM as if they were MAC commands, and execute as MAC commands during restore
• [LoRaWAN] Added support for LinkCheckReq MAC command
• [LoRaWAN] Added support for DeviceTimeReq MAC command
• [LoRaWAN] Added key checking: if new keys are entered into the device, automatically wipe the existing session such that users do not require to do this, and it cannot activate 'false sessions' where new keys are entered but an old session is restored
• [LoRaWAN] Added mode checking: if a new mode is selected (OTAA or ABP), automatically wipe the existing session, ... idem
• [LoRaWAN] Expand the MAC queue to 9 commands, which is the maximum encountered during the first ABP uplink
• [LoRaWAN] Implemented Duty Cycle limits, enabled by default to maximum-per-law (disabled if no limit exists)
• [LoRaWAN] Added functions to set duty cycle limits, and request the time until next uplink is allowed under imposed duty cycle limits
• [LoRaWAN] Implemented Dwell Time limits, enabled by default to maximum-per-law (disabled if no limit exists)
• [LoRaWAN] Added functions to set dwell time limits, and request the maximum payload that is allowed under imposed dwell time limits
• [LoRaWAN] Allow resetting the downlink frame counter, necessary for Specification Verification protocol TS008 (soon™️ to be tested! 🎉 )
• [LoRaWAN] Make room for future class B and C support in the EEPROM table and some definitions.
• [LoRaWAN] Improve examples to include a simple SX126x pin order, include duty cycle limits, set ABP to LW v1.1 by default
• [HAL] Update EEPROM table for LoRaWAN
• [BuildOpt] Expand EEPROM table size
• [TypeDef] Change comment for LoRaWAN error
• Update keywords.txt

I probably have made some platform-dependent mistakes at some places..

Zzzzzzzz 🛌 💤

[StevenCellist]

BTW, these changes are tested on EU868 only (both OTAA and ABP) - there may be bugs for fixed bands (e.g. US). But hey, there's a Beta label so we'll have to wait for people to try it out to see how it fares.

[StevenCellist]

Fixes #913 and should also fix #917

[StevenCellist]

• [LoRaWAN] Convert setDatarate() and setTxPower() to internal MAC commands following unification
• [LoRaWAN] Improve ADR logic when no downlink was received

And I forgot one earlier:

• [LoRaWAN] Widen FCntUp space in EEPROM to ensure better wear-resistance

I think I now got all internal processing converted to MAC commands.. last commit on this PR unless stuff requested by others!

[jgromes]

Thank you for this quite extensive contribution! I reviewed it and left some comments, mostly regarding readability and code clarity.

One thing I would like to stress is please don't use one-line ifs without braces. I've seen those go wrong on multiple occasions. And even if they are accepted in a lot of places (Linux kernel comes to mind), I would stronlgy prefer to avoid them.

[StevenCellist]

Thanks for working your way completely through the not very small PR.. kudos to you!
My VS Code was really messing up tabs and spaces because I was for the first time actually keeping track of keywords.txt which requires true tabs. Which fixes about a third of the comments. And sure thing about the brackets on the other third of the comments. And I'm fine fixing the last third of the comments as well!

[StevenCellist]

!! Hold on, there's something in the new checkSum16() that crashes on run-time. Investigating and fixing...

[StevenCellist]

Found a small bug in fixed bands (while testing US915 thanks to my companion). You can keep the PR open so I can fix it somewhere later this week, although the bug is of very low impact because it is almost always fixed after one uplink.

15:48:02.736 > UL: 65 1 904.60 (4 - 4) | DL: 0 0  0.00 (0 - 0)
15:48:02.740 > UL: 9 1 904.10 (0 - 3) | DL: 0 0  0.00 (0 - 0)
15:48:02.744 > UL: 10 1 904.30 (0 - 3) | DL: 0 0  0.00 (0 - 0)
15:48:02.748 > UL: 11 1 904.50 (0 - 3) | DL: 0 0  0.00 (0 - 0)
15:48:02.753 > UL: 12 1 904.70 (0 - 3) | DL: 0 0  0.00 (0 - 0)
15:48:02.756 > UL: 13 1 904.90 (0 - 3) | DL: 0 0  0.00 (0 - 0)
15:48:02.760 > UL: 14 1 905.10 (0 - 3) | DL: 0 0  0.00 (0 - 0)
15:48:02.764 > UL: 15 1 905.30 (0 - 3) | DL: 0 0  0.00 (0 - 0)
15:48:02.769 > UL: 0 0  0.00 (0 - 0) | DL: 0 0  0.00 (0 - 0)
15:48:02.772 > UL: 0 0  0.00 (0 - 0) | DL: 0 0  0.00 (0 - 0)
15:48:02.776 > UL: 0 0  0.00 (0 - 0) | DL: 0 0  0.00 (0 - 0)
15:48:02.781 > UL: 0 0  0.00 (0 - 0) | DL: 0 0  0.00 (0 - 0)
15:48:02.784 > UL: 0 0  0.00 (0 - 0) | DL: 0 0  0.00 (0 - 0)
15:48:02.788 > UL: 0 0  0.00 (0 - 0) | DL: 0 0  0.00 (0 - 0)
15:48:02.793 > UL: 0 0  0.00 (0 - 0) | DL: 0 0  0.00 (0 - 0)
15:48:02.796 > UL: 0 0  0.00 (0 - 0) | DL: 0 0  0.00 (0 - 0)

The join-accept's CFList sets a block of eight channels plus one channel from the second span, but that last channel accidentally overwrites one of the first eight channels.

As soon as one ADR message comes through (which usually is on the next uplink/downlink), this one channel is removed and the set of eight channels is set again.
(15:48:08.769 > Channel UL 0 (8) frequency = 903.899963 MHz)

[jgromes]

Found a small bug in fixed bands

@StevenCellist thank you - I left one more comment, I think this PR can wait until whenever you get around to it.

[StevenCellist]

Turned out that (as I knew) the channel selection stuff was quite borked. Figured I couldn't even maintain my own logic, so revamped the whole channel setup and mask stuff. Expanding on the earlier mentioned changelog:

• [LoRaWAN] Rework channel setup and selection logic
• [LoRaWAN] Change initializer to take subband parameter, removing separate subband function
• [LoRaWAN] Update examples to reflect new subband selection
• [TypeDef] Remove duplicate error value
• Update keywords.txt

HUGE thanks to @HeadBoffin who helped test this all on US915. Tests did not reflect real-life scenarios, just stationary in good reach of gateway, but it appears to be stable and working fine just fine, as well for EU868.

[jgromes]

Thanks so much @StevenCellist (and @HeadBoffin too)!

I went through all the changes, and I think everything is good to go - though I won't pretend I completely understand the new channel selection. Let's see how this works in the real world ;)

[StevenCellist]

@jgromes thank you!!
I completely understand that you don't understand it, but I plan to work on a "general design document" / kind of a README which outlines general caveats w.r.t. use of persistent storage, as well as a sort of image that shows the general flow of function calls & setup. Maybe we can find a good place for both those topics to reside.

[StevenCellist]

BTW - any chance you will do a version bump, e.g. 6.3.1? That'll make it easier for people to try it out.

[jgromes]

Maybe we can find a good place for both those topics to reside.

I think that would be either the Wiki or the extras directory. Though the wiki is probably better since the README.md already links to it in multiple places.

any chance you will do a version bump

It's probably about time. There is ongoign work on Si4463 and Si4063, as well as LR11xx, but those are still early in development.

[jgromes]

@StevenCellist I noticed there's a couple more variable arrays, which could be simplified to simple statically allocated arrays. Although there is one which I'm not completely sure about:

RadioLib/src/protocols/LoRaWAN/LoRaWAN.cpp

Lines 247 to 250 in c1bf281

	uint8_t numADRCommands = mod->hal->getPersistentParameter<uint8_t>(RADIOLIB_EEPROM_LORAWAN_NUM_ADR_MASKS_ID);
	uint8_t numBytes = numADRCommands * MacTable[RADIOLIB_LORAWAN_MAC_LINK_ADR].lenDn;
	uint8_t buffer[numBytes] = { 0 };
	mod->hal->readPersistentStorage(mod->hal->getPersistentAddr(RADIOLIB_EEPROM_LORAWAN_UL_CHANNELS_ID), buffer, numBytes);

Is there a maximum size of this array that could be used?

[StevenCellist]

There could be as many ADR commands as there are values for the chMaskCntl (for fixed bands), which caps out at 8. So 8×5=40 bytes.

[StevenCellist]

@jgromes bit of an unusual place but also a bit more private:
Is there a chance that I could get access/rights to a page on the Wiki to put up stuff on the LoRaWAN implementation technical side + general guidelines for implementing this LoRaWAN v1.1 on end devices?
There are quite some questions recently and also some commonly repeated fixes, it would be nice to have a place to point to before we're starting to answer questions into eternity.

If it's possible, ideally mister HeadBoffin would also get access to that page so we can together build up a useful place :)

[jgromes]

@StevenCellist I think the easiest option is to invite both you and @HeadBoffin as collaborators, that should allow you to edit the wiki. I don't think there's a way to limit the acces to just one page, so you're getting the whole thing - with great power etc. etc. You should get the invite shortly.

Though honestly I think it's only right, seeing as you are continuously putting in the effort to support the LoRaWAN stack, which seems to be getting quite a bit of attention.

[StevenCellist]

WOW... thank you!! It's an honour to be part of RadioLib and an exciting and educational journey. We'll try our best to make it a useful wiki, and you may find that it will probably become a popular place :)