-
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
introduce sensor_gyro_control message (1 kHz rate controller) #12145
Conversation
fc24649
to
fa3c66f
Compare
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.
Looks awsome 👍
Project board with all the blocking pieces that will need to be resolved before moving forward with this change. |
fa3c66f
to
7e755b9
Compare
7e755b9
to
f4cfee4
Compare
Almost there! |
792f48f
to
4474522
Compare
Tested on PixRacer V4: Position Mode: Good. - Procedure Notes: Log: Tested on CUAV V5+: Position Mode: Good. - Procedure Notes: Log: |
Tested on NXP FMUK66 v3: Modes Tested Procedure Notes: Log: Tested on Pixhawk Pro v4Pro: Modes Tested Procedure Notes: Log: Tested on Pixhawk 4 mini v5: Modes Tested Procedure Notes: Log: |
4474522
to
21c8ebf
Compare
f3a24d5
to
3a754e6
Compare
Flight tested on a 250 racer, pixhawk 4 mini. This PR flies great!
The last flight was a ton of fun! :) Nice work @dagar ! |
I did a bit more flight testing after resetting the airframe to ensure default params: 250 generic quad pixhawk 4 mini. Everything was awesome! I put the code through a few more lively flight tests involving full throttle climb in manual to neutral throttle position mode forcing the autopilot to recover a ballistic climb. After recoveries at ~100ft altitude I followed by zero throttle freefalls in manual again recovered by position mode, (only a few impacted the ground before the autopilot arrested the descent rate, but apparently those logs did not survive. The flights that ended abruptly landed right-side up with no damage... this was an entertaining test session!!!): Fantastic results and fun to fly! Nice work @dagar ! |
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.
1 kHz rate control is getting closer!
I started to review, but I would need more info to understand where this is going. More precisely:
- are sensor drivers going to publish on both topics (
sensor_gyro
andsensor_gyro_control
) forever, or is it an incremental step towards something simpler? - I see the fact a driver may publish on two topics with different uorb instances as unecessarily confusing
- in VehicleAngularAcceleration, what prevents you from polling
sensor_gyro_control
only?
I feel that it would be much simpler to go for the following:
- PX4Gyroscope drivers publish raw (
xyz_raw
) and unfiltered (xyz
) data at full rate in a single topic, with one instance per sensor (let's call the topicsensor_gyro
) - VehicleAngularVelocity polls on the selected
sensor_gyro
instance, then performs corrections, then
applies low pass filters and publishes at full rate on topicvehicle_angular_velocity
both the filtered values (xyz
) for use in controllers, and unfiltered values (xyz_unfiltered
) for use in estimators and whatever custom usage - EKF2 polls
vehicle_angular_velocity
, integrates the non-filtered values, and performs its update at whatever reduced rate best fits
src/modules/sensors/vehicle_angular_velocity/VehicleAngularVelocity.cpp
Outdated
Show resolved
Hide resolved
This is an incremental step until we can properly kill off the driver framework drivers (https://github.com/PX4/Firmware/tree/master/src/platforms/posix/drivers) and have all sources of IMU data respect the same interface (what PX4Gyroscope provides).
That's close to where I'm headed. Once the DriverFramework drivers are gone (dagar#19) all IMU data will flow through PX4Accelerometer and PX4Gyroscope. At that point we'll be able to easily make these changes and drop more of the legacy debt. Eventually I'd like to get to the point where drivers simply provide the raw data and nearly all processing happens downstream.
The estimator data path will be a bit different because we need to integrate raw data and then provide all instances of it (multi-EKF). At the moment I'm thinking of queuing the raw data (often 4 frames, but will be more soon) and doing the integration work in the same thread as the estimator (further optimizing the inner loop). |
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.
Isn't _selected_sensor_device_id
always valid? If it is not, ignore my inline suggestions
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
src/modules/sensors/vehicle_angular_velocity/VehicleAngularVelocity.cpp
Outdated
Show resolved
Hide resolved
Ok, thanks for the explanation. As long as |
Tested on Pixhawk4 v5 f-450 Acro mode Mode: Good. Notes: Log: pixhawk4 pro f450 log: |
Co-Authored-By: Julien Lecoeur <jlecoeur@users.noreply.github.com>
…ocity.cpp Co-Authored-By: Julien Lecoeur <jlecoeur@users.noreply.github.com>
…ocity.cpp Co-Authored-By: Julien Lecoeur <jlecoeur@users.noreply.github.com>
…ocity.cpp Co-Authored-By: Julien Lecoeur <jlecoeur@users.noreply.github.com>
Thanks @jlecoeur, all comments addressed. |
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.
LGTM
I have not flight tested. I have flashed on a pixhawk 4, which revealed different publish rates by the gyros. This will have to be addressed in the drivers.
Thanks everyone. |
With the code sprawl in the existing IMU drivers gradually coming under control (#12079, #12128, #12139, #12140, #12141) one of the things we can now easily do is change the underlying message architecture.
This PR creates a new
sensor_gyro_control
message that contains only calibrated + filtered (IMU_GYRO_CUTOFF) gyro data published for every sample. The existingsensor_gyro
is only published with each integrator reset (250 Hz).The result is the primary gyro, rate controller, and output module running synchronized at the native sample rate (currently 1 kHz on most setups).
The downside is the increased cpu usage from mc_att_control and the output module (px4fmu, px4io, etc), but I have several ideas (#12207) that will get that under control. This also won't be safe to merge until all gyro sources are updated to utilize the PX4Gyroscope class (eg #12142, #12141, #12140, #12139, #12136, #12128).