Feature Request: non-blocking recv()

[mavlink]

Feature Request: non-blocking recv()

[wucke13]

@tstellanova ping
Also please consider implementing the new async/await feature, as it is even nicer than non blocking recv()

[tstellanova]

@wucke13 feel free to submit a pull request

[wucke13]

I have looked into this a bit more. AFAIK, there are two ways of accomplishing this:

• ugly: spawning a new thread, blocking recv() in that thread, push to MPSC, use MPSC try_recv() to get async behavior. That's what I'm currently doing as a hot fix.
• correct: Adding state to read_versioned_msg(). Detect if buffer is not big enough to parse the message, and return std::io::ErrorKindWouldBlock in that case. Preserve the so far consumed bytes, as they might be part of a valid message. That way it becomes trivial to implement the actual try_recv(). One could orient at tokio's codec trait, for the implementation.

The correct way of doing this involves re-implementing parts of the MAVLink parser. Any chances that you will look into this? Can you confirm my understanding of the possible solutions, or is there a neat way that I'm missing out on?

[tstellanova]

Sounds like you’re making progress on a design. Agreed the second approach seems more beneficial than the first. I’m unlikely to have time to add features in the next few months, and admittedly I have not kept up with the latest async features in rust, so may not be the best person to advise on that. Last I heard, @podhrmic was working on a significant refactor, but I don’t know where that’s at.

…

On Dec 26, 2019, at 10:35 AM, wucke13 ***@***.***> wrote: I have looked into this a bit more. AFAIK, there are two ways of accomplishing this: ugly: spawning a new thread, blocking recv() in that thread, push to MPSC, use MPSC try_recv() to get async behavior. That's what I'm currently doing as a hot fix. correct: Adding state to read_versioned_msg(). Detect if buffer is not big enough to parse the message, and return std::io::ErrorKindWouldBlock in that case. Preserve the so far consumed bytes, as they might be part of a valid message. That way it becomes trivial to implement the actual try_recv(). One could orient at tokio's codec trait, for the implementation. The correct way of doing this involves re-implementing parts of the MAVLink parser. Any chances that you will look into this? Can you confirm my understanding of the possible solutions, or is there a neat way that I'm missing out on? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[podhrmic]

Hi @wucke13 !
I thought about this library as "bare minimum" - i.e. it only provides struct definitions and a couple of convenience functions (socket and serial read for example). What you are mentioning would benefit from using rust futures for example.

That being sad, feel free to submit a PR with your solution, just make sure it is properly feature gated with #[cfg(feature = "...")] (we want the library to be usable even in no-std environments)

@tstellanova I got kind of stuck typechecking enums and implementing a set of sensible quickcheck tests, then I ran out of time. So it is kind of half finished at the moment:-/

[wucke13]

I would oppose to using futures for this. From recent experiment I took away that futures are only beneficial when one build complex dependencies on them. For a simple "give it to me, if and only if its ready" they are not a good fit. I would therefore still prefer the try_recv style interface.

Also, I think try_recv should be considered part of bare minimum. I mean, even the Arduino serial interface offers a an aswer to "How much can I receive". So IMHO even in an absolute bare minimum setup, there should be an non blocking way of asking for new messages.

I'm still not convinced that my understanding is sufficient to make a PR for this, we will see 😄

[patrickelectric]

Just to update this issue, I'm working to add such feature, probably a PR should be ready in a couple of days, the ideas is to implement something like try_recv as @wucke13 commented, and maybe in the future having something closer to mpsc.

[wucke13]

@patrickelectric That is very good news! However, please note: Since I wrote my comment, futures came a long way! Especially with smol the futures thing became much more accessible, at least to me. I think the correct way of doing making the mavlink crate future proof is this:

• Offering recv() for legacy compatibilitie, and because it is sufficient sometimes
• Offering try_recv() for simple async-ish stuff. This is important especially for no_std, as it may not be so simple to spawn a real async executor there.
• Offering async recv(). This is the most useful way of interacting with a message stub, IMO.

Internally, recv() could just await the aysn recv for the sake of simplicity. I don't see any reason so far, why this wouldn't work. That way mavlink.rs can offer a rich and modern API without too much code duplication or other sacrifices.

What do you think about this?