cpu/cc2538: Enable CRC checking of received packets

[aeneby]

Drop packets which have invalid CRC.

[hexluthor]

@aeneby I think you should be using rfcore_peek_rx_fifo(RFCORE_XREG_RXFIFOCNT) instead of rfcore_peek_rx_fifo(RFCORE_XREG_RXFIFOCNT - 1). But even better, consider using rfcore_peek_rx_fifo(pkt_len). RXFIFOCNT could change during the call to _recv(), but pkt_len won't.

[aeneby]

@aeneby I think you should be using rfcore_peek_rx_fifo(RFCORE_XREG_RXFIFOCNT) instead of rfcore_peek_rx_fifo(RFCORE_XREG_RXFIFOCNT - 1)

You're right - RFCORE_XREG_RXFIFOCNT doesn't include the length byte itself. Fixed.

But even better, consider using rfcore_peek_rx_fifo(pkt_len)

Hmm, this might be a bad idea, since the length byte is not guaranteed to be correct (in case of interference, for example). We actually do a check for this condition in the if statement immediately following this CRC check.

[aeneby]

I guess we could change the order of - or even combine - the checks, but the end result will be the the same.

[hexluthor]

Your are right that the length byte may not be the same as transmitted in case of interference. But it will always indicate the length of the received frame (and hence the offset of the RSSI and LQI bytes).

Suppose that by the time _recv() is called the radio has already started receiving a second packet. If we check the RSSI/LQI bytes using RXFIFOCNT as a reference, we could be making decisions about the first packet using unrelated data from the second packet. We could also be incorrectly discarding a valid packet just because there are more bytes than we expected in the FIFO. Trusting pkt_len avoids these problems.

[aeneby]

Your are right that the length byte may not be the same as transmitted in case of interference. But it will always indicate the length of the received frame (and hence the offset of the RSSI and LQI bytes).

I see what you mean. I guess the only case we have to watch for is when pkt_len is larger than the FIFO itself, since then we know it's full of garbage and can just flush it. See updated commit.

We could also be incorrectly discarding a valid packet just because there are more bytes than we expected in the FIFO.

True, this is something we need to consider. We shouldn't really be flushing the FIFO on failed CRC, but rather checking if there's another packet arrived and advancing the FIFO pointer to it or something. But perhaps that's outside the scope of this PR - there are likely other places we need to do this too.

[hexluthor]

Thanks for making the tweaks @aeneby. Looks good on BOARD=cc2538dk. I'll merge soon if there are no objections. I agree about the extra FIFO flushes. There is some room for improvement there but in a future PR.

[kYc0o]

didn't Murdock look at this one?

[hexluthor]

@kYc0o sorry, my bad.

[aeneby]

Cheers!

[kYc0o]

Uhmm... well, let's put more attention for the next ones and to explicitly say "ACK" when the PR is ready to be merged.