How to discard the packet after hasData returns?

[leojay]

Hi,

First of all, thanks for the great library! It makes RF communication much easier!

When reading the code of readData, It seems to me that the "readData" function just writes everything from the packet into the parameter "data" disregarding of the actual size of the argument.
Here is the source code of the function:

    // Determine length of data in the RX buffer and read it.
    uint8_t dataLength;
    spiTransfer(READ_OPERATION, R_RX_PL_WID, &dataLength, 1);
    spiTransfer(READ_OPERATION, R_RX_PAYLOAD, data, dataLength);

This can be dangerous because the packet size can be larger than the actual size of the argument "data".
e.g. for code like this:

if (_radio.hasData()) {
  uint32_t value;
  _radio.readData(&value);
}

If a packet has more than 4 bytes in it, the memory will get corrupted.

I'm wondering whether I can somehow discard the packet when the packet data size is not expected?

Something like this:

uint32_t data_length = _radio.hasData();
if (data_length == 4) {
  uint32_t value;
  _radio.readData(&value);
} else if (data_length != 0) {
  // <-- What can I do here to discard the packet?
}

Or better, can you please change the signature of the function "readData" and add a second parameter "uint8_t dataLength"? So that the memory will not be corrupted even if the packet has too many bytes in it?
Thanks!

Leo

[dparson55]

Hi Leo, you can call init if you don't want to handle all the packets you are sending from other radios. This will remove all unread packets sitting in the RX buffer. In years of real world use, there have been no reports of an errant packet of unexpected length getting through the radio's packet processing engine, so that's a possibility I can recommend as not worth considering at this point.

I hesitate to add a dataLength parameter to readData since this would break back-compatibility, but it is a good idea, thanks! If the library was not already out for several years, it would be a much easier decision. Most of the time users only send a single type of packet, so they don't have code which looks at the size of packets. Conversely if a user is sending differently sized packets, they already have code to consider the result of hasData as you have done. So at this point in the library's life, I think the costs would outweigh the benefits.

[leojay]

Thanks for your reply! I will try "init" to workaround this.

Do you think adding a default value of 255 (i.e. "uint8_t dataLength = 255") can solve the back-compatibility problem?
It's interesting that I'm the first one to report this. I wrote a small piece of code to send 4-byte packets back and forth between 2 MCUs. While testing the communication in a busy loop, I found that sometimes it got packets with more than 4 bytes in it. That's why I dug into the source code of readData.

[dparson55]

No kidding, that is a first for me as well! Thanks for the report and now I really wish I would have included dataLength as a required parameter to readData.

I tried writing an update to readData with a default value for expectedDataLength = 0 back when I read your comment. It turned out as shown below.

uint8_t NRFLite::readData(void *data, uint8_t expectedDataLength)
{
    uint8_t statusReg = readRegister(STATUS_NRF);
    if (statusReg & _BV(RX_DR))
    {
        writeRegister(STATUS_NRF, statusReg | _BV(RX_DR));
    }

    uint8_t actualDataLength;
    spiTransfer(READ_OPERATION, R_RX_PL_WID, &actualDataLength, 1);

    if (expectedDataLength > 0)
    {
        if (actualDataLength != expectedDataLength)
        {
            spiTransfer(WRITE_OPERATION, FLUSH_RX, NULL, 0);
            return 0;
        }
    }
    
    spiTransfer(READ_OPERATION, R_RX_PAYLOAD, data, actualDataLength);
    return 1;
}

I didn't like this because I had to make readData return a uint8_t so that a user could determine if the data was of the expected length and actually read into the data parameter. Rather than having to do all that, a user should instead use the result of hasData from the start and act on it rather than having this function protect against an unexpected packet length. So the perfect solution is what you originally suggested, it's just that it comes at the huge price of back-compatibility.

[leojay]

Sorry, I guess I didn't explain myself clearly.
Instead of expectedDataLength, I was suggesting maxDataLength

uint8_t readData(void *data, uint8_t maxDataLength=255);

Just like the signature of the linux function read (link to the doc):

ssize_t read(int fd, void *buf, size_t count);

count is the maximum number of bytes the function read will write into the buffer buf. The actual number of data written into buf can be less than count.

The default value of maxDataLength is 255, so if the user doesn't care about the data length, they can use it just like before.

_radio.readData(data);

Everything in the packet will be written into data. It completely has the same behavior as before.

But, if maxDataLength is specified, the data will be truncated at that length:
e.g.

char name[16];
_radio.readData(name, 16);

This means that if there are only 5 bytes in the packet, fine, 5 bytes are written into name.
If there are, say 20 bytes in the packet, only the first 16 bytes are written. The rest are discarded.

The problem of using the result of hasData and acting on it is that it's not obvious how to "act". And that's exactly why I filed this issue: when I get too many data, how do I discard the packet?

[dparson55]

When a radio receives a packet with an unexpected data length, it means that portions of the packet were valid (preamble, address, and CRC) while an unknown number of portions of the packet are invalid. We know for certain that payload data length is wrong in the situation you ran into, but packet identity, the acknowledgement flag, and the data payload could also all be wrong as well.

Due to this, we really cannot trust the data in the packet. So I made an improvement to the library to help with the situation you ran into, and updated the example to show its use (helping to make it more obvious what to do). How's this look?

https://github.com/dparson55/NRFLite/compare/discardData

[leojay]

Cool! Thanks!