Skipping CRC OFF state at SX1278::setSpreadingFactorRaw()

[HamzaHajeir]

Hi

I'm reading the library to make a compatibility to STM32 HAL library I use.

I've came across this line, Which I expect it's a bug :

in SX1278.cpp L496 :

int16_t SX1278::setSpreadingFactorRaw(uint8_t newSpreadingFactor) {
  // set mode to standby
  int16_t state = SX127x::standby();

  // write registers
  if(newSpreadingFactor == SX127X_SF_6) {
    state |= _mod->SPIsetRegValue(SX127X_REG_MODEM_CONFIG_1, SX1278_HEADER_IMPL_MODE, 0, 0);
    state |= _mod->SPIsetRegValue(SX127X_REG_MODEM_CONFIG_2, SX127X_SF_6 | SX127X_TX_MODE_SINGLE | SX1278_RX_CRC_MODE_ON, 7, 2); // <<<<<<<<<< This line
    state |= _mod->SPIsetRegValue(SX127X_REG_DETECT_OPTIMIZE, SX127X_DETECT_OPTIMIZE_SF_6, 2, 0);
    state |= _mod->SPIsetRegValue(SX127X_REG_DETECTION_THRESHOLD, SX127X_DETECTION_THRESHOLD_SF_6);
  } else {
    state |= _mod->SPIsetRegValue(SX127X_REG_MODEM_CONFIG_1, SX1278_HEADER_EXPL_MODE, 0, 0);
    state |= _mod->SPIsetRegValue(SX127X_REG_MODEM_CONFIG_2, newSpreadingFactor | SX127X_TX_MODE_SINGLE | SX127x::_crcEnabled ? SX1278_RX_CRC_MODE_ON : SX1278_RX_CRC_MODE_OFF , 7, 2); // <<<<<<<<<< This line
    state |= _mod->SPIsetRegValue(SX127X_REG_DETECT_OPTIMIZE, SX127X_DETECT_OPTIMIZE_SF_7_12, 2, 0);
    state |= _mod->SPIsetRegValue(SX127X_REG_DETECTION_THRESHOLD, SX127X_DETECTION_THRESHOLD_SF_7_12);
  }
  return(state);
}

Shouldn't the two lines pointed to previously be :

state |= _mod->SPIsetRegValue(SX127X_REG_MODEM_CONFIG_2, SX127X_SF_6 | SX127X_TX_MODE_SINGLE | SX127x::_crcEnabled ? SX1278_RX_CRC_MODE_ON : SX1278_RX_CRC_MODE_OFF , 7, 2);

and
state |= _mod->SPIsetRegValue(SX127X_REG_MODEM_CONFIG_2, newSpreadingFactor | SX127X_TX_MODE_SINGLE | SX127x::_crcEnabled ? SX1278_RX_CRC_MODE_ON : SX1278_RX_CRC_MODE_OFF , 7, 2);

respectively ?

Because It will skip the CRC in case of being at OFF state.

Thanks

[jgromes]

You're right that the configuration for SF6 will indeeded ingore CRC configuration and always enable it. However, I don't see what's wrong with the second line, could you elaborate?

EDIT: Ah, both of them are actually wrong in the code - @HamzaHajeir you can insert links to actual files in the repository, this is the corresponding code section in the current revision:

RadioLib/src/modules/SX127x/SX1278.cpp

Lines 494 to 504 in 81135e0

	if(newSpreadingFactor == SX127X_SF_6) {
	state |= _mod->SPIsetRegValue(SX127X_REG_MODEM_CONFIG_1, SX1278_HEADER_IMPL_MODE, 0, 0);
	state |= _mod->SPIsetRegValue(SX127X_REG_MODEM_CONFIG_2, SX127X_SF_6 | SX127X_TX_MODE_SINGLE | SX1278_RX_CRC_MODE_ON, 7, 2);
	state |= _mod->SPIsetRegValue(SX127X_REG_DETECT_OPTIMIZE, SX127X_DETECT_OPTIMIZE_SF_6, 2, 0);
	state |= _mod->SPIsetRegValue(SX127X_REG_DETECTION_THRESHOLD, SX127X_DETECTION_THRESHOLD_SF_6);
	} else {
	state |= _mod->SPIsetRegValue(SX127X_REG_MODEM_CONFIG_1, SX1278_HEADER_EXPL_MODE, 0, 0);
	state |= _mod->SPIsetRegValue(SX127X_REG_MODEM_CONFIG_2, newSpreadingFactor | SX127X_TX_MODE_SINGLE | SX1278_RX_CRC_MODE_ON, 7, 2);
	state |= _mod->SPIsetRegValue(SX127X_REG_DETECT_OPTIMIZE, SX127X_DETECT_OPTIMIZE_SF_7_12, 2, 0);
	state |= _mod->SPIsetRegValue(SX127X_REG_DETECTION_THRESHOLD, SX127X_DETECTION_THRESHOLD_SF_7_12);
	}

[jgromes]

Fixed in 8438aa9, thanks for reporting. Let me know if you find anything else.

[HamzaHajeir]

Oh Great! I'm glad for the contribution

Thanks for the great library :)

[zhgzhg]

Hi @jgromes ,

While trying the changes, the following fragments turned out to be problematic:

SX1272_HEADER_IMPL_MODE | SX127x::_crcEnabled ? SX1272_RX_CRC_MODE_ON : SX1272_RX_CRC_MODE_OFF,
SX127X_TX_MODE_SINGLE | SX127x::_crcEnabled ? SX1278_RX_CRC_MODE_ON : SX1278_RX_CRC_MODE_OFF,

Surrounding the ternary conditionals with parentheses will fix the ...RX_CRC_MODE... from becoming the actual register value:

SX1272_HEADER_IMPL_MODE | (SX127x::_crcEnabled ? SX1272_RX_CRC_MODE_ON : SX1272_RX_CRC_MODE_OFF),
SX127X_TX_MODE_SINGLE | (SX127x::_crcEnabled ? SX1278_RX_CRC_MODE_ON : SX1278_RX_CRC_MODE_OFF),

[jgromes]

@zhgzhg thanks, reminds me why I dislike ternary so much. It's way too easy to forget about operator priority.

[HamzaHajeir]

It's a new thing to me :

For other people to evaluate : run this under any compiler, as cpp.sh :

// Example program
#include <iostream>
#include <string>
#define EIGHT 0x8
#define FOUR 0x4
#define TWO 0x2
#define ZERO 0x0
int main()
{
  std::string name;
  bool two = true;
  std::cout << (EIGHT | FOUR | (two ? TWO : ZERO) ) << std::endl;
}