LoRaWAN: fcntUp wraps after 50 transmissions

barnslig commented 7 months ago

Bug description

The fcntUp wraps around after 50 transmissions, resulting in no more messages being received by the network server.

Reproduction

The bug can be reproduced as follows in god mode without sending 50 uplinks:

#include <Arduino.h>
#include <RadioLib.h>

SX1262 radio = new Module(8, 14, 12, 13);
LoRaWANNode node(&radio, &EU868);

void setup()
{
  Serial.begin(115200);

  delay(2500);

  node.wipe();

  for (unsigned int i = 0; i < 100; i += 1)
  {
    node.restoreFcntUp();

    node.fcntUp += 1;

    Serial.printf("fcntUp: %u | iteration: %u\n", node.fcntUp, i);

    node.saveFcntUp();
  }
}

void loop()
{
}

The output is as follows and shows the wrap around after 50 iterations:

Log output

Expected behavior

The fcntUp should not wrap before the uint32 is full.

Additional info

MCU: Heltec WiFi LoRa 32 V3
Wireless module: SX1262
Platformio framework version: framework-arduinoespressif32 @ 3.20014.231204 (2.0.14)
Library version: 6.4.2

StevenCellist commented 7 months ago

I have a suspicion, I will investigate. Good find!

StevenCellist commented 7 months ago

Got it. The node.restoreFcntUp() function was nicely implemented with 'dynamic sizing' of the NVM buffer size which got expanded from 6.3.0 to 6.4.0, but unfortunately I apparently had forgotten to also dynamicalize (inventing new words there) node.saveFcntUp().

If it bothers you personally, the fix is to change lines 818/819 to

  uint8_t fcntBuffStart = mod->hal->getPersistentAddr(RADIOLIB_EEPROM_LORAWAN_FCNT_UP_ID);
  uint8_t fcntBuffEnd = mod->hal->getPersistentAddr(RADIOLIB_EEPROM_LORAWAN_FCNT_UP_ID + 1);
  uint8_t buffSize = fcntBuffEnd - fcntBuffStart;
  #if RADIOLIB_STATIC_ONLY
  uint8_t fcntBuff[RADIOLIB_STATIC_ARRAY_SIZE];
  #else
  uint8_t* fcntBuff = new uint8_t[buffSize];
  #endif

  mod->hal->readPersistentStorage(mod->hal->getPersistentAddr(RADIOLIB_EEPROM_LORAWAN_FCNT_UP_ID), fcntBuff, buffSize);

And modify the two occurrences of simply '30' in the lines 859 & 864 to buffSize. I will include this fix in a PR later, probably next week.

barnslig commented 7 months ago

@StevenCellist thank you so much for your quick investigation! I'll apply your fix locally, looking forward to your PR :)

StevenCellist commented 7 months ago

Welcome and thank you for digging this out + providing the easy code for me to run. I suspect this also solves behaviour that I thought I dreamt of where some packets went missing, but it may also help in solving #949 which will come clear soon.

@jgromes while debugging this, I noticed that PhysicalLayer.h does not expose its private members upon godmode because of the protected: at its line 491 - is this intentional, or could you move that block to below private?

jgromes commented 6 months ago

@StevenCellist good catch - no, that is not intentional. I guess I never really used god mode to acces private members from PhysicalLayer. I will fix it so that your PR can remain focused on LoRaWAN stuff :)

jgromes / RadioLib