Skip to content

QoS Matching#52

Open
MoffKalast wants to merge 4 commits into
v-kiniv:mainfrom
MoffKalast:main
Open

QoS Matching#52
MoffKalast wants to merge 4 commits into
v-kiniv:mainfrom
MoffKalast:main

Conversation

@MoffKalast
Copy link
Copy Markdown
Contributor

Hey, it's me again 😄

I've noticed recently that there are some more esoteric topics that get rejected by RWS completely due to incompatible QoS, mainly since it always subscribes with Reliable even to topics that are Best_effort. This change should match all params exactly instead of just durability.

The second thing is that often times I'll have a bunch of subscribers declared in the client ahead of time, and then only later on those publishers get set up and start sending. In this case we don't subscribe at all, and then one has to retry manually from the client.

So instead we can do what rosbridge does, which is subscribe to the given topic & type with the most compatible QoS, so Best_effort + Volatile. That will not receive old transient local messages since that needs to be set explicitly, but given the alternative I think this is still the far more practical solution.

It seems to work on my end, but I'll be testing a bit more in the coming days to see if I find any edge cases.

MoffKalast and others added 4 commits May 19, 2026 21:23
- choose subscription QoS compatible with all known publishers
- preserve client-provided history depth
- avoid copying liveliness from only the first publisher
- normalize explicit subscription type names
- reject explicit types incompatible with an existing topic
- update broken QoS-dependent tests
- add tests for new subscription behavior
@v-kiniv
Copy link
Copy Markdown
Owner

v-kiniv commented May 20, 2026

You again?!

gerara-here-man-gerara

Overall good PR, but there are a few missing things:

  • broken tests that relies on default QoS
  • missing tests for the new functionality
  • no check if topic exists but user provided incompatible type in json
  • the code is using QoS from the first topic only: if it is more strict than other topics in the array, RWS will create subscription that is too strict for the rest of the publishers

And a few things worth implementing:

  • use QoS that would be compatible with all publishers on the topic
  • drop qos.history(rmw_qos.history) because it will discard history_depth passed by client
  • drop qos.liveliness(rmw_qos.liveliness) because we either let it be default(as is it was before PR) or properly set it based on all publishers not just the first one
  • use rws::message_type_to_ros2_style to convert type name in json(as it is done in advertise_topic)

I implemented what I though was missing and added tests. But here's the thing: my ROS setup is degrading with each year passing since I have not been using ROS for a while, and now I don't even have my linux VMs, let alone my native Mac setup, I used to test RWS with my project simulation. And I don't feel like setting up all that stuff. So I've set up devcontainer+VSCode. I'm relying here on the unit tests, so I hope you can properly test it)

@v-kiniv
Copy link
Copy Markdown
Owner

v-kiniv commented May 20, 2026

Looks I pushed my commits to wrong place? I cannot see them here.
upd: fixed

@MoffKalast
Copy link
Copy Markdown
Contributor Author

MoffKalast commented May 22, 2026

🦟 Oh I gotta drop in at least once a year. They say everyone has some kind of ghost that chases them around. Usually it's more metaphorical though lmao. Also Lyrical for 26.04 just got released three hours ago...


Haha yeah, forgot about the tests. Things work so that's good enough right? 😝

I've tested the new changes out a bunch on a two Jazzy platforms with Cyclone (FastDDS is oddly bugged on Jazzy in general), and in Gazebo and it seems to check out. Cherry picking this to Humble also compiles and runs, though I haven't got anything notable to test with there anymore either.

no check if topic exists but user provided incompatible type in json

Hmm that can happen yeah. I guess that's the only case where we really shouldn't subscribe so it doesn't mess up the DDS side and we'll get a message that likely breaks something somewhere in the client pipeline anyway.

the code is using QoS from the first topic only: if it is more strict than other topics in the array, RWS will create subscription that is too strict for the rest of the publishers

It might actually make the most sense to plan for what's most compatible if any additional publishers join after we already subscribe too.

Are there any downsides if we just check if there's any transient locals, and if so, use best effort + transient local, otherwise best effort + volatile? I think all other combinations should then be compatible. Those two cases will typically always get separated too (at least if /tf and /tf_static are to go by), since it doesn't semantically make any sense to send a single latched UDP packet when it's entirely possible it gets lost.

The two major goofs are:

  • subscribe as reliable, breaks best effort publishers
  • subscribe as transient local, breaks volatile publishers

I'm not entirely sure what happens if you combine best effort + transient local in various RMWs, though in theory if the publisher is reliable it should still be fully reliable and not matter.

The current setup is probably fine for 99.9% of cases though anyway.

@MoffKalast MoffKalast marked this pull request as ready for review May 22, 2026 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants