rust-iot / radio-hal

Embedded rust radio abstraction crate
https://ryan.kurte.nz/notes/2020-01-05-rust-radio
MIT License
69 stars 13 forks source link

Timeout while receiving #20

Closed Tortoaster closed 2 years ago

Tortoaster commented 2 years ago

This is an issue I encountered when implementing LoRaWAN device behavior. When a class A device transmits a message, it should first wait 1 second and then try to receive a message on the RX1 channel. If it doesn't receive anything, it should wait until 2 seconds after transmission and try to receive on RX2. The problem is that receiving a message can take longer than a full second.

The current implementation of do_receive returns when it either completely received the message, or when a timeout occurs. Setting the timeout to 1 second will also discard any message that is currently being received (unless the implementation of check_receive blocks the thread until it can return true, but that is undesirable, as it is also used for the nonblocking traits), so the timeout must be longer than 1 second, but then the radio will be too late to switch to RX2 if it doesn't receive anything on RX1. Therefore, I think the radio traits should be able to determine whether or not a radio is busy receiving an actual message, even before having received it completely.

One simple solution could be to add a oneshot_receive method to BlockingReceive, which is identical to do_receive but with self.check_receive(false) instead of self.check_receive(true), but I'm not sure if that's the best option. Would love to know what others think about this!

ryankurte commented 2 years ago

hey thanks for the issue, interesting problem!

i don't think the restart flag in check_receive does what you're expecting. the intent was to signal that the driver should re-enter RX on failure (underrun / crc failure / internal radio timeout etc.), but, it's not always super easy / clear to implement so i have been pondering removing it.

Therefore, I think the radio traits should be able to determine whether or not a radio is busy receiving an actual message, even before having received it completely.

sgtm, specifically i think check_receive should be able to signal both busy and completion, which could then be used in blocking and nonblocking wrappers.

at the moment no guarantee is provided about returning the same state multiple times (ie. check_receive will return true only once), but for the busy state we would need to change this. we also often use an interrupt pin to avoid polling for state via SPI in check_receive as this can negatively impact receive sensitivity so to do this we'll likely need to retain an internal state in the radio object, which is no big.

as an aside, if you do need to do timing specific things, you probably are better to use the base traits rather than the blocking / non-blocking ones for more granular control.

any opinions @henrikssn?

henrikssn commented 2 years ago

First, for the record, +1 to removing the restart arg of check_receive.

Therefore, I think the radio traits should be able to determine whether or not a radio is busy receiving an actual message, even before having received it completely.

The question is if we can assume that all radios support this. In particular, the interrupt driven radios which only IRQ's on packet ready won't. Therefore, I don't think it is a good idea to extend the Receive trait with this additional requirement. I think the beauty of the current Receive trait is that it only cares about receiving a packet, nothing more and nothing less (barring the restart part, but you already know my opinion on that).

Perhaps introducing a new trait RadioBusy (name just an example) holding the busy() -> bool method would be a reasonable way forward. Then you could poll that method in addition to check_receive and correctly handle the case when the radio turns busy just before timeout.

Tortoaster commented 2 years ago

Both of those options sound good to me! On one hand, radios that don't know whether or not they are busy could just always return Pending, although I can see why a redundant extra case can be annoying. On the other hand, it makes sense that radios that can do this implement a new trait. I would be happy to create a PR for either option.

ryankurte commented 2 years ago

Perhaps introducing a new trait RadioBusy (name just an example) holding the busy() -> bool method would be a reasonable way forward. Then you could poll that method in addition to check_receive and correctly handle the case when the radio turns busy just before timeout.

ahh, there's actually already a Busy trait i have been using for this purpose with an .is_busy() method that i had forgotten 🤣

i had also considered adding another function to the RadioState bound for this as well, but the separate trait exists because to check state you're going to have to hit the radio over SPI whereas to tell if it's busy you can often implement via GPIO and avoid the performance impact, while wrapping state or whatever if not.

aside... really need to fix the docs.rs generation so we can see / share these easily eh.