Discussion: Improving the API

[wucke13]

Hi, I would like to start a discussion about improving the MAVLink API. I frequently come across situations which are (at least with my knowledge) not easy to solve using this library.

Subscribing to a specific message

Often I want to receive a certain message somewhere in the code. This is a key feature in order to not have this one huge match statement where each and every message is processed. For this, it would be very nice to be able to subscribe to a message. I imagine an API like this:

impl MavConnectionHandler {
  subscribe(&self, message: MavMessage)->std::sync::mpsc::Receiver<MavMessage>
  subscribe_once(&self, message: MavMessage)->std::sync::mpsc::Receiver<MavMessage>
  // call frequently to do the housework
  spin(&self);
}

Obviously this would require that someone is calling spin regularly. Another less no_std friendly approach would be

impl MavConnectionHandler {
   /// spawns the MavConnectionHandler which takes care of handling subscriptions
  new<M>(conn: Box<dyn MavConnection<M> + Sync + Send>)->(JoinHandle<()>, Arc<dyn MavConnectionHandler>)
  subscribe(&self, message: MavMessage)->std::sync::mpsc::Receiver<MavMessage>
  subscribe_once(&self, message: MavMessage)->std::sync::mpsc::Receiver<MavMessage>
}

This however bears a big problem. AFAIK there is no way to talk about the type of a mavlink message without creating an instance of it. This brings us to my next pain point:

Allow a message type to be specified, without an Instance of the message

Well there is the message id, but that is by no means type safe (and IMO nothing we should rely on in an API). And yes, there is std::mem::discriminant, however that does require one to construct a value. If I want to name the type of MavMessage, the best option so far seems to be this:

let message_type = discriminant(&MavMessage::PARAM_VALUE(Default::default()));

Allow conversion of a MavMessage to one specific ..._DATA struct

I'm currently in the making of a nicer async API which operates on top of the MavConnection trait. AFAIK it is not possible to convert a MavMessage into on specific data structure. This is not an issue if one doesn't know what message the next recv() will yield. However, in my implementation I do know that. Consider the following example (which continues the last snippet):

/// currently. The thing which bothers me the most is that there is 
/// no way of avoiding yet another level of indentation.
if let MavMessage::PARAM_VALUE(data) = wrappe_conn.request(&message_type).await {
    
}

/// maybe in the future? ^^
/// Notice that the request method can do the unwrapping internally, 
/// as we already know that we will only receive the correct messages
let data = wrapped_conn.request(&message_type).await;

If I understand correctly, the issue is simple: There is no way of extracting data out of an enum instance without mentioning the variant which contains said data. Maybe its possible to add a method like this to MavMessage:

/// Tries to convert an instance of the `MavMessage` enum into one of the `_DATA` structs
impl MavMessage {
    into_inner(self)->Option<T>
}

Maybe it's also possible to exploit Any for the polymorphism needed in a generic receive function. A rough draft could be like this: For sure runtime reflections do work for this kind of stuff, but that would be a huge break in the API which will likely not pay off UX wise.

pub enum MavMessageType {
    HEARTBEAT,
    SYS_STATUS,
    SYSTEM_TIME,
    //...
}

impl MavMessage {
   type(&self)->MavMessageType;
   as_message<T>(&self)->T{
        self.downcast_ref::<T>().clone()
   }
   // serialize, deserialize, ...
}

Summary

I'm sure there is more room for improvement, but these are the points which have bitten me the most. My proposals are in no way ready and probably will even fail compilation, they solely are meant to be early drafts. Speaking of drafts, out of the need for my use cases I already implemented an async API wrapper for MavConnection in the last couple of days. I would be very happy if it eventually find's it's way back in the this crate, behind a feature gate which is disabled per default maybe? Currently you can only have a look into it here. It isn't feature complete, but it already works!
What do you think?

Is there a general desire to further refine the API?

Edit:
Some of my statements turned out to be questionable, thus I corrected them. Furthermore I added some more information and examples.
Edit 2:
I added some more information and again refine the examples.

[wucke13]

The interest in discussing this seems to be on a moderate level at best. I released some of the proposed changes in an independent crate on crates.io. I would still be happy to have this discussion though, as there are quite some big compromises on my crate due to some limitation of the mavlink library.

[patrickelectric]

Hi @wucke13, sorry for not being able to help much about this matter, but I was planning to create an actor based communication method over mspc, that appears the correct way to accomplish it.
I started the implementation but since I'm out of time I was unable to finish it.

Don't you want to create a PR to add the async functionality in this repository ?

[wucke13]

Yeah, I'm still happy to merge my code. However, now that I released to an independent crate, I would like to use the opportunity to refine things more. I wan to add some convenience functions (like send message a, until the first message b arrives) for dealing with unreliable communications.

My strongest desire which AFAIK can not be solved outside of this crate would be an implementation of

TryFrom<::Message> for ::PLACEHOLDER_DATA. So a generic way of converting any message into a specific message struct without explicit matching. Can you add this?

[patrickelectric]

TryFrom<::Message> for ::PLACEHOLDER_DATA. So a generic way of converting any message into a specific message struct without explicit matching. Can you add this?

From what I know this is not possible directly and a generic tryfrom will need to be implemented, and I don't see a reason for this.
What is necessary IMO is to recreate the parser and deal with the parsing to be async, that's what I'm doing with my patch plus the async communication with mspc.
You can check that in my open draft PR here: #79

[wucke13]

I don't see a reason for this

Well, here is one: This is necessary to offer a sane API where the user receives filtered messages (e.g. only one message type). If I and a stream both now that said stream will only yield PARAM_VALUE messages, than it would be very nice for the stream to be able to directly return PARAM_VALUE_DATA structs. Currently, there is no way to offer such an API, for the reasons stated in my initial posting. Does this make sense to you, or shall I try to explain in more detail why I think that there is a strong reason for that?

From what I know this is not possible directly

Why is that? It's not possible in a generic way, because there is (IMHO major) problem in the TryFrom implementation from the std library (more details). However, with the information about all variants (which only this library has in it's build script) available at the code level, it is:

I thought one way would be this:

• to every enum MavMessage add a function in the impl which
  • matches against every possible variant
  • unpacks the contained data
  • casts a reference counted reference of it to Any
  • and returns this any
• Add this as_any function to the Message trait.
• Than on the Message trait, add a try_extract<T>(&self)->Option<T> function.

Do you see a problem with this approach, implementation wise? I'm sure there is more elegant ways of doing it, but I did not think of any so far.

Also, whats the point with channels when going the async route? I don't understand why you would be doing both.

At least the discussion started now 😄

[wucke13]

@patrickelectric Ping

Also: if this helps, I'm open to hop on a virtual meeting to discuss this issue further. However that's just an idea.

[patrickelectric]

Hi @wucke13, sorry I'm in a complicate period in the last days, will try to catch-up with this discussion when possible.

[wucke13]

@patrickelectric As stated before, I'm open to any means of digital communication, at your choice. Until then: take your time and good luck!

[onur-ozkan]

Topics discussed in this issue are extremely outdated and things been changed significantly since then. I think we can close this to not cause any confusion for other people.