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.

Sign up for GitHub

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

Jump to bottom

Adds pose and twist with covariance messages bridging #222

Merged
merged 18 commits into from
Apr 27, 2022
Merged

Adds pose and twist with covariance messages bridging #222

merged 18 commits into from
Apr 27, 2022

Conversation

adityapande-1995
Copy link
Contributor

@adityapande-1995 adityapande-1995 commented Mar 8, 2022 •
edited
Loading

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

🎉 New feature

Summary

This PR aims to interface the following message types :

  • ignition::msgs::PoseWithCovariance <--> geometry_msgs/msg/PoseWithCovariance
  • ignition::msgs::TwistWithCovariance <--> geometry_msgs/msg/TwistWithCovariance
  • ignition::msgs::OdometryWithCovariance <--> nav_msgs/msg/Odometry

This is a follow up PR to the message types added here : gazebosim/gz-msgs#224

Test it

TODO

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.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

@adityapande-1995 adityapande-1995 changed the base branch from melodic to ros2 March 8, 2022 00:11
@adityapande-1995
Populate cov matrix
57681a3 
Signed-off-by: Aditya <[email protected]>
@adityapande-1995 adityapande-1995 marked this pull request as ready for review March 8, 2022 06:17
@adityapande-1995 adityapande-1995 mentioned this pull request Mar 9, 2022
Merged
8 tasks
@chapulina chapulina added needs upstream release Blocked by a release of an upstream library ROS 2 ROS 2 labels Mar 9, 2022
@chapulina
Copy link
Contributor

Please don't forget to update the README with the new supported conversions:

https://github.com/ignitionrobotics/ros_ign/tree/ros2/ros_ign_bridge

@adityapande-1995 adityapande-1995 mentioned this pull request Mar 16, 2022
Merged
8 tasks
jacobperron
jacobperron approved these changes Mar 17, 2022
View reviewed changes
Copy link
Contributor

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM pending CI.

It's a bit unfortunate that the Ignition covariance values are not constrained to be exactly 36 values.

ros_ign_bridge/src/convert/geometry_msgs.cpp
convert_ign_to_ros(ign_msg.pose().position(), ros_msg.pose.position);
convert_ign_to_ros(ign_msg.pose().orientation(), ros_msg.pose.orientation);
int data_size = ign_msg.covariance().data_size();
if (data_size == 36) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How likely is this going to be false? Seems less than ideal.

Copy link
Contributor Author

@adityapande-1995 adityapande-1995 Mar 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, this is not likely to be false, but I was still worried if this loop will segfault at runtime if the data array in ros_msg.covariance[ ] exceeds index. There is no explicit check on Float_V size.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to define the type such that it always has a size of 36?

Copy link
Contributor Author

@adityapande-1995 adityapande-1995 Mar 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe those are not supported by protobuf, used by ign-msgs : https://www.aapelivuorinen.com/blog/2019/07/12/protobuf-arrays/

Copy link
Member

Choose a reason for hiding this comment

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

yeah I don't think protobuf supports fixed size arrays

we are on the verge of releasing the ign-msgs file into ign-msgs8. Does this approach seem reasonable to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to keep this PR as-is. I was just curious if it was possible to avoid.

@adityapande-1995 adityapande-1995 mentioned this pull request Apr 20, 2022
Open
@chapulina chapulina removed the needs upstream release Blocked by a release of an upstream library label Apr 22, 2022
@chapulina
Copy link
Contributor

CI is finally running for this branch. There are new linter errors:

 	  5 - cpplint (Failed)
	  6 - flake8 (Failed)
	  9 - uncrustify (Failed)

@adityapande-1995
Linter fixes
8154303 
Signed-off-by: Aditya <[email protected]>
@adityapande-1995
Linter fixes
b6e3dab 
Signed-off-by: Aditya <[email protected]>
@adityapande-1995
Liner fixes
3e33a26 
Signed-off-by: Aditya <[email protected]>
@adityapande-1995
Fixed test failures
80130d2 
Signed-off-by: Aditya <[email protected]>
@adityapande-1995
Flake8 fixes
557a680 
Signed-off-by: Aditya <[email protected]>
@adityapande-1995
Copy link
Contributor Author

adityapande-1995 commented Apr 27, 2022 •
edited
Loading

There is a problem with the post shutdown tests, which seems unrelated to this PR. Tested locally with the ros2 branch and it threw the same error. For instance, this error is also seen here : https://build.ros2.org/job/Rpr__ros_ign__ubuntu_jammy_amd64/36/

jacobperron
jacobperron approved these changes Apr 27, 2022
View reviewed changes
Copy link
Contributor

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

Latest changes LGTM

@adityapande-1995 adityapande-1995 merged commit 54c28b1 into ros2 Apr 27, 2022
@adityapande-1995 adityapande-1995 deleted the aditya/odom_with_covariance branch April 27, 2022 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobperron jacobperron jacobperron approved these changes

@scpeters scpeters Awaiting requested review from scpeters

@chapulina chapulina Awaiting requested review from chapulina

Assignees
No one assigned
Labels
ROS 2 ROS 2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@adityapande-1995 @chapulina @jacobperron @scpeters