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 API for enabling / disabling IMU orientation #142

Merged
merged 3 commits into from
Jul 10, 2021

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Jul 1, 2021

Signed-off-by: Ian Chen [email protected]

🎉 New feature

Not all IMUs produce orientation estimates. This PR adds a new API to the IMU sensor class to allow users to disable computing and publishing orientation values.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

@iche033 iche033 requested a review from mjcarroll July 1, 2021 23:57
@github-actions github-actions bot added the 🔮 dome Ignition Dome label Jul 1, 2021
@codecov
Copy link

codecov bot commented Jul 2, 2021

Codecov Report

Merging #142 (42522e8) into ign-sensors4 (e9aba3d) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head 42522e8 differs from pull request most recent head 43c4050. Consider uploading reports for the commit 43c4050 to get more accurate results
Impacted file tree graph

@@               Coverage Diff                @@
##           ign-sensors4     #142      +/-   ##
================================================
+ Coverage         76.10%   76.16%   +0.05%     
================================================
  Files                23       23              
  Lines              2390     2396       +6     
================================================
+ Hits               1819     1825       +6     
  Misses              571      571              
Impacted Files Coverage Δ
src/ImuSensor.cc 89.09% <100.00%> (+0.62%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9aba3d...43c4050. Read the comment docs.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM, just a nit about style.

src/ImuSensor_TEST.cc Outdated Show resolved Hide resolved
iche033 added 2 commits July 1, 2021 17:51
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

A minor style issue, but otherwise LGTM

// orientation should still be the previous value because it is not being
// updated.
EXPECT_EQ(newOrientValue, sensor->Orientation());

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

@chapulina chapulina merged commit 95bff1b into ign-sensors4 Jul 10, 2021
@chapulina chapulina deleted the imu_orientation branch July 10, 2021 02:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants