Handling Rx interrupt too slow causes SX126x Rx buffer offset to be non-zero

[GUVWAF]

I’ve been debugging a strange issue when receiving short packets quickly after each other using an SX1262 running Meshtastic. I found that I was getting packets with a different payload length than was transmitted, and sometimes twice the same packet with different payload length. Finally I found it was caused due to that we were not calling readData() fast enough after the RX_DONE interrupt, probably caused by e-ink updates. Since the Rx buffer pointer of the radio didn’t get reset to 0 before receiving the next packet using setBufferBaseAddress(), the radio appended the next packet after the previous one in the buffer. However, readBuffer() is only called with an offset of 0.
I verified this by printing the rxStartBufferPointer (first byte of the last packet received according to the datasheet) just before calling readData(). Normally it’s 0, but when I got the wrong payload length, the pointer was exactly equal to the expected payload length.

I’m posting this here mostly as an observation, because we should really fix our issue of slow interrupt handling in Meshtastic when using e-ink screens. However, I was wondering if it would make sense to try to recover from this state in Rx continuous mode? E.g. we could check whether the rxStartBufferPointer was really 0, and if not, we can read from 0 till this pointer. Then in the next readData(), we should be reading from the rxStartBufferPointer with the new packet length. This wouldn’t require an additional SPI read, because the pointer is already returned when calling getPacketLength().

Anyway, just a thought because it would be mostly a band-aid solution for something that shouldn't be happening at all.

Edit: Recovering both packets can't be done reliably, because you also don't know the IRQ status for e.g. the CRC check. However, to match the data with the packet length, I believe it would make sense to use the rxStartBufferPointer as offset for readBuffer().

[jgromes]

Hmm, I'm not sure this can be recovered from on the library side. Essentially, the problem is that there are two packets saved in the FIFO. So the real question is: do we want readData to return:

1. Everything in FIFO
2. The first packet received
3. The last packet received

Right now it's option 1, I think that's mostly for consistency with other modules like SX127x, which do not allow to manually change the read pointer.

[GUVWAF]

Currently it’s option 2, right? It only reads from base address 0, which is the first packet.
The issue with that is that GetRxBufferStatus returns the length of the last packet received, which is likely different from the first packet received and hence you read too much or too little from the buffer. Besides, you might read the same data twice if you handle both stacked interrupts (this is less of an issue in my opinion).

So I think option 3 would be better to have the length corresponding to the reading start address.

[GUVWAF]

Another reason why I would opt for option 3 is that the buffer gets overwritten.
I used the transmit and receive interrupt examples to transmit two packets quickly after each other, first with 250 bytes, then with 16 bytes. The payload is an incrementing byte array: {0x00, 0x01, …, 0xF9}. For the receiver, I just added a delay(1000) in the loop to mimic a long processing time.

Current master results in the following data received:
6789ABCDEFABCDEF

So, it returns the packet length of last packet and it reads the beginning of the buffer, which is partly overlapped by data of the second packet.

When reading from the new offset, it gets the correct data of the second packet:
0123456789ABCDEF

If you agree I can create a PR to start reading from the offset retrieved with the GET_RX_BUFFER_STATUS register for SX126x and SX128x (LR11x0 already uses this).

[jgromes]

Currently it’s option 2, right? It only reads from base address 0, which is the first packet.

Come to think of it again, you are right, it is mostly option 2 on SX126x - we are reading from 0 to the length of last packet. So depending on the lengths of the two packets, it will be 1st packet + some portion of the 2nd, or not even the full 1st packet.

If you agree I can create a PR to start reading from the offset retrieved with the GET_RX_BUFFER_STATUS

I think it will be useful - however, I would also suggest to add a warning printed with RADIOLIB_DEBUG_BASIC, to let the user know that something isn't exactly right (i.e. the base offset is not zero). This seems like the sort of automated behavior that can easily mask other bugs.

[GUVWAF]

Indeed the warning would be good.

However, I’ll need to do some more testing with this. It seems even when handling the packets correctly, the returned RxStartBufferPointer is non-zero, while the data is at zero because setBufferBaseAddress() is called in the previous readData(). The RxStartBufferPointer will be zero when going to standby in between, which is what Meshtastic does.

Removing setBufferBaseAddress() from readData() (which it seems the SX128x already doesn't have) and then always reading it with the returned offset results in correct behavior.

[jgromes]

Resolved by merging #1185 - thank you very much @GUVWAF for this fix!