-
Notifications
You must be signed in to change notification settings - Fork 14
added preamble deadline for proper LoRaWAN RX1+RX2 reception #33
added preamble deadline for proper LoRaWAN RX1+RX2 reception #33
Conversation
Used by lora-rs/lora-rs#142 |
We have to wait for ceekdee to come back after October 23 for review. I do not understand this code enough to make a decision. |
Basically it allows you to run the IRQ loop just until the preamble (or header) is received. Allows LoRaWAN RX1/RX2 windows to be properly implemented. As per spec the receiver should continue receiving the reply using RX1 settings if it's got the preamble in time, even if the RX1 window is already closed. |
605f062
to
78307d7
Compare
78307d7
to
1f95715
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look like they keep the existing mode to me, I have one comment on the tx path regarding the unreachable state.
Err(err) => { | ||
self.radio_kind.ensure_ready(self.radio_mode).await?; | ||
self.radio_kind.set_standby().await?; | ||
self.radio_mode = RadioMode::Standby; | ||
Err(err) | ||
} | ||
Ok(_) => unreachable!(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this is actually unreachable. What if you start an RX before, drop the future, and then try to TX? Couldn't the irq trigger with TargetIrqState::Preamble in that case? Feels safer if this was somewhat ignored or returned an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
process_irq will only return TargetIrqState::PreambleReceived
if explicitly asked to. In this case it's asked to only return TargetIrqState::Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking again I see what you mean. Looks good to me then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about changing the API to be a little bit more type-stateful. That'd resolve some issues like device being left in an inconsistent state after cancellation of a future. Also, it'd remove confustion between tx interrupts and rx interrupts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have a proper chat about thr overall layout of this crate. If you have the time you could join us at #public-lora-wan-rs:matrix.org
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm these changes works on my lora-discovery board (although requiring some custom configuration for joining to work in embassy, but it is likely unrelated to this PR since it is required on the main branch too).
thank you @ilya-epifanov! |
No description provided.