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 multichannel lookup for environment sensors. #1814

Merged
merged 7 commits into from
Dec 15, 2022

Conversation

arjo129
Copy link
Contributor

@arjo129 arjo129 commented Nov 25, 2022

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

🎉 New feature

Closes #

Summary

Often when sensing we may be interested in vector quantities. This PR extends the environmental_sensor to support vector quantities. Some additional work was required handle transformations. In particular if the data is something like Ocean Currents or Wind then the data has to be transformed to the robot's local coordinate frame and the robot's velocity also has to be accounted for.

Test it

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.

Some data such as ocean currents or wind speed take on vector values. This PR adds support for such types of data.

Signed-off-by: Arjo Chakravarty <[email protected]>
Often when sensing we may be interested in vector quantities. This PR extends the environmental_sensor to support vector quantities. Some additional work was required handle transformations. In particular if the data is something like Ocean Currents or Wind then the data has to be transformed to the robot's local coordinate frame and the robot's velocity also has to be accounted for.

Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
@arjo129 arjo129 added the MBARI-LRAUV Sponsored by MBARI-LRAUV project: https://github.com/osrf/lrauv label Nov 25, 2022
@arjo129 arjo129 requested a review from mjcarroll as a code owner November 25, 2022 07:42
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Nov 25, 2022
@arjo129 arjo129 changed the title Arjo/feat/multi channel lookup Add multichannel lookup for environment sensors. Nov 25, 2022
@arjo129 arjo129 mentioned this pull request Nov 25, 2022
38 tasks
@codecov
Copy link

codecov bot commented Nov 25, 2022

Codecov Report

Merging #1814 (6166f78) into gz-sim7 (4fa1f26) will increase coverage by 0.26%.
The diff coverage is 85.23%.

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

@@             Coverage Diff             @@
##           gz-sim7    #1814      +/-   ##
===========================================
+ Coverage    64.37%   64.64%   +0.26%     
===========================================
  Files          341      342       +1     
  Lines        27132    27383     +251     
===========================================
+ Hits         17467    17701     +234     
- Misses        9665     9682      +17     
Impacted Files Coverage Δ
...nmental_sensor_system/EnvironmentalSensorSystem.cc 83.25% <82.67%> (+0.49%) ⬆️
...tems/environmental_sensor_system/TransformTypes.hh 100.00% <100.00%> (ø)
src/systems/buoyancy/Buoyancy.hh 100.00% <0.00%> (ø)
src/SimulationRunner.cc 92.27% <0.00%> (+0.94%) ⬆️
src/systems/buoyancy/Buoyancy.cc 85.87% <0.00%> (+5.21%) ⬆️

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

Signed-off-by: Arjo Chakravarty <[email protected]>
@azeey azeey requested a review from caguero November 28, 2022 19:46
@arjo129 arjo129 requested a review from hidmic December 1, 2022 12:39
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

First pass. I don't yet buy the overall design.

Signed-off-by: Arjo Chakravarty <[email protected]>
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

Overall LGTM. CI failures look unrelated (would a rebase or merging back fix them?).

break;
default:
result = (_reading + offset);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@arjo129 meta: it would almost seem like local/global is orthogonal to whether velocity should be computed relative some observer or the world? Would it make sense to split them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats true, but in the interest of time we can check this in.

// Create a vehicle rotated 90 degrees in the yaw axis
math::Pose3d pose(math::Vector3d(0, 0, 0), math::Quaterniond(0, 0, GZ_PI_2));

// Vehicle should moving at 1m/s along x-axis (In global frame)
Copy link
Contributor

Choose a reason for hiding this comment

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

@arjo129 nit:

Suggested change
// Vehicle should moving at 1m/s along x-axis (In global frame)
// Vehicle should be moving at 1m/s along x-axis (In global frame)

// Vehicle should moving at 1m/s along x-axis (In global frame)
math::Vector3d velocity(1, 0, 0);

// Imagine a current moving against the vehicle
Copy link
Contributor

Choose a reason for hiding this comment

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

@arjo129 imagine is an interesting choice of words.

Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

@arjo129 arjo129 merged commit 652e238 into gz-sim7 Dec 15, 2022
@arjo129 arjo129 deleted the arjo/feat/multi_channel_lookup branch December 15, 2022 23:21
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://github.com/osrf/lrauv
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants