Extend actions with parameter and response #4
Conversation
ClemensElflein
left a comment
ClemensElflein
left a comment
There was a problem hiding this comment.
Thank you for the changes @rovo89
See review comments for changes (I'm not sure if you got the ping, since I opened the PR)
| std::string version_string = ""; | ||
|
|
||
| // separator for action_id, request_id and payload/response of actions | ||
| const char ACTION_SEP = '|'; |
There was a problem hiding this comment.
why not use json on the wire instead of custom format?
There was a problem hiding this comment.
I considered that, and if you want, I can change it to JSON. My thoughts were:
- Don't want to break existing apps, which is achieved by making the request_id and payload optional. Therefore a simple
mower_logic/reset_emergencystill works. Requiring the action to be a JSON like{"action": "mower_logic/reset_emergency", "request_id": "123-456", "payload": [1, 2, 3]}would break apps. We could check whether the first character is a{though, and if not convert it to{"action": "<message>"}, so that's no blocker. - We can't pass the parsed JSON object via ROS messages. That means we have two options:
- Pass the MQTT payload string unmodified to the callers. Publishing type would be string as before, but the downside is that every client has to decode the JSON on their own, including error handling. Also, I like in my approach that there are three fields with a clear purpose, rather than the generic string field. That would make it easier (from my point of view) to request an action within ROS itself.
- Decode the JSON in xbot_monitoring and fill the three fields in the ROS message with it. Problem: What if the payload is a JSON structure itself, as in the example above? Then xbot_monitoring would have to dump that sub-structure into a string again which gets filled into the payload field in the ROS message. Or the client would have to pass their payload as string only. Both are quite ugly.
On the other hand, I don't expect any ambiguities with my proposal, as neither action_id nor request_id will have the | character. It's easy to concatenate and split these strings. And JSON decoding only needs to be done if really required.
| } else if(ptr->get_topic() == this->mqtt_topic_prefix + "/action") { | ||
| // BEGIN: Deprecated code (2/2) | ||
| ROS_WARN_STREAM("Got action on deprecated topic! Change your topic names!: " + ptr->get_payload()); | ||
| std_msgs::String action_msg; | ||
| action_msg.data = ptr->get_payload_str(); | ||
| action_pub.publish(action_msg); | ||
| action_pub.publish(parse_action_request(ptr->get_payload_str())); | ||
| // END: Deprecated code (2/2) |
There was a problem hiding this comment.
As soon as we change the wire format old apps will break anyways, so we can drop the deprecated code here, WDYT @rovo89
There was a problem hiding this comment.
My intention was to stay compatible with the old format, and then I don't see a necessity to break clients (yet).
|
@ClemensElflein This should be closed in favor of ClemensElflein/open_mower_ros#225 (and in general, the repo should be archived). |
2 participants
No description provided.