Adding QoS to Publisher

[yoelbassin]

Working over wi-fi can be challenging due to communication over a lossy or unstable network. ROS QoS is a great mechanism to handle these issues, with it's reliability, durability and lifespan option it can solve and help in many ways. An example is a topic with a reliable publisher with volatile durability. Another option is using BEST_EFFORT reliability to lower traffic throughput.

A base for this approach was already merged In #86, but it still lacks many features (the attributes I mentioned above). A possible solution is adding durability, reliability and lifespan attributes to the query, such that the new JSON will be

{ "op": "publish",
  (optional) "id": <string>,
  "topic": <string>,
  (optional) "reliability": <string>,
  (optional) "durability": <string>,
  (optional) "lifespan": <string>,
  "msg": <json>,
}

or, a way better approach will be to send the entire QoS as an inner json in the message:

{ "op": "publish",
  (optional) "id": <string>,
  "topic": <string>,
  (optional) "qos": <json>,
  "msg": <json>,
}

Where the qos JSON is

{
  (optional) "reliability": <string>,
  (optional) "durability": <string>,
  (optional) "lifespan": <string>,
  ...
}

[danaiger]

Seem like a good idea. It caught me as well when I had to create a publisher with custom life span message. Anyone has an idea if this is already implemented somehow?

[rejero]

Following this, im running into issues where i need published messages sent via rosbrodge to have the same custom qos settings as my subscribers on ros2 nodes. Might need the same for my subscribers receiving via rosbridge too

[yoelbassin]

@sea-bass what do you think about these options? I want to start working on a PR for this issue but it will change the API and would require changes in multiple repos as @JayHerpin mentioned in #908

The first solution (of just adding some options to the JSON) is fine and wouldn't create breaking changes, but it would require changing the API once again if more QoS options would be added.

The second option of adding a QoS sub-JSON would just add a single parameter which could easily be extended later, but since there is already an option for queue_size (merged in #86) it would either require a breaking change, or a duplicate parameter that can be marked as deprecated.

Would love to hear opinion about this issue and the proposed options

[sea-bass]

I would as much as possible keep the QoS configuration at the time of creating the publisher/subscriber itself, and not every time you send a single message payload. This mimics how ROS does it anyhow.

So if the configuration can be done at the advertise / subscribe level, not with publish, that would be ideal. And then each client library can tap into that as needed.

Also, I don't think the protocol needs to change all that much. Maybe just one qos field which itself is a JSON dictionary? It could have the individual fields like reliability/durability, but maybe also have an option to specify a preset as well, like SENSOR_DATA or SYSTEM_DEFAULT.

Long as we have good unit tests here for parsing that qos payload, we should be alright!

[sea-bass]

Also FWIW replacing that single queue size parameter with something more generic seems like a welcome breaking change.

People can always use older versions of this repo as needed.

[JayHerpin]

I would as much as possible keep the QoS configuration at the time of creating the publisher/subscriber itself, and not every time you send a single message payload. This mimics how ROS does it anyhow.

So if the configuration can be done at the advertise / subscribe level, not with publish, that would be ideal. And then each client library can tap into that as needed.

I agree with @sea-bass on that it really can't be per publish as many of those QoS properties are immutable

The second option of adding a QoS sub-JSON would just add a single parameter which could easily be extended later, but since there is already an option for queue_size (merged in #86) it would either require a breaking change, or a duplicate parameter that can be marked as deprecated.

Deprecating the queue_size seems best, but even if you did keep it, its not totally off base. I am most familiar with the rclcpp API, but at least in that client library, the history depth is sorta special in that they provide a convenience constructor where an integer is implicitly converted to a system default QoS with that queue_size/history_depth. So having both works and is consistent with other client APIs. We'd just want to make it clear that if the QoS struct contains a history_depth it overrides the the queue_size if it was also set in that same message.

Also, @yoelbassin Can I ask which client library you are primarily working with?

[JayHerpin]

Am I correct in my understanding that the queue_size is currently pulling double duty? It seems to be used for both an internal publisher queue, and is passed through to be used for the history depth? (new the codebase, exclude me if that is not correct).

However, if that is right, I think it probably makes sense to keep both and decouple them.

[yoelbassin]

Also, @yoelbassin Can I ask which client library you are primarily working with?

I use rclpy which works the same for the queue_size/history_depth.
Using the same API in the rosbridge protocol is probably best, although personally I don't like using the same argument for multiple purposes. But that's the ros2 client libs api's so it's probably best to follow this method 🤷

About the second usage of queue_size, it is used in the MessageHandle but theoretically it should be the same as the parameter in the given qos if we do add this argument, so instead of taking this value immediately we may just take it from the new qos JSON