-
Notifications
You must be signed in to change notification settings - Fork 211
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
Introduce QoS property #409
Conversation
I think this would be nice to have, but I'm not sure the best way to detect incompatible publishers. I think one way would involve comparing the number of matched publishers to the actual number of publishers for a given topic. We can get the number of matched publishers with SubscriptionBase::get_publisher_count(), and number of publishers on a topic with NodeGraph::count_publishers(). I think it we be okay to leave this as an enhancement "TODO" for the first pass.
Consider that lifespan and liveliness are not addressed the relevant design document I would say we can ignore them for now.
I think the unknown options are more for error handling and we probably shouldn't include those as options in RViz.
👍 Makes sense to me.
Seems reasonable. For example, I could imagine someone wanting the initial pose tool to have durability set to transient local. |
1bf3aa2
to
9235106
Compare
Sounds good to me.
Map display and Camera (those are the two displays with an additional subscription) also use the qos profile in their second subscription now. However, I did not yet add another profile property. The reason is that both displays only have one topic property and the second subscription is derived "magically" from the first (e.g. the map update topic is the map topic + "_update"). Therefore, there is no good place to have a separate qos property. Does it make sense to have different QoS settings for the camera and the camera info topic? What about the map and map update topic?
Publishing tools now have their own qos_profile. I tried to use the Apart from this, I'm nearly done - one test is failing that I have to fix. |
9235106
to
f3db9bf
Compare
Test is fixed - the I'm done for now, we only need to decide what to do with map and camera, otherwise this is good to review. |
I agree it makes sense to have separate QoS settings for the map and map_update topics, and therefore an additional topic property. I guess it makes sense for the camera and camera_info topics as well. Looking at the image_transport code, it looks like they use the same QoS settings, but I think this is more of a convention. I'm not aware of any REP or standard stating otherwise. I'm fine leaving this as a follow-up enhancement, but I'll leave it up to you. |
ee02d28
to
528e719
Compare
@jacobperron I'm done. I tested it locally and it all seemed to work. Regarding the camera and map display, here is what I did:
It's not difficult to change the behaviour to allow the user to change the update topic, too, but there are a few questions and fine points to this (Should we keep the old behaviour, i.e. automatically subscribe to the update topic? In code, the functions are a bit intertwined, one has to be careful to not produce an infinite update loop), so I think we should wait until somebody really needs this. When this gets merged, we should also open a ticket to track future improvements for user feedback regarding subscriptions to existing topics which have different QoS properties. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, mostly just minor comments.
This will also need a rebase before running CI.
|
||
private Q_SLOTS: | ||
void updateQueueSize(); | ||
void updateQosProfile(); | ||
|
||
private: | ||
rviz_common::properties::IntProperty * queue_size_property_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The term depth is used in the context of DDS. What do you think about renaming instances of "queue" to "depth"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used "depth" instead of "queue_size" as "depth_size" seemed redundant. Hopefully caught everything.
rviz_common/src/rviz_common/properties/qos_profile_property.cpp
Outdated
Show resolved
Hide resolved
rviz_common/include/rviz_common/properties/qos_profile_property.hpp
Outdated
Show resolved
Hide resolved
rviz_common/src/rviz_common/properties/qos_profile_property.cpp
Outdated
Show resolved
Hide resolved
rviz_common/src/rviz_common/properties/qos_profile_property.cpp
Outdated
Show resolved
Hide resolved
rviz_common/src/rviz_common/properties/qos_profile_property.cpp
Outdated
Show resolved
Hide resolved
rviz_default_plugins/src/rviz_default_plugins/displays/map/map_display.cpp
Outdated
Show resolved
Hide resolved
rviz_default_plugins/src/rviz_default_plugins/displays/map/map_display.cpp
Show resolved
Hide resolved
- not a "true" property, but a container of properties - will call a callback when changed, similar to queue_size_property before - this will allow the property to be put below each ros topic property
- Will be superseded by QOS property
will be superseded by qos profile property
- below ros topic property to not needlessly clutter the panel
The fluent API is nice, but for the property itself, it seems easier to just use the rmw type. The alternative is a lot of if-statements which is not easier to read
925eff4
to
f3c91ec
Compare
f3c91ec
to
a060815
Compare
I added the suggestions and rebased it. Ready for the next round. |
There a Windows warning and looks like the change may have introduced some test failures on Linux and macOS |
Deleting the queue size property changes the position of all properties. Tests which rely on the absolute position need to be changed, too.
The tests had a simple problem: Removing the queue size property (which was always below the topic property and thus the second property) changes the position of all other properties by one. There are a few tests which actually rely on the position of the property - those are the ones that failed. So hopefully, that should fix all warnings and test failures (I ran the failing tests locally on Linux and they are fine now). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix #360
Idea:
unreliable
property as well as thequeue_size
property.Not in this PR:
rclcpp::QoS
. I did not do this change to simplify reviewing and because at least for the property, I think it is much easier to work with the rmw profile as it simplifies working with all options.Feedback needed for:
volatile
but I settransient local
, then no message gets received. Should we and how can we provide user feedback?rmw_qos_profile_t
are necessary. In particular, I'm unsure whether we need theUnknown
options.