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

Conform to ros format for header field frame_id of sensor msgs #195

Merged
merged 6 commits into from
Mar 1, 2022

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Feb 18, 2022

🎉 New feature

Summary

While I was giving support for the turtlebot3 in Ignition , I found that the frame_id is generated with this format <model_name>::<link_name>::<sensor_bane>, then when we use the bridge to republish the data in the ROS network, the frame_id is modified here and it will look like <model_name>/<link_name>/<sensor_bane>, but this format is still not valid, it not matching with any frame_id in tf.

This PR overwrite the frame_id value and set it to another value which allows to make it compatible with ROS.

I don't know where is the right place to document this.

Test it

Following the steps in the other PR you can test this.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde self-assigned this Feb 18, 2022
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Feb 18, 2022
@codecov
Copy link

codecov bot commented Feb 18, 2022

Codecov Report

Merging #195 (b2ad054) into ign-sensors3 (5d24d67) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff                @@
##           ign-sensors3     #195      +/-   ##
================================================
+ Coverage         77.48%   77.57%   +0.09%     
================================================
  Files                23       23              
  Lines              2367     2377      +10     
================================================
+ Hits               1834     1844      +10     
  Misses              533      533              
Impacted Files Coverage Δ
src/AirPressureSensor.cc 86.30% <100.00%> (ø)
src/AltimeterSensor.cc 88.50% <100.00%> (ø)
src/CameraSensor.cc 75.37% <100.00%> (ø)
src/DepthCameraSensor.cc 74.48% <100.00%> (ø)
src/ImuSensor.cc 90.38% <100.00%> (ø)
src/Lidar.cc 73.15% <100.00%> (ø)
src/LogicalCameraSensor.cc 90.14% <100.00%> (ø)
src/MagnetometerSensor.cc 88.63% <100.00%> (ø)
src/RgbdCameraSensor.cc 75.21% <100.00%> (ø)
src/Sensor.cc 89.78% <100.00%> (+0.80%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d24d67...b2ad054. Read the comment docs.

@ahcorde ahcorde changed the base branch from ign-sensors6 to ign-sensors3 February 18, 2022 15:41
src/Lidar.cc Outdated Show resolved Hide resolved
Signed-off-by: ahcorde <ahcorde@gmail.com>
…ame_id

Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde changed the title Added frame_id parameter to lidar sensor Conform to ros format for header field frame_id of sensor msgs Feb 28, 2022
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from iche033 March 1, 2022 09:45
Copy link
Contributor

@iche033 iche033 left a 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. Not sure what's wrong with the sensors windows CI build. I just retriggered a new build

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

CI is green now

@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-25-fortress-edifice-citadel/1343/1

@doisyg
Copy link
Contributor

doisyg commented Mar 26, 2022

Is it possible to trigger a release of libignition-sensors6 to benefit from this PR in Fortress bin?

@chapulina
Copy link
Contributor

@doisyg , see #207

@doisyg
Copy link
Contributor

doisyg commented Apr 1, 2022

Hello,
Testing this PR since it is now merged and released. It works for laserscan msgs but not for pointclouds.
I believe the type ignition.msgs.PointCloudPacked is missing the frame field.

@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1

@doisyg
Copy link
Contributor

doisyg commented Apr 17, 2022

This PR is not working for GpuLidarSensor published as ignition.msgs.PointCloudPacked
#217 and fixed here: #218

@ashBabu
Copy link

ashBabu commented Sep 14, 2023

Saw this now only. I tried <optical_frame_id>, <ignition_frame_id>, <ign_frame_id> etc but nothing worked for me in gazebo fortress, ros2 humble. #384. ignition sensors6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants