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 magnetometer plugin parameter to publish units as tesla #2336

Open
wants to merge 3 commits into
base: gz-sim8
Choose a base branch
from

Conversation

gilborty
Copy link

@gilborty gilborty commented Mar 17, 2024

🦟 Bug fix

Fixes #2312

Summary

As mentioned in #2312, the magnetic field message is incorrectly using Gauss as the units for the field strength, rather than the SI unit of teslas.

This PR adds a flag, use_tesla_for_magnetic_field, to the MagnetometerSystem to enable this fix as to not break any existing functionality that assumes Gauss. It also adds the Configure step to the MagnetometerSystem.

While I was here, I noticed that the integration test doesn't actually update the MagnetometerSystem because the SphericalCoorindates are not set. I think this might be an oversight. I did not change that existing test in case it was like that on purpose.

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.

@gilborty gilborty requested a review from mjcarroll as a code owner March 17, 2024 23:13
@github-actions github-actions bot added the 🎵 harmonic Gazebo Harmonic label Mar 17, 2024
test/integration/magnetometer_system.cc Show resolved Hide resolved
test/integration/magnetometer_system.cc Show resolved Hide resolved
test/integration/magnetometer_system.cc Show resolved Hide resolved
test/integration/magnetometer_system.cc Outdated Show resolved Hide resolved
test/integration/magnetometer_system.cc Show resolved Hide resolved
test/integration/magnetometer_system.cc Outdated Show resolved Hide resolved
test/integration/magnetometer_system.cc Outdated Show resolved Hide resolved
@gilborty
Copy link
Author

Thank you for the review @ahcorde, I think I updated everything you asked for. Thank you for the helper function point out as well!

@gilborty gilborty requested a review from ahcorde March 18, 2024 17:32
@gilborty
Copy link
Author

Are the INTEGRATION_triggered_publisher tests failing on CI for anyone else? Mine are passing locally, I don't think it is related to this commit.

[       OK ] TriggeredPublisherTest.StringMsgReqBooleanRep (1053 ms)
[----------] 23 tests from TriggeredPublisherTest (13137 ms total)

[----------] Global test environment tear-down
[==========] 23 tests from 1 test suite ran. (13137 ms total)
[  PASSED  ] 23 tests.


@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 29, 2024
@iche033
Copy link
Contributor

iche033 commented Aug 7, 2024

with #2460, support for publishing units as tesla should now be available. It would be nice to add the integration test from this PR though.

@azeey azeey removed the beta Targeting beta release of upcoming collection label Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎵 harmonic Gazebo Harmonic
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

Magnetometer system publishes field strength in incorrect units
4 participants