Skip to content
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

Replaced numbers with SystemDefaultsQos() #2271

Merged
merged 4 commits into from
Jul 31, 2023
Merged

Replaced numbers with SystemDefaultsQos() #2271

merged 4 commits into from
Jul 31, 2023

Conversation

Shobuj-Paul
Copy link
Contributor

Description

This PR replaces "magic numbers" with SystemDefaultsQos(), making it more readable and possibly more stable.
Fixes #2225

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@codecov
Copy link

codecov bot commented Jul 23, 2023

Codecov Report

Patch coverage: 35.72% and project coverage change: +0.01% 🎉

Comparison is base (830ceda) 50.50% compared to head (3d96f76) 50.50%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2271      +/-   ##
==========================================
+ Coverage   50.50%   50.50%   +0.01%     
==========================================
  Files         386      386              
  Lines       31732    31736       +4     
==========================================
+ Hits        16023    16026       +3     
- Misses      15709    15710       +1     
Files Changed Coverage Δ
...g/rdf_loader/src/synchronized_string_parameter.cpp 32.31% <0.00%> (-0.50%) ⬇️
...it_ros/robot_interaction/src/robot_interaction.cpp 0.00% <0.00%> (ø)
...obot_state_rviz_plugin/src/robot_state_display.cpp 0.00% <0.00%> (ø)
...ugin_render_tools/src/trajectory_visualization.cpp 0.00% <0.00%> (ø)
...nning_scene_monitor/src/planning_scene_monitor.cpp 45.44% <66.67%> (+0.18%) ⬆️
...or/src/current_state_monitor_middleware_handle.cpp 88.89% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sjahr
Copy link
Contributor

sjahr commented Jul 24, 2023

Thanks for starting this @Shobuj-Paul. Ping me once the PR is ready for review!

Copy link
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good so far

@Shobuj-Paul
Copy link
Contributor Author

Hi @sjahr
Can I leave instances such as this untouched? This seems to be more explanatory than just a number.

@sjahr
Copy link
Contributor

sjahr commented Jul 25, 2023

Hi @sjahr Can I leave instances such as this untouched? This seems to be more explanatory than just a number.

Good question 🤔 Can you add a comment to explain this choice (if you know it)? Otherwise just add a TODO(): ? comment to mark it and we'll figure it out in the review of this PR. @tylerjw Do you know this?

@Shobuj-Paul
Copy link
Contributor Author

Hi @sjahr Can I leave instances such as this untouched? This seems to be more explanatory than just a number.

Good question thinking Can you add a comment to explain this choice (if you know it)? Otherwise just add a TODO(): ? comment to mark it and we'll figure it out in the review of this PR. @tylerjw Do you know this?

Understood, I shall comment on all such instances.

@Shobuj-Paul Shobuj-Paul marked this pull request as ready for review July 25, 2023 19:24
@Shobuj-Paul
Copy link
Contributor Author

@sjahr
This PR is ready for review. Couple of things left to be confirmed during review:

  • Documenting a reason for giving this kind of QoS profile.
  • For python nodes, I couldn't find any equivalent of SystemDefaultsQoS(), so if there a better way of increasing readability of instances like these?

Copy link
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good so far 👍 Thank you! I'll approve it once we've got an answer regarding rclcpp::QoS(1).transient_local().reliable()

@sjahr
Copy link
Contributor

sjahr commented Jul 25, 2023

I think you can explicitly set it via https://docs.ros2.org/foxy/api/rclpy/api/qos.html but if you just don't set it like in the tutorial, it seems to default to the system default. Can you check if that works? @peterdavidfagan Do you have any advice here?

@Shobuj-Paul
Copy link
Contributor Author

I think you can explicitly set it via https://docs.ros2.org/foxy/api/rclpy/api/qos.html but if you just don't set it like in the tutorial, it seems to default to the system default. Can you check if that works? @peterdavidfagan Do you have any advice here?

Yep, this looks good. I'll leave the python nodes as they are for now, since this kind of depth setting seems to be documented.

@Shobuj-Paul Shobuj-Paul requested a review from sjahr July 28, 2023 14:40
Copy link
Member

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good cleanup. Thank you!

@henningkayser henningkayser added the backport-humble Mergify label that triggers a PR backport to Humble label Jul 31, 2023
@henningkayser henningkayser merged commit 5506dd5 into moveit:main Jul 31, 2023
@Shobuj-Paul Shobuj-Paul deleted the remove_magic_numbers branch July 31, 2023 14:05
mergify bot pushed a commit that referenced this pull request Jul 31, 2023
henningkayser pushed a commit that referenced this pull request Aug 2, 2023
(cherry picked from commit 5506dd5)

Co-authored-by: Shobuj Paul <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble Mergify label that triggers a PR backport to Humble
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use rclcpp::SystemDefaultsQoS() instead of magic numbers (or document magic numbers)
4 participants