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.

Sign up for GitHub

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

Jump to bottom

Added airspeed sensor #1847

Merged
merged 9 commits into from
Feb 11, 2023
Merged

Added airspeed sensor #1847

merged 9 commits into from
Feb 11, 2023

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Dec 23, 2022 •
edited
Loading

Signed-off-by: Alejandro Hernández Cordero ahcorde@gmail.com

🎉 New feature

Summary

Added air speed sensor

Depends on:

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.

@ahcorde
Added airspeed sensor
4b4496f 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Dec 23, 2022
@ahcorde
Clean ups
5a5b5ba 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde ahcorde mentioned this pull request Dec 29, 2022
Closed
@bperseghetti
Copy link
Member

What's the timeline of this being updated from a draft, would love to start using this upstream!

@ahcorde ahcorde self-assigned this Feb 3, 2023
@ahcorde ahcorde requested a review from jennuine February 3, 2023 13:43
@ahcorde ahcorde marked this pull request as ready for review February 3, 2023 13:43
@mjcarroll mjcarroll added the needs upstream release Blocked by a release of an upstream library label Feb 3, 2023
@ahcorde
Use Airspeed sensor msgs
1151011 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde ahcorde removed the needs upstream release Blocked by a release of an upstream library label Feb 10, 2023
@ahcorde
Copy link
Contributor Author

ahcorde commented Feb 10, 2023

@osrf-jenkins run tests please!

@ahcorde
Fixes
aeeeba1 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde ahcorde mentioned this pull request Feb 10, 2023
Merged
@ahcorde
make linters happy
9999d98 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@codecov
Copy link

codecov bot commented Feb 10, 2023 •
edited
Loading

Codecov Report

Merging #1847 (b01bda8) into gz-sim7 (3dc8c7a) will increase coverage by 0.06%.
The diff coverage is 75.64%.

❗ Current head b01bda8 differs from pull request most recent head 154d2c9. Consider uploading reports for the commit 154d2c9 to get more accurate results

@@             Coverage Diff             @@
##           gz-sim7    #1847      +/-   ##
===========================================
+ Coverage    64.68%   64.74%   +0.06%     
===========================================
  Files          343      347       +4     
  Lines        27430    28776    +1346     
===========================================
+ Hits         17743    18632     +889     
- Misses        9687    10144     +457
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/SdfGenerator.cc 90.33% <40.00%> (-0.62%) ⬇️
src/Conversions.cc 85.90% <57.14%> (+1.91%) ⬆️
...rc/systems/ackermann_steering/AckermannSteering.cc 84.07% <74.25%> (-1.48%) ⬇️
src/systems/air_speed/AirSpeed.cc 75.55% <75.55%> (ø)
src/Util.cc 92.06% <76.47%> (-0.18%) ⬇️
src/systems/joint_controller/JointController.cc 78.74% <79.03%> (-3.62%) ⬇️
... 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.

@ahcorde
Copy link
Contributor Author

ahcorde commented Feb 10, 2023

@jennuine and @mjcarroll friendly ping, I would love to merge this, there is a PR in PX4 waiting for a while

@mjcarroll
Copy link
Contributor

It looks good, it looks like there aren't tests to exercise some of the new functionality (at least according to coverage) can you add some?

@ahcorde
Added test
28135bb 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Feb 10, 2023

@mjcarroll Added test 28135bb

iche033
iche033 reviewed Feb 10, 2023
View reviewed changes
src/Util.cc Outdated
// vel = parentVel->Data() * vel;
// // keep going up the tree
// v = _ecm.Component<components::ParentEntity>(v->Data());
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

remove?

src/Util.cc Outdated
}

math::Vector3d vel = worldLinVel->Data();
auto v = _ecm.Component<components::ParentEntity>(_entity);
Copy link
Contributor

Choose a reason for hiding this comment

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

unused variable v

examples/worlds/sensors.sdf
@@ -136,21 +136,20 @@
</vertical_velocity>
</altimeter>
</sensor>
<sensor name="air_pressure" type="air_pressure">
<sensor name="air_speed" type="air_speed">
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add a new air_speed sensor (and keep the existing air_pressure sensor)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you also need to add the gz-sim-air-speed-system?

test/integration/air_speed_system.cc Outdated
@@ -0,0 +1,136 @@
/*
* Copyright (C) 2019 Open Source Robotics Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

2019 - > 2023

@ahcorde
make linters happy
b01bda8 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Added feedback
154d2c9 
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from iche033 February 11, 2023 00:14
@ahcorde ahcorde merged commit 5881559 into gz-sim7 Feb 11, 2023
@ahcorde ahcorde deleted the ahcorde/air_speed branch February 11, 2023 08:39
henrykotze pushed a commit to henrykotze/gz-sim that referenced this pull request Nov 28, 2023
Copied Airspeed Sensor PR: gazebosim#1847 and renamed
afbb02d
henrykotze pushed a commit to henrykotze/gz-sim that referenced this pull request Nov 29, 2023
Copied Airspeed Sensor PR: gazebosim#1847 and renamed
705b514 
Signed-off-by: henrykotze <henry@flycloudline.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjcarroll mjcarroll mjcarroll approved these changes

@jennuine jennuine Awaiting requested review from jennuine

@iche033 iche033 Awaiting requested review from iche033

Assignees

@ahcorde ahcorde

Labels
🌱 garden Ignition Garden
Projects
Core development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ahcorde @bperseghetti @mjcarroll @iche033