lora: Fix IRQ bit mask for sx126x

[maholli]

Fixes #1112 which causes corrupt packets to silently be passed as valid by the driver.

[projectgus]

Hi @maholli, sorry it took me a little while to get back to this. Thanks for very clear description of the problem and the fix.

I have a request about the code comment surrounding this change, and also please edit micropython/lora/lora-sx126x/manifest.py and bump the bugfix version of this package so mip will update it.

Thanks again!

[projectgus]

@maholli I went to apply the suggestions above so I could quickly merge this, but I realised that there's another bug here - if dio1 is not set (i.e Python code will poll for interrupt status) then the mask bits are set to 0 and probably a number of things don't work!

I've pushed a fix here: projectgus@44b6fca

Any chance you're in a position to test this? If it looks good to you then you can cherry-pick it here, or I can open a new PR.

[maholli]

@maholli I went to apply the suggestions above so I could quickly merge this, but I realised that there's another bug here - if dio1 is not set (i.e Python code will poll for interrupt status) then the mask bits are set to 0 and probably a number of things don't work!

I've pushed a fix here: projectgus@44b6fca

Any chance you're in a position to test this? If it looks good to you then you can cherry-pick it here, or I can open a new PR.

@projectgus I think this would cause unwanted (or at the very least, different) behavior compared to the current lib.

I agree that including _IRQ_DRIVER_RX_MASK makes sense based on the name, but since the driver hasn't been doing that, by adding it now it will introduce _IRQ_HEADER_ERR to the actual IRQ mask which I think might have strange behavior. Based on my testing, _IRQ_HEADER_ERR is firing all the time but unlike the other bits it doesn't trigger an "RxDone" event. That means by the time a real packet comes along and RxDone triggers and you read the IRQ byte, you have a high likelihood of a stale _IRQ_HEADER_ERR bit hanging out in there which can cause the valid packet to be tossed.

So I agree that the driver could use other tweaks, but I think it'll take dedicated time to test and understand the implications of the changes - deserving more time and attention than I have to spare :)

Thoughts?

[projectgus]

Thanks for looking into this, @maholli!

_IRQ_HEADER_ERR is firing all the time but unlike the other bits it doesn't trigger an "RxDone" event. That means by the time a real packet comes along and RxDone triggers and you read the IRQ byte, you have a high likelihood of a stale _IRQ_HEADER_ERR bit hanging out in there which can cause the valid packet to be tossed.

Oh, that's interesting. I assumed it'd be the same as _IRQ_CRC_ERR, which I don't believe we manually reset either - IIUC that bit is updated each time a new packet is received instead of being sticky.

Agree that needs some more testing to be confident about it, so will merge this less ambitious PR first!

[projectgus]

Squashed and pushed as single commit 9f7f99b

Thanks again!

[maholli]

Squashed and pushed as single commit 9f7f99b

Thanks again!

@projectgus thank YOU for all your awesome work. Feel free to tag me if/when you start working on the other fixes and I'll help if I can.