search

jgromes / RadioLib

Universal wireless communication library for embedded devices
https://jgromes.github.io/RadioLib/
MIT License
1.56k stars 390 forks source link

SX126x reports CRC mismatch upon previously received corrupted header #1202

Closed GUVWAF closed 2 months ago

GUVWAF commented 2 months ago

For SX126x (and SX128x and LR11X0) radios, a CRC mismatch is returned when either the header CRC or the payload CRC is incorrect: https://github.com/jgromes/RadioLib/blob/d3f9eaf30b438faa2406982a75f54be64f681909/src/modules/SX126x/SX126x.cpp#L709-L710 Although it is not clear from the data sheet, in explicit header mode I would assume the radio would not continue decoding the payload when receiving a corrupt header, because it doesn’t know whether the coding rate and packet length is correct. It will then set the HEADER_ERR IRQ, but it will not generate the RX_DONE IRQ. Then, when receiving a subsequent packet, the HEADER_ERR is still set, while it might not be corresponding to the current packet.

I confirmed this by enabling the HEADER_VALID IRQ flag and then caught a case where the HEADER_ERR IRQ was set, but also the HEADER_VALID IRQ, and also the payload CRC was okay.

I think there are three options for fixing this:

  1. Simply remove the check for HEADER_ERR, as it’s the payload we care about anyway;
  2. Enable the HEADER_VALID flag by default and ignore the HEADER_ERR when also a valid header is received;
  3. Return RADIOLIB_ERR_LORA_HEADER_DAMAGED when only the HEADER_ERR IRQ is set, such that the user can decide what to do in this case.

I would like to hear what you think such that I continue with a PR.

Edit: After reading the SX127x data sheet, I now understand why this was probably done. They mention there that if the header is corrupt, it might not have the "CRC on payload" bit set, so it would not generate the CRC error IRQ even if it was incorrect. They do not mention this in the SX126x datasheet, but to be safe maybe disregard option 1.

jgromes commented 2 months ago

I'm not sure I fully understand the issue - one of the things that happens in both startReceive and readData is to clear the IRQ flags, so regardless of what happened when receiving packet (N), all should be reset by the time interrupt for packet (N+1) fires and we start checking flags/reading the data. But from what you are describing, it seems to me that the HEADER_ERR IRQ flag survives that - would it be possible to confirm?

And yes, you are correct - the behavior of SX127x for mismatched CRC configuration is exactly where this originated from.

GUVWAF commented 2 months ago

What I believe happens is that if packet (N) has a corrupted header, it won't try to decode the payload and thus RX_DONE never fires, so it never enters readData(). In continuous Rx mode, we also do not call startReceive() again. Therefore, the IRQ flags are not cleared and when getting the RX_DONE interrupt for packet (N+1) the HEADER_ERR IRQ flag is still set.

I don't see another explanation otherwise for having both HEADER_VALID and HEADER_ERR set, and also no payload CRC error.

jgromes commented 2 months ago

I see - thank you for the clarification, it makes a lot of sense.

In terms of the options you have outlined, I think option 1 is the easiest, but may have unintended side effects, precisely becaue as you noted, the behavior for CRC configuration mismatch being unspecified in the SX126x datasheet. I also don't think option 3 is very straight-forward for the user. Presumably, the will get an interrupt, try to call readData, but that will return a RADIOLIB_ERR_LORA_HEADER_DAMAGED, which would mean that they have missed one packet, and to read the current one, they need to clear this interrupt flag to call radData again. So to me, option 2 makes the most sense.

However, I would propose to wait a bit with the PR before #1190 is merged, as that does a significant rework to the IRQ handling, basically introducing an IRQ abstraction to the PHY.

GUVWAF commented 2 months ago

Thanks for your answer - option 2 would indeed also have my preference now. I’ll wait for that PR (looks good!) and then create a PR which basically only reports CRC mismatch for SX126x, SX128x and LR11x0 if the payload CRC failed, or HEADER_ERR is set and HEADER_VALID is not.

For your information, I found this after the Meshtastic app added bad/correct packet reception statistics and users were reporting relatively many “bad” packets (https://github.com/meshtastic/firmware/issues/4578). Hopefully this at least partially resolves that.