-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
sensors: create vehicle_angular_velocity module #12596
Conversation
Fixes #12451 (hopefully). |
The rollspeed, pitchspeed, and yawspeed in the vehicle_attitude message (now removed) are from raw IMU data integrated down to 250 Hz. For the majority of consumers it's clear what they actually want is the low pass filtered rate data (now in vehicle_angular_velocity), but there may be exceptions. |
36152e2
to
95a19e4
Compare
The CI failure is related to RTPS:
|
95a19e4
to
ee402b4
Compare
Thanks @jlecoeur, should be fixed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much needed cleanup!
I have made some suggestions and asked a few questions.
src/examples/position_estimator_inav/position_estimator_inav_main.cpp
Outdated
Show resolved
Hide resolved
src/modules/sensors/vehicle_angular_velocity/VehicleAngularVelocity.cpp
Outdated
Show resolved
Hide resolved
src/modules/sensors/vehicle_angular_velocity/VehicleAngularVelocity.cpp
Outdated
Show resolved
Hide resolved
5500b13
to
2580008
Compare
Thanks for the quick review @jlecoeur. |
00a2bbc
to
c2d0def
Compare
Tested on Pixhawk 2 Cube V3:Modes Tested Position Mode: Good. - Procedure Notes: Log: Tested on PixRacer V4: Position Mode: Good. - Procedure Notes: Log: https://review.px4.io/plot_app?log=0fbcb9c3-9bfd-4b23-8b91-1f07a10626ed Tested on CUAV V5+ Position Mode: Good. - Procedure Notes: Log: https://review.px4.io/plot_app?log=5006f107-db4d-469f-832d-01fa0f99e04a |
- mc_att_control split out sensor_gyro aggregation and move to wq:rate_ctrl
Conflict with position_estimator_inav (deleted) fixed manually, then squashed and rebased on master. |
a9b4522
to
4b43c1c
Compare
Modes Tested Position Mode: no issue
Notes: pr log pixhawk 4 v5 f-450: pr log pixhawk 4 mini v5 f-450: pr log pixhawk pro v4 f-450 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see the reasoning, but I find it weird to run this from sensors
:
- it does not interact with that module, or share anything
- and it even runs on a different process
@@ -610,15 +610,16 @@ void Logger::add_default_topics() | |||
add_topic("optical_flow", 50); | |||
add_topic("position_setpoint_triplet", 200); | |||
//add_topic("radio_status"); | |||
add_topic("rate_ctrl_status", 30); | |||
add_topic("rate_ctrl_status", 200); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add this with 30ms to add_high_rate_topics
?
if (_selected_gyro != corrections.selected_gyro_instance) { | ||
if (corrections.selected_gyro_instance < MAX_GYRO_COUNT) { | ||
// clear all registered callbacks | ||
for (auto sub : _sensor_gyro_sub) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (auto& sub : _sensor_gyro_sub) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I agree it's a bit unusual and I originally had it as a standalone module (#12225). At the moment there's no scenario where you don't require both, so it started to feel like more of a liability. It's also going to get worse as more of these aggregators are added (vehicle_acceleration #12597 also now in master). |
@bkueng rate_ctrl_status is already in add_high_rate_topics full rate. Do you want to limit it? |
Sorry I missed that. It's fine then. |
* split out filtered sensor_gyro aggregation from mc_att_control and move to wq:rate_ctrl
I want to know the units of rollspeed, pitchspeed, and yawspeed inside the [rate_ctrl_status.msg] file. The results from the px4 review site are different from those from the matlab. |
@jinwoolee6906 All the angles are in radians and all the angular rates are in rad/s. |
This is a new part of the sensors module that creates the system wide angular velocity message needed by the rate controllers and several other modules. Because this message is needed at a relatively high rate (250 Hz now, >= 1 kHz shortly) with absolute minimum latency it runs out of the WQ rate_ctrl.