Allow async radio implementations

[lulf]

With async-await rust gaining more usage in embedded, and mechanisms for async traits being feature complete on nightly, it's not too long before stable rust will support it. There are now async SPI implementations, which in turn will allow async radio drivers. The advantages of that would be to use less power awaiting radio irqs, polling for busy signals (on some radios) without explicitly handling those peripheral-specific states at the "link layer".

I'd like to sketch up a proposal for an AsyncPhyRxTx trait, supported in nightly rust configured using a Cargo feature. The code that calls into the radio would need to be async as well, so I was thinking to introduce a new API/types for that, but keep the internals mostly shared apart from the radio interaction.

Wdyt @ivajloip @lthiery ?

[lthiery]

That sounds really great! A lot of the current device driver design was a result of me trying to wrap in Rust the vendor radio drivers while leaving seams for pure Rust drivers. I think that, with async-await gaining traction in embedded, a new radio API focused on enabling async-await usage would be fantastic.

I'd like to sketch up a proposal for an AsyncPhyRxTx trait, supported in nightly rust configured using a Cargo feature. The code that calls into the radio would need to be async as well, so I was thinking to introduce a new API/types for that, but keep the internals mostly shared apart from the radio interaction.

I have vague notions on how this could work but I definitely haven't had a hands-on work with the embedded sync stuff so I'm very excited to see what you come up with.

[ivajloip]

That sounds indeed very interesting! I would love to see this and if you need any discussion before diving in a complete implementation, let me (and I believe Louis) know.

[lulf]

Glad to hear the openness to the idea! I have experimented a bit, and got a working async radio. I've needed to make some changes:

• Reduce stack usage of owned data in Session. This is fixed in Use mut ref device ref to save stack space #48

Changes I plan to implement next/not yet finished:

• Create async version of handle_event from Device, through Session/NoSession and for all the relevant. This is necessary in order to allow async-await inside the states. The non-async logic that doesn't call the radio can be moved into helper methods that can be called by both implementations.

• Add an 'async' feature gate that will enable the async features at compile time. This will enable the async versions of the handle_event methods, as well as define a PhyRxTx trait that allows radio implementations to use async as well.

An alternative approach would be to define a completely new Device and logic for async, but I think it would be good to not having to maintain the logic in two places as much as possible. Moreover, I think reusing parts of the state machine for async is probably fine, especially for TX which already allows for awaiting a TX to finish. So I imagine that some of the states can be skipped in an async version.

[lulf]

An alternative approach would be to define a completely new Device and logic for async, but I think it would be good to not having to maintain the logic in two places as much as possible. Moreover, I think reusing parts of the state machine for async is probably fine, especially for TX which already allows for awaiting a TX to finish. So I imagine that some of the states can be skipped in an async version.

Having done some experiments now, I'm leaning towards this option: Create a separate implementation that is async native. The reason is that a lot of the radio trait can be simplified with async, as one can have the radio behave in a 'blocking' way instead of going through the different event states.

It would be great to keep the interface similar though, and I intend to keep the current Device impl as is, and then introduce the new one under an 'async_device' module instead. We can then review that independently and decide where it belongs.

[ivajloip]

Hey, thanks for the additional details. I am not sure if I fully grasp the suggestion. I might have misunderstood, but my understanding is that there will be a new definition of basically device/src/radio. If that is the case, what would be the effect on the state_machines and the the device implementation itself. One of the ways I read the suggestion is to remove/avoid all reliance on state machines for the device.

[lulf]

@ivajloip My last comment is basically what I've done in #49 . The existing device does not change at all. Instead, a new module structure with whatever is needed for an 'async device' is made. The reason is simple: async-await in rust ends up generating the state machine, therefore having the implementation take advantage of async-await, you end up with a simpler implementation instead. If you look at the code, it is about 1/3 the size of the state-machine based device.

The only code that is shared between the 'state machine' and 'async' implementation is device/src/region and some of the types in device/src/types.rs . The 'Device' type is completely different, because for async-await the 'handle_event' API doesn't make sense anymore. As you point out, there is also a new radio trait definition, because the underlying radio trait has different requirements (being async 'blocking').

I think both of these implementations can co-exists. If not, I'm happy to maintain the 'async device' implementation elsewhere too, but I think there is value in sharing the current set of types, and maybe they can eventually merge if async-await becomes more natively supported in rust stable.

In the above PR you can use the 'async device' implementation by importing lorawan_device::async_device::* and compiling lorawan-device with the 'async' feature flag.

[lthiery]

I would certainly agree with the rewrite of the state-machine using async-await syntax. I think we can immediately see how much more digestible the logic is when you don't have to parse the explicit (and verbose) SM definition.

However, I do notice a bit of code duplication. For example, the original function here is not far from this PR's.

It might be too cumbersome to create shareable functions every time. But in that particular case, I imagine implementing create_join_request on the Credentials struct might work, for example.

[lulf]

I agree, we should should try to find these places where we can refactor so that the implementations can share more code. I will have a look at the create_join_request. Maybe also prepare_buffer can benefit of a similar refactoring, if we put that on SessionData impl and have it take the parameters needed for region, buffer, random.

[ivajloip]

I believe I understand now. Thank you for the explanation! It sounds good to me. 🙂 And yes, we should try not to duplicate code if that is easily avoidable, but balance that with the readability, which could suffer from trying to remove duplication at all cost.

[lulf]

It's been there for a while now :)