Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SX127x] When clearing the FHSS interrupt, don't also clear all the others #1155

Merged

Conversation

SebKuzminsky
Copy link

Writing 1 bits to the ReqIrqFlags register clears the corresponding interrupt sources.

Screenshot from 2024-07-08 15-39-08

Before this commit, the clearFHSSInterrupt() function would clear all set bits, plus unconditionally clear the FhssChangeChannel bit. This could cause the following problem:

  1. A receiver receives a packet, hopping frequencies as requested by the FhssChangeChannel interrupt. Each interrupt clears all other interrupt bits.
  2. At the conclusion of the packet, the SX127x simultaneously sets FhssChangeChannel to request a hop back to the starting channel, and also sets RxDone to indicate that there is a complete packet to read from the FIFO. If the packet had CRC enabled and the CRC failed, the PayloadCrcError bit will also be set at this point.
  3. If the receiving application services the FhssChangeChannel interrupt before the RxDone interrupt, the other two bits (RxDone and PayloadCrcError) will be lost. At worst this will mean that the packet will not be read (but will remain in the FIFO). At best this will mean that readData() will not know that the packet failed CRC, and will deliver a corrupted packet.

The old behavior seems like a just a simple bug, if i've missed something here please let me know.

@jgromes
Copy link
Owner

jgromes commented Jul 9, 2024

Very well spotted - thank you! Did you test this change with the hardware?

Looking at the example code, I think the reason we have not seen this be an issue before is that there are two flags, receivedFlag and fhssChangeFlag which are set by different ISRs. It is likely that the interrupts at the end of packet fire in a quick succession, so if the application checks starts processing fhssChangeFlag, it is likely that the receivedFlag will be set by the other ISR before clearFHSSInterrupt is called. Bit of a race condition there ...

@SebKuzminsky
Copy link
Author

Yes, i'm running this code on real hardware. It works well in for my application.

@SebKuzminsky
Copy link
Author

In the application i'm working on we handle FhssChangeChannel before RxDone. By the time we process RxDone the PayloadCrcError bit had been cleared, and we'd deliver corrupted packets up to the application. The proposed patch fixes this issue.

@jgromes jgromes merged commit 5185443 into jgromes:master Jul 9, 2024
30 checks passed
@jgromes
Copy link
Owner

jgromes commented Jul 9, 2024

@SebKuzminsky thank you for the extra context - CI reports all green so I merged this.

Thank you for hte contribution!

@SebKuzminsky
Copy link
Author

Thanks for developing & maintaining RadioLib, it's super useful!! :-)

@SebKuzminsky SebKuzminsky deleted the fix-clear-fhss-interrupt-6.6.0 branch July 9, 2024 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants