orb_exists: change semantics from (is published or subscribed) to (is published) #8135
Conversation
… published) Existing users of orb_exists: - logger (dynamic subscribe to multi-instances) - mavlink (orb subscription) - sdlog2 - preflightcheck (check for home_position) - wait_for_topic shell command (it's not used) - orb_group_count() (sensors: dynamic sensor addition) All use-cases benefit from the changed semantics: they are really only interested if there is a publisher, not another subscriber.
…instance To keep track of the configured interval, we store it as negative file descriptor, until we do the subscription. This frees up a considerable amount of file descriptors in most use-cases.
If logger is started very early, orb_exists() will fail for a lot of topics, they will be advertised within the next few seconds. Logger already dynamically adds subscriptions during logging, but if we do that before as well, we'll avoid any delays and having to subscribe to a lot of topics all at once.
…ointers to avoid dynamic memory allocations & frees (specifically in orb_exists)
flight review now uses vehicle_status & manual_control_setpoint
Checking with 3Hz for new topics should be fast enough.
src/modules/logger/logger.cpp
Outdated
| // - we avoid subscribing to many topics at once, when logging starts | ||
| // - we'll get the data immediately once we start logging (no need to wait for the next subscribe timeout) | ||
| if (next_subscribe_topic_index != -1) { | ||
| for (uint8_t instance = 0; instance < ORB_MULTI_MAX_INSTANCES; instance++) { |
There was a problem hiding this comment.
using uint8_t as index is not an optimization, you should use a counter of processor word size
There was a problem hiding this comment.
I know, I copy-pasted that :/
Fixed in 37902d9
|
This is great! Should we reduce the nuttx fds again? |
Thanks! I deliberately did not do that (yet), because:
If we're short on RAM we can certainly reduce them. One FD needs 16 bytes, so reducing by one implies around 16*20 (tasks) = 320 Bytes more RAM. |
|
Memory can be tight in some setups right now, but in most cases it's avoidable. For example it's easy for a user to unintentionally configure a pixracer where they'll run out of memory. Certain combinations of USB connected, SYS_COMPANION, vmount, camera trigger, etc can already leave you without enough memory to successfully calibrate. Not to mention UAVCAN. It's too bad we can't (easily) configure the nuttx FDs per task. Alternatively we create another uORB interface that doesn't use FDs (for logger and mavlink), but that would need significantly more thought. Other than mavlink and logger most tasks use < 20 FDs with the exception of commanader, but I already see quite a few ways to bring that down. At what point does it become worth it? Reducing by 20-30 FDs and save 5-10kBs of RAM? |
|
@bkueng Could you look into fixing the CI failures (or rebase) and please ping and tag @PX4TestFlights so they can start testing it? Thanks! |
we're out of flash again :/
bkueng
force-pushed
the
uorb_exists_update
branch
from
825681a to
0bc3d16
Compare
|
@dagar Specifically on Pixracer, we can do one more thing that does not require much effort: How about we reduce the number of FD's by 5? That should definitely be safe. As for dynamic FD's: I already talked to @davids5 and he told me that Greg had some NuttX testing branch for that, but there were some complications with sockets. So I think it's not going to happen soon. But it's also not that urgent anymore with this PR. CI failed because of flash overflow. I removed the blacksheep telemetry driver from fmu-v2. Circle-CI failure is unrelated and succeeded before. @PX4TestFlights this is ready for testing. Please test on all boards and look that:
|
|
couple flights on vtol: https://logs.px4.io/plot_app?log=b8e5a7f0-19ab-4348-84b2-64c179acede2 good flights; no issues; no notable difference from master 747e392 |
|
couple flight on quadcopter running pixracer (v4) https://logs.px4.io/plot_app?log=cd5708ab-986a-4bed-b650-0b695709c64b good flights; no issues; no noticeable different from master d83073f |
Existing users of
orb_exists:All use-cases benefit from the changed semantics: they are really only interested if there is a publisher, not another subscriber.
Additional changes:
This has deeper impacts on the overall system, hopefully mostly positive!
The following provides some test results.
Testing
To get representative results, I tested on a fully assembled quad (pixracer) in armed state with RC and an open QGC connection.
Used file descriptors
Number of used file descriptors (mavlink instance is the one that is connected to QGC, without b50b0b7):
Memory (Fragmentation)
The additional file operations (open & close) in
orb_existsdo not lead to memory allocations/frees on Nuttx, but did so on POSIX. I changed this in 3b31a03.My tests showed a considerable memory reduction of 4.2 KB. A bit more than half is because of mavlink, the rest is logger. Since pixracer runs more mavlink instances than other boards, it will be slightly less on other boards.
CPU usage
Tests showed this is pretty much unchanged (without 191622b). This is the execution time of
orb_existsbefore (with locked scheduling):And this PR:
Obviously slower, but I think it's still ok, since it's not called with high frequency or at critical places.
I suggest reviewing each individual commit.