Setting the DIO functionality

[obones]

Hello,

Many chips include DIO pins that can output various internal states for use by the host MCU. For instance, the CC1101 can output a logic 1 when it sees the carrier (CarrierSense), the RFM69 does the same when it has seen a strong enough signal (RSSI) and so on.
I could not find a user-friendly way in RadioLib to set those pins functions and I'm using the low level SPIsetRegValue methods after having activated god mode.
But because it's not "safe" to use this mode, I'd like to create a pull request to add this functionality.
However, I would like to discuss the possible options with you.

At first, I thought of adding a method like this:

int16_t CC1101::setDIOMapping(uint8_t dio_reg, uint8_t value) {
  return(SPIsetRegValue(reg, value));
}

But this would have allowed users to "abuse" this method to set pretty much any register value.
And this does not translate well to RFM69 where multiple pins are in the same register, forcing users of RadioLib to learn about the mapping details.

So I think it would be better to have methods like this one:

int16_t CC1101::setDIOMapping(uint8_t pin, uint8_t value) {
  if (pin > 2)
    return RADIOLIB_ERR_INVALID_DIO_PIN;

  return(SPIsetRegValue(RADIOLIB_CC1101_REG_IOCFG0 - pin, value));
}

int16_t RF69::setDIOMapping(uint8_t pin, uint8_t value) {
  if (pin > 5)
    return RADIOLIB_ERR_INVALID_DIO_PIN;

  if (pin < 4)
    return(_mod->SPIsetRegValue(RADIOLIB_RF69_REG_DIO_MAPPING_1, value, 7 - 2 * pin, 6 - 2 * pin));
  else
    return(_mod->SPIsetRegValue(RADIOLIB_RF69_REG_DIO_MAPPING_2, value, 15 - 2 * pin, 14 - 2 * pin));
}

This could then be replicated for other boards, but I'm wondering if setDIOMapping should be declared as a virtual abstract method in the PhysicalLayer base class.

What do you think of this? Do you have other suggestions?

[jgromes]

In theory, this is a good addition; however, as usual, the devil is in the detail.

You can use the RADIOLIB_LOWLEVEL macro to only access the low-level methods, without exposing everything like god mode does. I consider this unsafe because it allows user to do anything, including things the library does not expect (e.g. switching the module to standby while it should be receiving).

The first option is definitely a no-go, since it would effectively allow to edit any register, as you correctly identified.

The second one is much better, but like you said, making it a PhysicalLayer abstraction would be the best. However, there are two questions that need to be examined before implementation:

1. What is the general interface? I assume it's going to have two arguments: pin number, and "function", which is effectively going to be some register value, defined as a RadioLib macro. The macros are going to be different for different modules. Is there a way to make them more generic? E.g. a "RADIOLIB_DIO_PACKET_RECEIVED" macro that would evaluate to the correct setting for different modules.

2. Is this a feature that would be useful to someone else? Possibly so, but I'm not sure.

[obones]

Thanks for your reply, at least I know I'm not completely off track here.
To answer your questions:

1. Yes, two arguments with pin number and "function" is what I have shown and what I had in mind. But I don't see how we could have more generic defines that would apply across all modules. I mean, even DATA has a different value depending on async mode or sync mode in CC1101. And it would force RadioLib to have a mapping from the generic value to a per module value. This is why I think using direct register value is enough as the associated macro has a meaningful name. I had a look at SX1276 and RF69 who have the same background, and even then the functions for the 6 DIO pins are mapped differently.
   What could be done, though, is for a given module to have a single definition for the same function. For instance, with RF69 in continuous RX mode, the RSSI signal can be mapped to DI00, DIO3 and DIO5 each time with a different bit setting. And in packet RX mode, it's available on DIO0, DIO3 and DIO4 with yet other values. Having 6 possible defines is what's (partially) already available in the RF69.h header file, but having RADIOLIB_RF69_DIO_RSSI given to setDIOMapping would be much more user friendly. This however has the drawback that it would require more code inside setDIOMapping and thus more flash space consumed on the device that runs RadioLib. And the end user would have to check for the return of setDIOMapping as it will return an error if the required function cannot be mapped to the given pin. For instance, setting RADIO_LIB_RF69_DIO_AUTOMODE on DIO4 is not be possible.
2. I'm using it with a CC1101 to start reading the data pin only when a carrier has been sensed. In between I put my host MCU in sleep mode as it has nothing else to do. Others might want to be notified of a packet arrival, or that the TX FIFO is empty, all while doing other things. Using those signals along with interrupts in the host MCU makes main loop and power management much easier.

[jgromes]

1. OK, let's keep it simple, just with macros. Regarding checking the output of setDIOMapping, that should be done anyway, since there's going to be at least an SPI transaction, which may fail. However, I don't think we need to check whether the value "makes sense", just that it's being set to the correct register. If the user provides DIOx setting that is not available on that pin, that's incorrect usage.
2. Seems like a reasonable use-case, so I will be glad if this is added!

[obones]

Here is a pull request for this, happy to get your feedback: #542