-
Notifications
You must be signed in to change notification settings - Fork 772
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
Add gazebo_ros::QoS class #1091
Conversation
Whenever a plugin creates a ROS publisher or subscription, use the QoS profile provided by the node for the given topic. This enables users to override the QoS settings in SDF. Depends on ros-simulation#1091. Signed-off-by: Jacob Perron <[email protected]>
Here is a follow-up PR that makes use of the new |
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.
Great work @jacobperron !!
I've left some minor comments
I don't think the current implementation will work with remapped topic names. I'm going to look into it and add some tests. |
Great point. IMO, remapping should be done before querying the qos profile.
For that, I think we need to apply remapping rules/topic name expansion in P.S.: Shouldn't |
I agree with your assessment. I'll take a look at implementing it now.
Yes, when the SDF is parsed here in gazebo_ros_pkgs/gazebo_ros/src/node.cpp Line 105 in 728bb0e
The I'll update the example in this PR description. For unit testing |
@ivanpauno Here's my attempt at supporting remapping: 9e753f5 I don't believe there's a way to get the topic remappings from |
@jacobperron By mistake I did my comments in commit 9e753f5, if you open it you can see the comments in their context. It would be nice to add a test were a namespace is specified in the |
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.
LGTM!!
std::string node_namespace_; | ||
|
||
/// The options of the node associated with the QoS settings | ||
rclcpp::NodeOptions node_options_; |
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.
@chapulina I'm not sure if we're using the PIMPL pattern in gazebo_ros_pkgs
yet, but I'm wondering if this new class is a good candidate for it, since it has lots of private member variables
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.
Thank you for catching this, @scpeters . Indeed we're using PIMPL for the ROS 2 version of gazebo_ros_pkgs
, as described on the Contribution guide.
I've updated the test to include a node namespace: 19469da |
@jacobperron @scpeters is this ready to go in? |
@ivanpauno I think the point about considering to use a pimpl pattern is something we should consider (#1091 (comment)). |
Contains logic for parsing <qos> SDF elements and creating rclcpp::QoS objects for ROS publishers and subscriptions. Signed-off-by: Jacob Perron <[email protected]>
Add a getter method to gazebo_ros::Node returning a gazebo_ros::QoS. Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
We should pass the node name, node namespace, and node options containing the remap arguments so that we can remap topic names passed to get_*_qos() properly. Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
88e5d48
to
5bcfe93
Compare
I rebased on |
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@scpeters @jacobperron I finally figured out the remaining build problems before closing. |
The latest changes LGTM 👍 |
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.
@@ -38,7 +38,7 @@ Node::SharedPtr Node::Get(sdf::ElementPtr sdf) | |||
{ | |||
// Initialize arguments | |||
std::string name = ""; | |||
std::string ns = ""; | |||
std::string ns = "/"; |
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.
Is this change needed?
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.
Valid namespaces should be always absolute (contrary to names, which can be relative/private/absolute).
I guess @jacobperron did that change because the namespace is being passed to the new QoS class, and some of the API it uses may be doing namespace validation.
@jacobperron can you confirm if that was the case?
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.
Got it. Since we started going down this road on this PR, do you think we should also do some checking on line 58 in case the user passes an invalid namespace? Or maybe we prepend the /
for them in case it's missing?
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.
@ivanpauno is exactly right. Without this change, I was seeing runtime errors from some rcl
/ rclcpp
API calls (though I can't recall exactly which ones).
I guess if a user passes an invalid namespace, they will see similar runtime errors. I think the validation and error message from the underlying layers is enough (it also is nice to keep it encapsulated there in case the naming requirement changes for some reason).
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.
Got it, makes sense. I'm just wondering why we haven't seen problems so far with this default value, and whether this change is backwards compatible. But you peeps know best about these details 😉
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 think this may have messed something up 🤔 #1116
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.
Doesn't seem related to me.
That default value is being immediately overridden by the value in the SDF if it exists
gazebo_ros_pkgs/gazebo_ros/src/node.cpp
Lines 57 to 59 in 57732fe
if (sdf->HasElement("namespace")) { | |
ns = sdf->GetElement("namespace")->Get<std::string>(); | |
} |
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.
Maybe, it's related to this
gazebo_ros_pkgs/gazebo_ros/src/gazebo_ros_factory.cpp
Lines 231 to 235 in 57732fe
// Walk recursively through the entire file, and add <ros><namespace> tags to all plugins | |
auto robot_namespace = req->robot_namespace; | |
if (!robot_namespace.empty()) { | |
AddNamespace(entity_elem, robot_namespace); | |
} |
gazebo_ros_pkgs/gazebo_ros/src/gazebo_ros_factory.cpp
Lines 354 to 355 in 57732fe
// Set namespace | |
ns_elem->Set<std::string>(_robot_namespace); |
?
According to grep that's the only place where we set the namespace
SDF tag.
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.
Nice catch! @ahcorde , are you spawning your robot through the factory? In my case I was.
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.
gazebo_ros/test/test_qos.cpp
Outdated
"<sdf version='1.6'>" | ||
" <world name='default'>" | ||
" <plugin name='node_1' filename='libnode_name.so'>" | ||
" <qos>" |
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 know both <plugin><qos>
and <plugin><ros><qos>
are supported, but I guess the latter is the recommended one (based on documentation). How about using that on most tests? Just so people looking have a good example to follow.
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.
Done in 7a5188a
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
I've addressed/replied to the commets.
I would prefer to do that in a follow up if you don't mind. |
@chapulina is this ready to go in? |
I haven't tried it out but looking at the code I don't see anything weird. Could you peeps just run CI before merging? Instructions here: https://github.com/ros-simulation/gazebo_ros_pkgs/wiki/ROS-2-CI#next-turtle |
Thanks for the instructions! I didn't know how to run CI here. CI: |
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.
Thanks for triggering CI. I see all QoS tests passed. I don't think the flake8 failures are this PR's fault.
All other tests failing with:
[Err] [Master.cc:95] EXCEPTION: Unable to start server[bind: Address already in use]. There is probably another Gazebo process running.
are being caused by some earlier test not being properly terminated and leaving Gazebo running. I think it's because of test_entity_spawner.test.py
, see the testing adventure here: #1033 (comment). Again, not this PR's fault.
I saw the flake8 failures locally, and they are also reproducible in
I experienced that locally too 😂. @chapulina I don't have write access here, please merge if it's ready. |
I can rebase the connected PR. |
Whenever a plugin creates a ROS publisher or subscription, use the QoS profile provided by the node for the given topic. This enables users to override the QoS settings in SDF. Depends on ros-simulation#1091. Signed-off-by: Jacob Perron <[email protected]>
Yeah I think this could be built into the test cases themselves to kill any running |
* Make QoS for publishers and subscriptions configurable Whenever a plugin creates a ROS publisher or subscription, use the QoS profile provided by the node for the given topic. This enables users to override the QoS settings in SDF. Depends on #1091. Signed-off-by: Jacob Perron <[email protected]> Signed-off-by: Jacob Perron <[email protected]>
@jacobperron |
this can just be added in a normal gazebo sdf world file, the new qos parameters can be applied to gazebo_ros_pkgs plugins as shown in the first post #1091 (comment) |
Connects to #1079
This PR adds a new class that parses SDF for ROS 2 QoS settings that plugins can use when creating publishers and subscriptions.
Here's an example of valid SDF (from one of the tests):
The new
<qos>
tag can contain zero or more<topic>
tags. Each topic tag must have aname
attribute.The implementation will look for optional
<publisher>
and<subscription>
tags, which can contain any number of tags corresponding to all of the QoS settings supported by ROS 2.A description of QoS settings can be found here:
One limitation of the current approach is that all subscriptions (or publishers) for a given topic will use the same QoS overrides. I don't think there's a way to differentiate them in ROS 2, but I also don't think this will be an issue for any of the plugins in this repository.
The choice to use milliseconds for all duration values is arbitrary. If there is a more preferable unit, it's easy to change.