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 DVL specific messages #317

Merged
merged 2 commits into from
Nov 23, 2022
Merged

Add DVL specific messages #317

merged 2 commits into from
Nov 23, 2022

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Nov 21, 2022

🎉 New feature

Summary

This patch brings a number of DVL specific messages, necessary for DVL sensor interfaces.

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 <[email protected]>
@hidmic hidmic added 🌱 garden Ignition Garden MBARI-LRAUV Sponsored by MBARI-LRAUV project: https:/osrf/lrauv labels Nov 21, 2022
@hidmic hidmic requested a review from caguero as a code owner November 21, 2022 18:58
@codecov
Copy link

codecov bot commented Nov 21, 2022

Codecov Report

Merging #317 (d27b90f) into gz-msgs9 (85102af) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           gz-msgs9     #317   +/-   ##
=========================================
  Coverage     95.41%   95.41%           
=========================================
  Files            10       10           
  Lines          1026     1026           
=========================================
  Hits            979      979           
  Misses           47       47           

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

Copy link
Contributor

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

LGTM

Copy link
Collaborator

@caguero caguero 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 a few minor clarifications and suggestions.

{
enum ReferenceType
{
DVL_REFERENCE_UNSPECIFIED = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you document each element of the enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in d27b90f.

/// \brief Measured background noise spectral density.
double nsd = 5;

/// \brief Whether beam is locked or not.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this is obvious if you're familiar with DVLs but I don't have a good intuition about what does it mean "locked" in this context. It's OK to also include a link to some reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in d27b90f.

DVLRangeEstimate range = 3;

/// \brief Target position estimate.
DVLKinematicEstimate position = 2;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason for placing the position = 2 after range = 3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason. Fixed in d27b90f.

/// \brief Type of target used for tracking.
TargetType type = 1;

/// \brief Target range (or distance).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd recommend to mention the units here and in the position fields

Copy link
Contributor Author

@hidmic hidmic Nov 23, 2022

Choose a reason for hiding this comment

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

Are there units other than SI units? :) Done in d27b90f.

DVLTrackingTarget target = 3;

/// \brief Estimated velocity.
DVLKinematicEstimate velocity = 4;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the difference between this velocity field and the velocity of each beam?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added clarification in d27b90f.

double rssi = 4;

/// \brief Measured background noise spectral density.
double nsd = 5;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not familiar with this term, does it have units?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added units in d27b90f. Noise power spectral density characterize how much noise power is there per unit bandwidth.

Signed-off-by: Michel Hidalgo <[email protected]>
@hidmic
Copy link
Contributor Author

hidmic commented Nov 23, 2022

Alright, going in!

@hidmic hidmic merged commit ad32258 into gz-msgs9 Nov 23, 2022
@hidmic hidmic deleted the hidmic/dvl branch November 23, 2022 15:39
@hidmic
Copy link
Contributor Author

hidmic commented Nov 23, 2022

@nkoenig would it be possible to roll out a gz-msgs9 minor release?

@hidmic hidmic mentioned this pull request Nov 23, 2022
38 tasks
@caguero
Copy link
Collaborator

caguero commented Nov 23, 2022

@nkoenig would it be possible to roll out a gz-msgs9 minor release?

I can help with the release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden MBARI-LRAUV Sponsored by MBARI-LRAUV project: https:/osrf/lrauv
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants