commander force offboard control update when first entering mode

[dagar]

Working on a fix for #12241.

[dagar]

@julianoes the current theory (untested) is that the offboard subscription within commander isn't updating the first time through before hitting the timeout. This is an optimization in uORB::Subscription to prevent subscriptions from creating every single uORB::DeviceNode (publication) that may never actually be published.

[julianoes]

@dagar this is hard to review. You did a refactor and presumably a functional change.

[julianoes]

This fixes the test but it worries me that this force init is required.

[dagar]

This fixes the test but it worries me that this force init is required.

Background:
When you subscribe with orb_subscribe it actually creates the uORB::DeviceNode (the internal data structure for each orb publication). As a result typical systems have several kilobytes worth of unused publications (take a look in /obj in stable vs master). One of the ways the updated uORB::Subscription saves memory is by only "subscribing" to topics that actually exist. Unfortunately the orb existence check is relatively expensive, so I spaced it out after 1000 attempts.

https://github.com/PX4/Firmware/blob/79d4c09d59759efbc228f413dda992562004e29e/src/modules/uORB/Subscription.cpp#L77-L82

What all of this means is that an orb publication that advertises and publishes late at runtime (eg mavlink offboard) won't be subscribed to immediately.

Now back in commander when switching to OFFBOARD mode we know an offboard_control_mode message is required. The change in this PR simply forces that initial subscription attempt immediately when first going into OFFBOARD. If the offboard timeout were slightly longer we would probably get away with it as is.

Does that make sense?

[dagar]

@dagar this is hard to review. You did a refactor and presumably a functional change.

It was originally more of a refactor to enable the change, but it ended up simpler. I'll split it into multiple commits.

[julianoes]

Does that make sense?

Ok I suppose that makes sense.

Unfortunately the orb existence check is relatively expensive

That's the strcmp, right? And if so could we make that more efficient?

[dagar]

Unfortunately the orb existence check is relatively expensive

That's the strcmp, right? And if so could we make that more efficient?

Yes, iterating a linked list of all publications and strcmp. https://github.com/PX4/Firmware/blob/ae6fed4f2915def3df6adadb850c559b050055fd/src/modules/uORB/uORBDeviceMaster.cpp#L444-L447

What I'd like to do is generate an ordinal number for each orb topic and then have a simple bitset of published topics, but we'll need to be careful on platforms like Snappy where those things need to be in sync.

[julianoes]

Yes good to go from my side. Thanks!

[julianoes]

platforms like Snappy where those things need to be in sync.

Right, that's annoying.

[julianoes]

Squash or rebase?