search

Carter12s / roslibrust

An async first rust client for ROS1 native and rosbridge.
https://docs.rs/roslibrust
MIT License
48 stars 6 forks source link

Support cbor-raw encoding for messages #34

Open ssnover opened 2 years ago

ssnover commented 2 years ago

Currently roslibrust assumes and only supports default json-encoding.

It looks in the two affected places are Client::handle_publish where messages are decoded (and there's an unwrap assuming the encoding is json) and Writer::publish which assumes the msg is a RosMessageType. It probably makes sense to construct the publisher with an encoding mechanism defined in order to prevent changing the publish/subscribe APIs.

The format of these messages is not extremely well documented in rosbridge_suite's protocol document, nor is it even implemented in roslibpy, but from what I can tell it should produce messages like:

"msg": {
    "bytes": <bytearray>
}

We can make use of serde_rosmsg which implements the correct encoding mechanism, as despite the name it has no relation with cbor other than that their both in binary...

ssnover commented 2 years ago

@Carter12s would like to get your take on this since you've written the trait for RosBridgeComm. Currently the API doesn't give a means of expressing the desired encoding:

async fn publish<T: RosMessageType>(&mut self, topic: &str, msg: T) -> RosLibRustResult<()>;

The RosBridgeComm trait is intended as a generic interface to a ROS bridge server, not a generic ROS IPC layer (ROS bridge, TCPROS for ROS1, whatever IPC for ROS2). I think it makes sense to expand the APIs to be encoding aware with an enum of supported transport methods. I think this crate should also define a different trait which is intended to be middleware agnostic (though that's of course completely tangential to this issue).

ssnover commented 2 years ago

Ah I see that I misunderstood. cbor-raw is only allowed for subscribers, not for publishers. Publishers seem to be required to use JSON encoding, as there is no compression field for advertise, only for subscribe.

Carter12s commented 2 years ago

Yeah I think this should be as easy as subscribe() taking an argument about compression and then swapping in a different decode closure into the internal callback queue.

There is a bigger conversation around how to more broadly restructure the crate to support completely different comm backends if/when we want to do something like support ROS1 or ROS2 native protocols, but I don't want to think about that yet. I reality I think we should punt on cbor-raw support for now, and focus more on documentation and tests.