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

Refactor air pressure demo #632

Merged
merged 19 commits into from
Dec 13, 2024
Merged

Refactor air pressure demo #632

merged 19 commits into from
Dec 13, 2024

Conversation

caguero
Copy link
Contributor

@caguero caguero commented Nov 4, 2024

Refactors the air_pressure demo leveraging the latest improvements in ros_gz. If we're happy with it, I'll propagate the refactor to all the demos.

  • I replaced the previous launch file from Python to XML because I think it's the recommendation (and looks easier) but I'll still keep some of the demos in Python to offer some variety.
  • I added the QoS support as well. I noticed that because of the QoS, rqt_topic isn't able to show any data. This was probably happening before. Does anybody know if we can pass QoS settings to rqt_topic?

Summary

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
@caguero caguero requested a review from ahcorde as a code owner November 4, 2024 17:55
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
@caguero caguero marked this pull request as draft November 6, 2024 17:11
@caguero caguero changed the title Air pressure demo refactor Refactor air pressure demo Nov 6, 2024
@caguero caguero marked this pull request as ready for review November 22, 2024 20:40
Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

I can see these errors:

[gz-3] Warning [Utils.cc:132] [/sdf/model[@name="force_torque_demo"]/link[@name="base_plate"]/static:<data-string>:L3]: XML Element[static], child of element[link], not defined in SDF. Copying[static] as children of [link].
[gz-3] Warning [Utils.cc:132] [/sdf/model[@name="force_torque_demo"]/link[@name="base_plate"]/static:<data-string>:L3]: XML Element[static], child of element[link], not defined in SDF. Copying[static] as children of [link].

rqt_topic is not able to show the value, maybe a bad topic configuration ? the readme shows how to read the value with cmd commands, maybe remove rqt_topic from the launch file ?

@caguero
Copy link
Contributor Author

caguero commented Nov 25, 2024

I can see these errors:

[gz-3] Warning [Utils.cc:132] [/sdf/model[@name="force_torque_demo"]/link[@name="base_plate"]/static:<data-string>:L3]: XML Element[static], child of element[link], not defined in SDF. Copying[static] as children of [link].
[gz-3] Warning [Utils.cc:132] [/sdf/model[@name="force_torque_demo"]/link[@name="base_plate"]/static:<data-string>:L3]: XML Element[static], child of element[link], not defined in SDF. Copying[static] as children of [link].

rqt_topic is not able to show the value, maybe a bad topic configuration ? the readme shows how to read the value with cmd commands, maybe remove rqt_topic from the launch file ?

I think this is the issue that I mentioned in the description. Because of the QoS seetings of the publisher, rqt cannot show it. Is it possible to pass QoS settings to the rqt subscriber?

@caguero
Copy link
Contributor Author

caguero commented Nov 27, 2024

I can see these errors:

[gz-3] Warning [Utils.cc:132] [/sdf/model[@name="force_torque_demo"]/link[@name="base_plate"]/static:<data-string>:L3]: XML Element[static], child of element[link], not defined in SDF. Copying[static] as children of [link].
[gz-3] Warning [Utils.cc:132] [/sdf/model[@name="force_torque_demo"]/link[@name="base_plate"]/static:<data-string>:L3]: XML Element[static], child of element[link], not defined in SDF. Copying[static] as children of [link].

These errors look unrelated to ros_gz. I do see them if I launch gz sim -v 4 sensors.sdf.

@ahcorde
Copy link
Collaborator

ahcorde commented Nov 29, 2024

I created this draft PR ros-visualization/rqt_topic#51 to try to override the QoS in rqt_topic, but for now it's not working.

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

Do you mind to try these last 3 suggestions ?

@caguero
Copy link
Contributor Author

caguero commented Dec 6, 2024

Do you mind to try these last 3 suggestions ?

I tried ros-visualization/rqt_topic#51 in combination with your three suggestions:

  1. Bridge the clock topic.
  2. Pass QoS settings and use_sim_time to rqt_topic.
  3. Pass use_sim_time to the ros_gz bridge.

I'm getting the following issue:

[component_container-1] [ERROR] [1733502964.485608076] [ros_gz_container]: Component constructor threw an exception: parameter 'use_sim_time' has invalid type: Wrong parameter type, parameter {use_sim_time} is of type {bool}, setting it to {string} is not allowed.

It looks like there's an issue passing 'use_sim_time': True to the bridge. I tried using 'True' as well as a string but getting the same error.

I also tried just passing the QoS settings to rqt_topic and the bridge without setting use_sime_time. In fact, I removed the /clock bridge from the yaml file as well and things seem to work.

Screenshot from 2024-12-06 17-43-13

Independently of the bridge problem, why do we need to pass use_sim_time?

Co-authored-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

I had to use upper case for 'true' otherwise the code was not working, is this happening to you ?

the clock bridge is required by rqt_topic

Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

It's working without the clock bridge, I don't know what I was doing wrong.

You just need to use the new changes from @azeey. Otherwise LGTM

Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
@caguero
Copy link
Contributor Author

caguero commented Dec 11, 2024

It's working without the clock bridge, I don't know what I was doing wrong.

You just need to use the new changes from @azeey. Otherwise LGTM

Thanks, good to go again!

@caguero
Copy link
Contributor Author

caguero commented Dec 13, 2024

@ahcorde , are you OK with the changes now?

@caguero caguero merged commit 13b6640 into ros2 Dec 13, 2024
5 checks passed
@caguero caguero deleted the caguero/air_pressure_demo_v2 branch December 13, 2024 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants