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

add covariance fields to imu message #333

Merged
merged 4 commits into from
Aug 2, 2023
Merged

add covariance fields to imu message #333

merged 4 commits into from
Aug 2, 2023

Conversation

ejalaa12
Copy link
Contributor

🎉 New feature

Add the covariance fields to the imu message

Summary

The imu message is missing the covariance fields for linear_acceleration, angular_velocity and orientation_covariance.

Those can be very useful when bridging between gz and ros.

Note: a PR is coming to gz-sensors to fill those field.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • 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.

@ejalaa12 ejalaa12 requested a review from caguero as a code owner March 15, 2023 19:38
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Mar 15, 2023
@ejalaa12 ejalaa12 mentioned this pull request Mar 15, 2023
Merged
9 tasks
mjcarroll
mjcarroll reviewed Mar 15, 2023
View reviewed changes
proto/gz/msgs/imu.proto Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 15, 2023 •
edited
Loading

Codecov Report

Merging #333 (7e2dea6) into main (d58a6ce) will not change coverage.
The diff coverage is n/a.

❗ Current head 7e2dea6 differs from pull request most recent head ad212aa. Consider uploading reports for the commit ad212aa to get more accurate results

@@           Coverage Diff           @@
##             main     #333   +/-   ##
=======================================
  Coverage   95.71%   95.71%           
=======================================
  Files           9        9           
  Lines        1073     1073           
=======================================
  Hits         1027     1027           
  Misses         46       46

@ejalaa12 ejalaa12 mentioned this pull request Mar 15, 2023
Merged
9 tasks
@iche033
Copy link
Contributor

iche033 commented Mar 15, 2023

I think this breaks ABI so will need to go into main?

@ejalaa12
Copy link
Contributor Author

@mjcarroll I just pushed the changes to re-order and document the new fields to stick to ros conventions

@ejalaa12
Copy link
Contributor Author

I think this breaks ABI so will need to go into main?

@iche033 so are you proposing to change the branch into which we merge this PR ?

@mjcarroll
Copy link
Contributor

are you proposing to change the branch into which we merge this PR ?

Yes, please. Our policy is to not break ABI in an existing release. We are working on a mechanism for message changes to not break ABI, but it is not in version 9. Please target main/gz-msgs10

@mjcarroll
Copy link
Contributor

Other than branch target, changes LGTM.

@mjcarroll mjcarroll changed the base branch from gz-msgs9 to main April 12, 2023 12:51
@ejalaa12
Copy link
Contributor Author

@mjcarroll Thanks for the answer. I just resolved the merge conflicts and updated the PR with it.

@ejalaa12
Copy link
Contributor Author

@mjcarroll How do you plan on releasing this? Since it's related to PRs on other repos ?

@mjcarroll
Copy link
Contributor

How do you plan on releasing this? Since it's related to PRs on other repos ?

We will need to get each of the other ones landed first and released, and then this one can get in.

ahcorde
ahcorde requested changes May 8, 2023
View reviewed changes
Copy link
Contributor

@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.

conflicts

@ejalaa12
Copy link
Contributor Author

@ahcorde fixed the conflicts

ahcorde
ahcorde requested changes May 11, 2023
View reviewed changes
Copy link
Contributor

@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.

CI is failing https://github.com/gazebosim/gz-msgs/actions/runs/4950760191/jobs/8859092077?pr=333

tools/gz_msgs_generate.py Outdated Show resolved Hide resolved
tools/gz_msgs_generate.py Outdated Show resolved Hide resolved
@ejalaa12
Copy link
Contributor Author

@ahcorde @mjcarroll The confligs mainly came from the various merge from main into this branch.
To fix everything and limit this PR to only my changes, i've rebased my changes on main and force-pushed.
Hope this clears things up !

@ejalaa12
Copy link
Contributor Author

@ahcorde @mjcarroll Hey guys, any update on this PR ? shall I rebase again ?

@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023
@mjcarroll mjcarroll enabled auto-merge (squash) August 1, 2023 19:51
@mjcarroll mjcarroll disabled auto-merge August 2, 2023 16:25
@mjcarroll mjcarroll merged commit 241b06e into gazebosim:main Aug 2, 2023
3 checks passed
@ejalaa12
Copy link
Contributor Author

ejalaa12 commented Aug 3, 2023

Thank you for the PR merge! :)

ejalaa12 added a commit to ejalaa12/gz-msgs that referenced this pull request Aug 4, 2023
@ejalaa12 @mjcarroll
add covariance fields to imu message (gazebosim#333)
673875e 
* add covariance fields to imu message

Signed-off-by: Alaa El Jawad <[email protected]>

* re-order msg fields and doc to stick to ros convention

Signed-off-by: Alaa El Jawad <[email protected]>

---------

Signed-off-by: Alaa El Jawad <[email protected]>
Co-authored-by: Michael Carroll <[email protected]>
ejalaa12 added a commit to ejalaa12/gz-msgs that referenced this pull request Sep 25, 2023
@ejalaa12 @mjcarroll
add covariance fields to imu message (gazebosim#333)
a2216ab 
* add covariance fields to imu message

Signed-off-by: Alaa El Jawad <[email protected]>

* re-order msg fields and doc to stick to ros convention

Signed-off-by: Alaa El Jawad <[email protected]>

---------

Signed-off-by: Alaa El Jawad <[email protected]>
Co-authored-by: Michael Carroll <[email protected]>
ejalaa12 added a commit to ejalaa12/gz-msgs that referenced this pull request Dec 4, 2023
@ejalaa12 @mjcarroll
add covariance fields to imu message (gazebosim#333)
0c408c4 
* add covariance fields to imu message

Signed-off-by: Alaa El Jawad <[email protected]>

* re-order msg fields and doc to stick to ros convention

Signed-off-by: Alaa El Jawad <[email protected]>

---------

Signed-off-by: Alaa El Jawad <[email protected]>
Co-authored-by: Michael Carroll <[email protected]>
@DanielBar98
Copy link

Are there plans to implement or release this feature for older versions, including Fortress?

@azeey
Copy link
Contributor

azeey commented Nov 4, 2024

Unfortunately, this is not a backward compatible change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahcorde ahcorde ahcorde requested changes

@mjcarroll mjcarroll mjcarroll approved these changes

@caguero caguero Awaiting requested review from caguero caguero is a code owner

Assignees

@ejalaa12 ejalaa12

Labels
beta Targeting beta release of upcoming collection 🌱 garden Ignition Garden
Projects
Core development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ejalaa12 @iche033 @mjcarroll @DanielBar98 @azeey @ahcorde