Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support cbor-raw encoding for messages #34

Open
ssnover opened this issue Sep 3, 2022 · 3 comments
Open

Support cbor-raw encoding for messages #34

ssnover opened this issue Sep 3, 2022 · 3 comments
Labels
enhancement New feature or request

Comments

@ssnover
Copy link
Collaborator

ssnover commented Sep 3, 2022

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 ssnover self-assigned this Sep 3, 2022
@ssnover ssnover mentioned this issue Sep 3, 2022
5 tasks
@ssnover
Copy link
Collaborator Author

ssnover commented Sep 4, 2022

@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
Copy link
Collaborator Author

ssnover commented Sep 4, 2022

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
Copy link
Collaborator

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.

@Carter12s Carter12s added the enhancement New feature or request label Sep 11, 2022
@ssnover ssnover removed their assignment Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants