uORB add WorkItem callbacks on publication #12159
Conversation
dagar
force-pushed
the
pr-orb_wq_schedule_callback
branch
from
cabe881 to
f8bdacb
Compare
They're responsible for removing (unregister) themselves when stopping. For most usage I anticipate this would be rolled into a module base class. At that point simple modules will effectively be actors/active objects. |
dagar
force-pushed
the
pr-orb_wq_schedule_callback
branch
from
f8bdacb to
38b7b87
Compare
|
Just from a super high level, I'd love it if the interface was the same as regular subscription to a topic, but with an enum to specify the priority. So iif there is something super critical you just use the Not sure if that is the direction this is going, maybe just laying the foundations for something like that here? |
Actually that's part of why it's tightly integrated with the WQ, where the priority is set ahead of time for the specific thread. I did actually consider the direct callback, but it gets questionable with the different publication contexts and stack size concerns. For example, up until recently a huge portion of publications were happening from interrupts. I should mention the high level idea here is that several of the "hot paths" involving chains of multiple modules will now actually be able to share a single WQ thread. Example:
Publishing data is what queues the next module to run. If those modules are in the same WQ they'll run back to back without even a context switch. |
dagar
force-pushed
the
pr-orb_wq_schedule_callback
branch
from
38b7b87 to
63db265
Compare
There was a problem hiding this comment.
The biggest danger I see, as well as @davids5, is dangling pointers if orb_unregister_work_callback is forgotten. If wrapped accordingly into objects, it will work though.
@dagar I see various people (including myself) having questions around the more high-level overall design. Is there a document where you outline your vision, discuss alternative solutions, compare to the current state, and pros/cons of each?
I think that would be helpful.
| uORB::Manager::orb_register_work_callback(px4::WorkItem *item, const orb_metadata *meta, int instance) | ||
| { | ||
| // find node and insert callback (like pollset) | ||
| if ((item != nullptr) && (meta != nullptr)) { |
There was a problem hiding this comment.
I prefer early returns here, it will be more readable.
| // prevent duplicate registrations | ||
| for (auto existing_item : _registered_work_items) { | ||
| if (item == existing_item) { | ||
| PX4_ERR("work item already registered, skipping"); |
There was a problem hiding this comment.
This needs to go after ATOMIC_LEAVE.
Also such low-level methods should return an appropriate error code instead of using printf.
| #include <px4_defines.h> | ||
| #include <drivers/drv_hrt.h> | ||
|
|
||
| namespace px4 | ||
| { | ||
|
|
||
| class WorkItem : public IntrusiveQueueNode<WorkItem *> | ||
| class WorkItem : public IntrusiveQueueNode<WorkItem *>, public ListNode<WorkItem *> |
There was a problem hiding this comment.
Can you document the purpose of IntrusiveQueueNode and ListNode?
Continued in #12207 with some background and example. |
This PR adds a simple callback mechanism to uORB on publication that schedules a cycle of a WorkItem in the new PX4 work queue.
This is effectively an alternative to the typical PX4 pattern of a task per module with a main loop polling a single topic.
Coupling uORB to WorkItems wasn't my first choice, but after experimenting with various c++ delegate options the simplicity of this with the intended usage is appealing. I'm certainly open to suggestions though.