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

Add DopplerVelocityLog sensor #290

Merged
merged 13 commits into from
Dec 7, 2022
Merged

Add DopplerVelocityLog sensor #290

merged 13 commits into from
Dec 7, 2022

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Nov 21, 2022

🎉 New feature

Summary

This patch adds a DVL sensor, implemented as a custom rendering sensor. Needs gazebosim/gz-msgs#317.

Test it

TBD

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.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic added the 🌱 garden Ignition Garden label Nov 21, 2022
@hidmic hidmic requested a review from iche033 as a code owner November 21, 2022 18:59
@osrf-triage osrf-triage added this to Inbox in Core development Nov 21, 2022
@hidmic hidmic requested a review from caguero November 21, 2022 19:00
@hidmic
Copy link
Contributor Author

hidmic commented Nov 21, 2022

First iteration. Once we've agreed on gazebosim/gz-msgs#317, I'll add integration tests here.

@azeey azeey moved this from Inbox to In review in Core development Nov 21, 2022
Copy link

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

Just did a quick first pass. I wonder if we should simply point Environment.hh in gz-sim7 to here. Perhaps also move the utility function. Alternatively, you could just continue using DVLs as a custom sensor and not open a PR here.

include/gz/sensors/EnvironmentalData.hh Outdated Show resolved Hide resolved
include/gz/sensors/EnvironmentalData.hh Show resolved Hide resolved
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@arjo129
Copy link

arjo129 commented Nov 23, 2022

CI and codecheck are still not happy. Once they are happy this looks good to me.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Nov 23, 2022

CI and codecheck are still not happy. Once they are happy this looks good to me.

Done with codecheck, but for CI we'll need to merge and release gazebosim/gz-msgs#317 first.

@hidmic hidmic mentioned this pull request Nov 23, 2022
38 tasks
@caguero caguero mentioned this pull request Nov 23, 2022
7 tasks
@mjcarroll mjcarroll added the needs upstream release Blocked by a release of an upstream library label Nov 28, 2022
@mjcarroll
Copy link
Contributor

@hidmic Go ahead and bump the find_package call to require gz-msgs 9.2.0

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic requested a review from arjo129 December 5, 2022 14:46
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@codecov
Copy link

codecov bot commented Dec 5, 2022

Codecov Report

Merging #290 (8301678) into gz-sensors7 (ea7d33a) will increase coverage by 2.46%.
The diff coverage is 81.79%.

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

@@               Coverage Diff               @@
##           gz-sensors7     #290      +/-   ##
===============================================
+ Coverage        69.19%   71.66%   +2.46%     
===============================================
  Files               35       37       +2     
  Lines             3753     4665     +912     
===============================================
+ Hits              2597     3343     +746     
- Misses            1156     1322     +166     
Impacted Files Coverage Δ
src/DopplerVelocityLog.cc 81.65% <81.65%> (ø)
src/EnvironmentalData.cc 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@hidmic
Copy link
Contributor Author

hidmic commented Dec 5, 2022

https://github.com/gazebosim/gz-sensors/actions/runs/3623412682/jobs/6109243959 is failing on all integration tests for rendering sensors, incl. the DVL. Any ideas @mjcarroll @caguero ?

@j-rivero
Copy link
Contributor

j-rivero commented Dec 5, 2022

ign_sensors-pr-win Pending — deploying to build.osrfoundation.org

I've resurrected this one.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Dec 6, 2022

Alright, I understand rendering sensors tests are expected to fail on Jammy and Windows. CC @iche033 @mjcarroll just in case.

@arjo129 @caguero this is ready for a final review.

Copy link
Contributor

@adityapande-1995 adityapande-1995 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, just that INTEGRATION_dvl is segfaulting on jammy, but so are the camera tests.

@mjcarroll
Copy link
Contributor

Looks good to me, just that INTEGRATION_dvl is segfaulting on jammy, but so are the camera tests.

Michel and I spoke about this earlier today, I think that all rendering tests are having issues in actions/jammy CI at the moment.

@hidmic
Copy link
Contributor Author

hidmic commented Dec 7, 2022

Alright, thanks @adityapande-1995 ! Going in.

@hidmic hidmic enabled auto-merge (squash) December 7, 2022 11:25
@hidmic hidmic disabled auto-merge December 7, 2022 11:26
@hidmic
Copy link
Contributor Author

hidmic commented Dec 7, 2022

@arjo129 this PR cannot be merged until you approve or dismiss your earlier review.

@arjo129 arjo129 dismissed their stale review December 7, 2022 11:48

Should be all good

@hidmic hidmic merged commit 306e92a into gz-sensors7 Dec 7, 2022
Core development automation moved this from In review to Done Dec 7, 2022
@hidmic hidmic deleted the hidmic/dvl branch December 7, 2022 12:52
@hidmic
Copy link
Contributor Author

hidmic commented Dec 7, 2022

@caguero may I ask you to release gz-sensors7? We'll need that for CI in gazebosim/gz-sim#1804 to pass.

hidmic added a commit that referenced this pull request Dec 9, 2022
hidmic added a commit that referenced this pull request Dec 9, 2022
This reverts commit 306e92a.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@azeey azeey moved this from Done to Highlights in Core development Dec 12, 2022
iche033 pushed a commit that referenced this pull request Dec 12, 2022
This reverts commit 306e92a.
Revert addition of DopplerVelocityLog due to need for new eigen dependency

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
iche033 pushed a commit that referenced this pull request Jan 27, 2023
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden needs upstream release Blocked by a release of an upstream library
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants