-
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
PX4 general work queue #11261
PX4 general work queue #11261
Conversation
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 - Can the changes to all the boards for px4_platform_init() should be in a separate commit?
Yes, I'll split it out as a separate PR to come in first. I thought it might be slightly easier to first introduce the idea here as motivation. |
1c384bb
to
3fa18b0
Compare
e8f4080
to
7dbdc20
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.
This looks good. Just did an initial pass today. I may have more to add as I start shuffling things into this. See review comments, found a bug.
1091eb9
to
a1d2045
Compare
Looks good! What's your idea now with the shutdown of the WorkQueueManager? Instead of asking the WorkItems to stop, you could also block the WorkQueue and WorkQueueManager while there are WorkItems active. Aside from that, I don't really see a reason why you would want a WorkQueue or WorkQueueManager to stop. Even if there is a temporary WorkItem (for example a module running from nsh) on a WorkQueue, you probably want to keep the WorkQueue in case it comes up again. |
@bartslinger the current work queues don't even bother shutting down. The main reason I want it is to be able to run under AddressSanitizer (LeakSanitizer) cleanly. The current idea is that there are core parts of the "PX4 platform" that start together, and later will shutdown together. Before that platform shutdown we'll stop all the drivers and modules. Quite a lot of this is currently missing. It didn't seem worth it to get into more bookkeeping to track all the active usage of WorkQueues and corresponding WorkItems. |
a1d2045
to
0729861
Compare
TODO: add safe bus locking in PX4's SPI class to prevent conflicts between SPI drivers running out of the new work queue and legacy drivers running out of ISRs. This will only be needed for a short transition period. |
0729861
to
50337ff
Compare
@PX4/testflights could you give this an initial test on a pixracer only? Do a thorough ground checkout first, and if that seems good then a short flight with a corresponding comparison flight in master. Thanks! |
Addresses SPI locking |
This PR is getting a little hard to manage, I'll split it again into separate PRs for the new work queue and separately moving all the drivers.
|
More background to come, but in short this general px4 work queue will allow us to finally unblock several important things.
Closes #8814
Blocked on #11302
Design Notes
Each PX4 module that previously used hrt_calls, HPWORK, or LPWORK can now easily be moved into this framework. These classes inherit either WorkItem to be run as needed, or ScheduledWorkItem if a fixed interval is desired.
The reason for the (somewhat awkward) WorkQueueManager is to get every queue running as a thread within the same task on Nuttx. This significantly reduces overhead, and when combined with uORB changes (#11176) has no disadvantages. The cost of each work queue is little more than the stack.
The other option for scheduling WorkItems is something I've bolted into uORB itself. The common pattern throughout most important PX4 modules is a task that polls a uORB topic for updates. Many of these processes are already fundamentally serialized, so the coexistence of all these tasks uses quite a lot of memory. Anything work pipeline (a collection of tasks) that's fundamentally serial can be moved to share the same WorkQueue. Then we can lean on the uORB publication to schedule appropriate work.