-
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
uORB Subscription callbacks with WorkItem scheduling on publication #12207
Conversation
cfbdf2a
to
cd47b84
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.
@dagar this make sense to me, but life cycle management needs to be documented.
cd47b84
to
2203e4d
Compare
2203e4d
to
739d6c3
Compare
2f9e550
to
c62eb55
Compare
c62eb55
to
ff6cc9d
Compare
c1fba18
to
571dc35
Compare
As discussed on the dev call, here's a combined branch for testing. #12256 |
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.
From a design point, what I'm still missing is how you plan to handle failover, and sensor timeouts in particular?
There's also a callback on 6516ae8#diff-69f93e040a677c931e797b5203c03387R101 6516ae8#diff-e1d4d75170cc48374d5f144ebfca43d9R105 As for the most other modules, they generally don't do anything on poll timeout other than setup the poll again. We'll still need to be careful and check things on a case-by-case basis. |
0e7a407
to
312999b
Compare
312999b
to
a67e42b
Compare
Updated interval to microseconds to properly handle pwm rates (50-400 Hz or an interval of 2500 - 20000us). |
The testing of this PR with several migrated modules (#12256) looks good. Unless there's any objection let's merge this (not yet used anywhere) and continue iterating within the context of updating appropriate modules (#12224, #12225, #12226, etc). If there are still concerns about failover or timeout handling it needs to be handled at that level anyway. |
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.
Re timeout: that works then. I was under the impression you wanted to pull the rate controller into the sensor acquisition thread.
Still a possibility, but not if we don't need to. I'm actually considering going the other direction and pulling the filtering out of each driver and into the rate controller thread. |
The goal of this PR is to introduce a new module scheduling mechanism that combines the best aspects of tasks and work queues.
Background
Historically PX4 modules have been either individual tasks (primarily) or run out of the NuttX work queues (HPWORK/LPWORK). There are pros and cons to each.
Tasks
Work queues
Pull Request
This PR adds a new uORB Subscription class (uORB::SubscriptionCallbackWorkItem) that has a callback mechanism to schedule a cycle of a WorkItem when new data is published.
As an example I've also updated px4fmu (the pwm output module) and mc_att_control (multicopter attitude + rate controller) to run in the rate_ctrl WQ, rather than as two separate tasks.
New PX4 Work queue + ORB callback scheduling (PR)
Example (compared to master)
Generic multicopter on a pixracer
master
PR
Summary
By only moving two tasks to a workqueue overall cpu load decreased by a couple percent and we've saved over 5 kB of ram.
The downside? End-to-end control latency increased slightly (~ 90 us), but this is a result of the combined mc_att_control that can easily be split following this PR.
There are a number of other opportunities for merging multiple tasks into workqueues like this that will result in significant memory savings and performance improvements. Perhaps more importantly for some is that this structure will enable us to run the multicopter rate controller significantly faster without choking. The small cpu savings above mainly comes from reducing the number of context switches (at a paltry 250 Hz) that become crippling at significantly higher rates (1 - 4 kHz).