add functions to PhysicalLayer interface

[gasagna]

Hi, following our discussion I have added a few functions to improve the PhysicalLayer interface. Surely there are others missing, but these are those i need for now!

[jgromes]

Everything looks OK, thanks for the contribution. Feel free to open another PR in case you feel the need to add more methods.

[gasagna]

@jgromes, I discovered a bug in my PR today, for the startReceive function implementation. I had to define default values for the three arguments, because I thought that subclasses would define their own. The signature in PhysicalLayer.h is

virtual int16_t startReceive(uint32_t timeout = 0, uint16_t irqFlags = 0, uint16_t irqMask = 0);

while, for instance, in SX126X.h this is the signature:

int16_t startReceive(uint32_t timeout = RADIOLIB_SX126X_RX_TIMEOUT_INF, uint16_t irqFlags = RADIOLIB_SX126X_IRQ_RX_DEFAULT, uint16_t irqMask = RADIOLIB_SX126X_IRQ_RX_DONE);

However, calling startReceive(RADIOLIB_SX126X_RX_TIMEOUT_INF) on a SX1262 object does not seem to pick up the correct default values for the irqFlags and irqMask arguments, and it looks like the compiler is picking up the default values in the declaration in PhysicalLayer.h Did I forget an override?

[jgromes]

@gasagna I was not able to reproduce this - when I call startReceive(RADIOLIB_SX126X_RX_TIMEOUT_INF) and print the value of e.g. irqFlags, I get 0x262, which is the correct default value.

[gasagna]

Thanks, where do you read the irqFlags from?

I see what you mean. I put a print statement inside SX126x::startReceive to see what values the arguments have, but I see that the flags and the mask are both 0 (I have a SX1262 radio). Note that I am calling startReceive from a member of a class that wraps a PhysicalLayer object, see here. This might make a difference.

[jgromes]

I see - I was able to replicate this using a simple wrapper class:

class Wrapper {
  public:
    PhysicalLayer& _phy;
    Wrapper(PhysicalLayer& phy) : _phy(phy) { }
    int16_t startReceive(uint32_t timeout) {
      Serial.print("Wrapper::startReceive, timeout=");
      Serial.println(timeout);
      _phy.startReceive(timeout);
      return(0);
    } 
};

(...)

Wrapper w(radio);
w.startReceive(10);

This indeed produces the following:

Wrapper::startReceive, timeout=10
SX126x::startReceive, irqFlags=0

It is very similar to how standby is done: add SX126x::startReceive(uint32_t timeout = RADIOLIB_SX126X_RX_TIMEOUT_INF) and remove the defaults from the original SX126x::startReceive. Then the implementation of startReceive with a single timeout argument must call the full version startReceive(timeout, RADIOLIB_SX126X_IRQ_RX_DEFAULT, RADIOLIB_SX126X_IRQ_RX_DONE).

However, there's a bigger issue - the PhysicalLayer::startReceive method has to be common for all modules. CC1101, nRF24, RF69 and Si443x have startReceive without arguments. SX127x has a mode and len arguments, and SX126x/8x have the timeout and IRQ flags/mask. For the implementation in PhysicalLayer to make sense, it needs to be comon for all the modules.

[gasagna]

Thanks for confirming this. Not sure how to best address it. The question is if you'd like PhysicalLayer to be a complete base class for all the other radio drivers. I remember you have discussed this in the context of a LoRaWAN implementation in some issue in this repo.

[jgromes]

I think it would be best if we turn PhysicalLayer into a complete base class. It started as a collection of useful methods and abstraction for ham radio modes, but since then it clearly evolved.

Since this is will take some architectural decisions, I will do it myself and let you know once it's implemented.

[gasagna]

Thanks!

[jgromes]

@gasagna startReceive compatibility across different modules should be fixed now, building a common baseline is going to take a bit more time.