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

Added magnetometer value based on location #1907

Merged
merged 5 commits into from
Mar 2, 2023

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Feb 24, 2023

🦟 Bug fix

Summary

The magnetometer sensor is publishing a value but it's not based on the location. This PR calculate the value based on the location.

Checklist

  • Signed all commits for DCO
  • Added tests
  • 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: Alejandro Hernández Cordero <[email protected]>
@mjcarroll
Copy link
Contributor

I think that we need some documentation on the derivation of these tables. It's worth-while to note that these are for a) earth only and b) only valid in a certain window (I think they are updated every 5 years).

A more generic solution would allow for the consumption of the world magnetic model: https://www.ncei.noaa.gov/products/world-magnetic-model, but I understand if that's out of the scope of the current PR.

@ahcorde
Copy link
Contributor Author

ahcorde commented Feb 24, 2023

Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@mjcarroll
Copy link
Contributor

I used this code from this other Gazebo classic plugin

Ah, makes sense, all good then. I think the only other thing I would request is a switch to be able to switch between a constant magnetic declination/field and a varying one. That way people can opt into the new behavior (in the unlikely case someone was depending on a constant field?)

Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde
Copy link
Contributor Author

ahcorde commented Feb 24, 2023

I used this code from this other Gazebo classic plugin

Ah, makes sense, all good then. I think the only other thing I would request is a switch to be able to switch between a constant magnetic declination/field and a varying one. That way people can opt into the new behavior (in the unlikely case someone was depending on a constant field?)

the spherical coordinates should turn on and off this feature.

@mjcarroll
Copy link
Contributor

the spherical coordinates should turn on and off this feature.

Perfect, I think that is reasonable.

Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Merging #1907 (7f7e2dd) into gz-sim7 (3dc8c7a) will decrease coverage by 0.15%.
The diff coverage is 63.12%.

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

@@             Coverage Diff             @@
##           gz-sim7    #1907      +/-   ##
===========================================
- Coverage    64.68%   64.54%   -0.15%     
===========================================
  Files          343      347       +4     
  Lines        27430    27766     +336     
===========================================
+ Hits         17743    17921     +178     
- Misses        9687     9845     +158     
Impacted Files Coverage Δ
include/gz/sim/Util.hh 100.00% <ø> (ø)
...rc/systems/ackermann_steering/AckermannSteering.hh 100.00% <ø> (ø)
src/systems/joint_controller/JointController.hh 100.00% <ø> (ø)
...int_position_controller/JointPositionController.hh 100.00% <ø> (ø)
src/systems/magnetometer/Magnetometer.cc 51.77% <10.86%> (-20.86%) ⬇️
src/gui/plugins/component_inspector/Inertial.cc 11.76% <11.76%> (ø)
src/SdfGenerator.cc 90.33% <40.00%> (-0.62%) ⬇️
src/systems/apply_link_wrench/ApplyLinkWrench.cc 77.77% <46.15%> (ø)
...int_position_controller/JointPositionController.cc 74.13% <67.56%> (+0.56%) ⬆️
src/Util.cc 91.29% <71.42%> (-0.95%) ⬇️
... and 15 more

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

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

Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@mjcarroll
Copy link
Contributor

CI Failures here aren't from this PR.

@mjcarroll mjcarroll merged commit 13c582a into gz-sim7 Mar 2, 2023
@mjcarroll mjcarroll deleted the ahcorde/7/magnetometer_based_on_location branch March 2, 2023 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants