send_event: move to new WQ and uORB::Subscription #12176
Conversation
| * @class SubscriberHandler | ||
| * Contains a list of uORB subscriptions and maintains their update state. | ||
| */ | ||
| class SubscriberHandler |
There was a problem hiding this comment.
I think the original idea of the SubscriberHandler was to save memory by not having all event tasks subscribe to the same uorb topics. But I don't know what the quantitative difference for this approach is.
| } | ||
|
|
||
| ret = wait_until_running(); | ||
| SendEvent *send_event = new SendEvent(); |
There was a problem hiding this comment.
This runs now in a different task, which can lead to problems.
There was a problem hiding this comment.
Yes, that's why it's also moving to uORB::Subscription in the same PR. Other than file descriptors what problems are you thinking of?
There was a problem hiding this comment.
Yes fd's being the main point, and it's more in general, since orb_subscribe can still be used and tasks might open actual files, pipes, etc. It's just a better design doing it in the same context and it's a typical source of error that is difficult to debug.
I'm also thinking of task-based accounting of resources.
There was a problem hiding this comment.
This was a design decision to make the PX4 WQ as simple as possible for the majority of drivers and modules that do not (and should not) open actual files, pipes, etc. Unless you see an exception here we need to keep going. We're very close to not needing HPWORK and LPWORK (2 entire tasks with relatively large stacks).
I'm also trying to get to module-based resource accounting. With current modules and traditional orb usage too much is hidden (the size of the allocation of the main object, true cost of all these orb subscriptions, other dynamic allocations, etc).
There was a problem hiding this comment.
Serial drivers, such as distance sensors are already one exception we have. Existing drivers work around the problem, but I still see people making mistakes w/o being able to figure out what the issue is.
While I don't question it's simpler without that requirement, it's also a step back.
There was a problem hiding this comment.
The distance sensors are a large inconsistent mess that needs structure (only 1 uses ModuleBase) and serial in general is likely going to need to be abstracted both to handle port deconfliction PX4 side and the possibility of porting to another platform (much longer discussion). Virtually all of these drivers evolved via copy and paste because hardly anyone actually understands how to create a module from scratch.
We need a good interface for handling this case (2 phase init, etc), implement it in every driver we actually care about, and then provide a well documented template.
What do you want to do here?
There was a problem hiding this comment.
Yes exactly. Without having looked at the details, an idea would be to add a helper method that calls a provided callback on the wq (which does the initialization), and the helper method waits for that to finish, so it can keep the required WorkItem on the stack.
The resulting effect of a more serialized startup would also help with orb instance and driver class instance race conditions (even more important when splitting gyro topics). Currently when multiple drivers initialize separate PX4Accelerometer instances concurrently, there is no guarantee to get consistent instances.
There was a problem hiding this comment.
Thinking about this again, each WorkItem could really be a small state machine (extending _task_should_exit).
- initializing
- running
- waiting
- stopping
- etc
Those could correspond to different methods we optionally fill in as needed for specific states or transitions.
dagar
force-pushed
the
pr-send_event_workqueue
branch
from
26cc33a to
b7424c6
Compare
|
Test flown on a 250 quad, pixhawk 4 mini, RC transmitter powered off part way through the flight. |
|
This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions. |
No description provided.