Feature request: expose LoRaWAN fcnt / channel / power for logging

[StevenCellist]

I've been trying out the LoRaWAN examples, mostly without difficulty. Great work!
However, for some purposes, I would very much like to be able to get some uplink (and maybe some downlink) properties, such that they can be logged for e.g. coverage mapping.
Most importantly, these are the fcnt, frequency/channel and TX power.
I imagine that the latter two would be part of the radio implementation, while the first one (for both uplink and downlink) are node functions.
Could these be exposed through functions like node.getFcnt() and radio.getFreq()? It would be much appreciated!
Cheers, Steven

[jgromes]

I think this can be added - the public API of the LoRaWAN implementation is not fixed yet (for example, I don't yet know how to pass the port from downlink frames). Which frame counter(s) exactly are you interested in?

[StevenCellist]

Maybe a bit of a bold suggestion - but would it be an idea to make an Event class with all 'loggable' properties? Payload, (un)confirmed, fport, fcnt, power (I may be missing some). And then it would generate an event for each uplink and downlink individually.

[jgromes]

@StevenCellist not entire sure how exactly the Event class should work - could you elaborate with some pseudo-code to better illustrate?

[StevenCellist]

I'm thinking in terms of the following:

typedef enum {
    UPLINK,
    DOWNLINK
} LoRaWANPktDir;

typedef enum {
    UNCONFIRMED,
    CONFIRMED
} LoRaWANPktConf;

struct {
    LoRaWANPktDir direction;     // these could be booleans just as well
    LoRaWANPktConf confirmed;
    int frequency;
    int fcnt;
    int fport;
    char message[256];
} LoRaWANEvent 

The fields would get populated at places such as LoRaWAN.cpp#410

uint32_t fcnt = mod->hal->getPersistentParameter<uint32_t>(RADIOLIB_PERSISTENT_PARAM_LORAWAN_FCNT_UP_ID) + 1;
event.fcnt = fcnt;

And in code it would read like this:

state = node.uplink(strUp, 10);
LoRaWANEvent eventUp = node.lastEvent();
// retrieve properties such as eventUp.fcnt or eventUp.frequency

state = node.downlink(strDown);
LoRaWANEvent eventDown = node.lastEvent();
// retrieve properties such as eventDown.fport or eventDown.message

I hope that shows what I'm thinking of - but I have no idea if it's your style of programming or how you typically handle things in RadioLib.

[HeadBoffin]

These values are an actual part of the uplink/downlink - separating them out to a different object / data structure would be rather odd in my not so humble opinion and as the downlink contents is returned via the *data & *len parameters.

Perhaps the parameter could be a *struct which would allow extras to be added with just the one breaking change in the API before people really get started?

[HeadBoffin]

Is this in your PR?

[StevenCellist]

No, the PR is purely focused on persistence through sleep & powerdown + adhering to v1.1 spec. Feel free to add something :)

[jgromes]

@StevenCellist I think I like @HeadBoffin's approach better - passing pointer some "extra info" struct as one of the arguments to uplink/downlink leads to better atomicity, manually requesting this information by calling a different method can lead to synchronization issues if the user (incorrectly) calls the method after another uplink. The argument can be set to NULL by default, to signal the user does not want any auxilliary information.

I'll implement this after #835 is merged (since it's a separate functionality from that PR).

[jgromes]

@StevenCellist @HeadBoffin I started working on this, currenty the struct looks like this. Anything else you think might be useful?

struct LoRaWANEvent_t {
  /*! \brief Event direction, one of RADIOLIB_LORAWAN_CHANNEL_DIR_* */
  uint8_t dir;
  
  /*! \brief Whether the event is confirmed or not */
  bool confirmed;
  
  /*! \brief Frequency in MHz */
  float freq;
  
  /*! \brief Transmit power in dBm for uplink, or RSSI for downlink */
  int16_t power;
  
  /*! \brief The appropriate frame counter - for different events, different frame counters will be reported! */
  uint32_t fcnt;
  
  /*! \brief Port number */
  uint8_t port;
};

[StevenCellist]

Much appreciated!
However, I would suggest you maybe wait a few days - there's a huge PR upcoming to revamp the whole band-system, Rx windows, includes ADR and better NVM handling that (yes!) even partially supports non-EEPROM builds.
Apart from some minor details (and some bugs most definitely), it would be the complete deal!

If you'd like, you could do some initial testing before we start the PR. The Rx windows are still a bit of a problem and could use some help.

[jgromes]

That's amazing, looking forward to that! Of course, this issue can wait - I'll take a look into your fork.

[StevenCellist]

Before you dig in completely: I have local changes that I still need to verify. Will let you know once those are online as well. But all the work is in the second branch of my fork.

[StevenCellist]

All the changes are online.
Some points worth mentioning:

• only works for the SX1261/2 currently due to the way downlink must be handled
• couldn't switch fast enough from CAD to RX for low SF / high DR so doing it the 'old-fashioned' way
• there's probably a minor problem in the RxTimeout stuff, I can't nail it down exactly. Works now because of looking at IRQ values directly, hopefully it can work through the callbacks but it requires very precise timing / knowledge
• there's probably some MIC calculation problem for confirming a downlink through an uplink - the ACK bit is set but uplink is not accepted

[StevenCellist]

Spent another day on the above mentioned points. Current state of the code:

• confirmed uplinks and downlinks are working now
• OTAA join-accept is listened for through downlinkCommon (same for downlink)
• an attempt is made to make the code module-agnostic, but functions are only implemented for SX126x and I'm not completely sure that it will work just as well for SX127x (or others) if their functions are implemented - I have only SX126x in my possession.

Please give it a shot and see how it fares; we'll need to get the SX127x sorted and maybe then we're ready for broad testing / pre-release??

[HeadBoffin]

• an attempt is made to make the code module-agnostic, but functions are only implemented for SX126x and I'm not completely sure that it will work just as well for SX127x (or others) if their functions are implemented - I have only SX126x in my possession.

I can test this.

[StevenCellist]

@jgromes came to think of this once again - the info in your struct looks good, although it may be worth to discern two types of confirmation: a downlink can be confirming a confirmed uplink, or a downlink can be confirmed requiring a confirming uplink. Yes, that sounds horrible, but your idea would not be able to tell those apart. The info for these two booleans is readily available in the PR through isConfirming and isConfirmed.

I also remembered that we'll need a way to get MAC info from and to the user, but I propose we delay that and solve that separately later in another way.

[jgromes]

a downlink can be confirming a confirmed uplink, or a downlink can be confirmed requiring a confirming uplink

Should be easy enough, instead of having just bool confirmed we can have two variables/flags mirroring those in the PR, bool confirmed and bool confirming.

I also remembered that we'll need a way to get MAC info from and to the user

Agreed, let's get the major things out of the first.

[StevenCellist]

We have another option as well: create a function .getDatarate() with the direction as an argument, or add the dr field to the struct. Especially for the fixed bands, the datarate can differ for uplink and downlink. I do not have a strong preference personally for one of either approaches.

[jgromes]

Implemented in the latest couple of commits.