Main radio parameters set as constants and cached where possible

[phretor]

I cleaned up where the main radio parameters (carrier frequency, deviation, bitrate, sync word, etc.) are defined and changed and cached them to simplify get/set-ters and made them more consistent.

Worked only on CC1101, RF69 and nRF24 but I think other radios can be treated similarly.

[jgromes]

Thanks, is there a particular reason to cache every parameter, not just those that are actively used?

[phretor]

I think the usage of each parameter depends heavily on the type of application. My criterion is to cache those that 1) are passed to the constructor and 2) have at least a setter, a getter or both. A few bytes saves SPI transactions. 😊

[jgromes]

It does save SPI transactions, but the side effect is that the configuration will exist in two places - in the radio module and in the MCU RAM. If at some point those get out-of-sync, we need to decide which one has the higher priority.

For example, I can easily picture a situation where the radio module resets (or goes through a power cycle) unexpectedly. Since all modules supported by RadioLib store their configuration in non-volatile memory, it will get reset to the default values and now what RadioLib thinks is configured is no longer true. This is why the number of cached parameters was kept to the bare minimum. It could be argued that even that is insecure and that nothing should be stored in the library. Hence, every time we need a parameter value it should be read out from a register.

I'm a bit conflicted about this. On one hand, it will save transactions and it's already being done for some parameters. On the other hand it's less secure. In an ideal world we'd have both options implemented and let the user pick speed over security with a compile-time option, much like RADIOLIB_SPI_PARANOID.

[phretor]

@jgromes something like this would be acceptable?

#if defined(RADIOLIB_CACHE_RADIO_PARAMETERS)
  if(state == RADIOLIB_ERR_NONE) {
    _freq = freq;
  }
#endif

[jgromes]

@phretor it would, unfortunately it's not enough, a significant rework would be needed to make everything consistent, so that only cached or non-cached parameters are used.

[phretor]

@jgromes would you be OK if I remove those caching and create some getter methods that read from the registers? I have them in RFQuack but if they're useful I can remove them and push them down into RadioLib. It's a bit of a pain to implement some getters (e.g., frequency), but I like it better.

BTW, do you mind double checking that I revered the arithmetic correctly?

int16_t RF69::setFrequency(float freq) {
  // check allowed frequency range
  if(!(((freq > 290.0) && (freq < 340.0)) ||
       ((freq > 431.0) && (freq < 510.0)) ||
       ((freq > 862.0) && (freq < 1020.0)))) {
    return(RADIOLIB_ERR_INVALID_FREQUENCY);
  }

  // set mode to standby
  setMode(RADIOLIB_RF69_STANDBY);

  //set carrier frequency
  //FRF(23:0) = freq / Fstep = freq * (1 / Fstep) = freq * (2^19 / 32.0) (pag. 17 of datasheet) 
  uint32_t FRF = (freq * (uint32_t(1) << RADIOLIB_RF69_DIV_EXPONENT)) / RADIOLIB_RF69_CRYSTAL_FREQ;
  _mod->SPIwriteRegister(RADIOLIB_RF69_REG_FRF_MSB, (FRF & 0xFF0000) >> 16);
  _mod->SPIwriteRegister(RADIOLIB_RF69_REG_FRF_MID, (FRF & 0x00FF00) >> 8);
  _mod->SPIwriteRegister(RADIOLIB_RF69_REG_FRF_LSB, FRF & 0x0000FF);

  return(RADIOLIB_ERR_NONE);
}

float RF69::getFrequency() {
  uint32_t FRF = 0;
  float freq = 0.0;

  //FRF(23:0) = [     [FRF_MSB]|[FRF_MID]|[FRF_LSB]]
  //FRF(32:0) = [0x00|[FRF_MSB]|[FRF_MID]|[FRF_LSB]]
  FRF |= (((uint32_t)(_mod->SPIreadRegister(RADIOLIB_RF69_REG_FRF_MSB)) << 16) & 0xFF0000);
  FRF |= (((uint32_t)(_mod->SPIreadRegister(RADIOLIB_RF69_REG_FRF_MID)) <<  8) & 0x00FF00);
  FRF |= (((uint32_t)(_mod->SPIreadRegister(RADIOLIB_RF69_REG_FRF_LSB))      ) & 0x0000FF);

  //freq = Fstep * FRF(23:0) = (32.0 / 2^19) * FRF(23:0) (pag. 17 of datasheet) 
  freq = FRF * ( RADIOLIB_RF69_CRYSTAL_FREQ / (uint32_t(1) << RADIOLIB_RF69_DIV_EXPONENT) );

  return(freq);
}

int16_t RF69::setBitRate(float br) {
  RADIOLIB_CHECK_RANGE(br, 1.2, 300.0, RADIOLIB_ERR_INVALID_BIT_RATE);

  // check bitrate-bandwidth ratio
  if(!(br < 2000 * _rxBw)) {
    return(RADIOLIB_ERR_INVALID_BIT_RATE_BW_RATIO);
  }

  // set mode to standby
  setMode(RADIOLIB_RF69_STANDBY);

  // set bit rate
  uint16_t bitRate = 32000 / br;
  int16_t state = _mod->SPIsetRegValue(RADIOLIB_RF69_REG_BITRATE_MSB, (bitRate & 0xFF00) >> 8, 7, 0);
  state |= _mod->SPIsetRegValue(RADIOLIB_RF69_REG_BITRATE_LSB, bitRate & 0x00FF, 7, 0);

  return(state);
}

float getBitRate() {
  uint16_t bitRate = _mod->SPIgetRegValue(RADIOLIB_RF69_REG_BITRATE_MSB, 7, 0) << 8;
  bitRate |= _mod->SPIgetRegValue(RADIOLIB_RF69_REG_BITRATE_LSB, 7, 0);

  float br = bitRate / 32000.0;

  return(br);
}

[jgromes]

My goodness, I completely ignored this for so long - sorry @phretor :(

The upstream diverged quite a bit since this was created, so much so that I can't really tell anymore what are the changes in the PR. Would it be possible to create a new PR for the getter methods?

[phretor]

No worries. Also my strategy has changed as well: I decided to remove all cached parameters unless strictly needed and declare getters and setters instead. I'm not 100% sure that I decided the raw SPI data correctly, but I'll send a new PR for sure.

[jgromes]

Thanks, in that case I'll close this one. Again, sorry for taking so long.

[phretor]

@jgromes here - https://github.com/jgromes/RadioLib/pull/614