Added non-blocking radio receive

[eivindbergem]

I've added a simple non-blocking receive. The method returns a struct that takes care of checking status and calling cancel_recv(). Now it calls cancel_recv() for every drop. I can add some state to avoid this if needed.

I didn't put much thought into the names, so please feel free to bikeshed.

[BartMassey]

Thanks much for the contribution!

Sorry about the CI failure, which is new: PR #486 should fix it shortly, after which you'll need to rebase.

Should this be adapted to support the nb crate infrastructure?

[eivindbergem]

Should this be adapted to support the nb crate infrastructure?

Yes, that's a good idea :) I'll get on to it.

[eivindbergem]

If is_done() is called after having received an Ok() or a CRC error, you would get a WouldBlock for ever:

let recv = radio.recv_non_blocking(&mut packet);
recv.is_done(); // -> WouldBlock
recv.is_done(); // -> Ok(crc)
while recv.is_done() == Err(nb::Error::WouldBlock) {} // Would never leave loop

This is not great. I could add some internal state and another error to indicate that we already have received:

enum RecvError {
    CrcFailure(u16),
    AlreadyReceived,
}

Or, maybe subsequent calls to is_done() should just repeatedly return Ok(crc) or Err(nb::Error::Other(crc)).

[BartMassey]

Is this with nb or without? I've lost track.

[eivindbergem]

This applies to both, with and without nb.

I'm not sure if it matters that much anyway, I was just thinking out loud. Futures suffer from the same issue, and the documentation makes it clear that futures that have returned Ready should not be polled.

[eivindbergem]

I refactored recv() and recv_timeout() to use recv_non_blocking() under the hood. I removed Event::End as it is no longer used. The enum only has one member now, so not sure if it's needed anymore.

[qwandor]

This is no longer true. start_recv doesn't have its safety conditions documented, but I suspect it requires the packet reference passed in to live at least until the receive completes. You don't enforce that here, so recv_non_blocking is unsound. You either need to make it unsafe (and document that the caller must ensure the packet lives at least until the receive operation completes), or add some safe abstraction to ensure that it does, such as having the Radio handle allocation of the packet.

[eivindbergem]

Good catch! I've added a mutable reference to the packet in Recv to enforce this.

[qwandor]

Unfortunately that's still not sound. The caller can call forget on the returned Recv and then drop the Packet that they passed a reference to, while the hardware still tries to write to it.

[eivindbergem]

If we require a pinned pointer for Packet, we get a drop guarantee.

[eivindbergem]

No, that won't help as we can still forget Recv and then drop the Packet. It's the Recv that needs to be dropped, not Packet. I guess unsafe is the best option then.

[eivindbergem]

Tried something new. recv_non_blocking takes a closure that takes &Recv as argument.

[qwandor]

Interesting approach, it's not a pattern I've seen much before for something like this but I think it's sound.

[BartMassey]

If it's a soundness issue we might want to get a second review before we merge. I can try, but I'm bad at that stuff: might want to tag somebody?