core: heartbeat watchdog (deadman timer), reworked - #3020
Closed
julianoes wants to merge 14 commits into
Closed
Conversation
While a watchdog timeout is configured, MAVSDK's periodic heartbeats are only sent as long as the watchdog keeps being fed at least once per timeout period, so heartbeats reflect the liveness of the client application. - Configure via Configuration::set_heartbeat_watchdog_timeout_s() or at runtime via Mavsdk::set_heartbeat_watchdog_timeout_s(); must be 0 (disabled) or at least 1 second. - Feed via Mavsdk::feed_heartbeat_watchdog() (C++/C API) or the FeedHeartbeatWatchdog gRPC RPC. Also exposed as the SetHeartbeatWatchdogTimeout RPC (proto submodule bump) and the mavsdk_server --heartbeat-watchdog-timeout CLI option. - Heartbeats latch off until fed: on enable, timeout change, expiry, and any other stop (e.g. disconnect), so a dead client cannot emit heartbeats across reconnects or discovery. A feed only restarts heartbeats that are supposed to be sent (always_send_heartbeats or a connected system). - Watchdog state is guarded by _heartbeat_mutex with a generation counter against in-flight expiries; configuration writers are serialized on a dedicated mutex, and heartbeat starts/stops self-heal against stale policy snapshots. - Unit tests (expiry, latching, policy, feed restart, reconfiguration, stress) and gRPC service tests.
Use a checked-out MAVLink channel for test inject packing, and define MavlinkChannels::Instance() in the .cpp so shared builds share one allocator.
Address julianoes review on #2905: drive heartbeats from a lifetime CallEvery that gates sends on policy and watchdog state, drop the recursive configuration mutex, move timeout validation onto Mavsdk, and complete mavsdk_server API docs.
maybe_send_heartbeats() evaluated the watchdog state under _heartbeat_mutex but then read always_send_heartbeats outside it. A configuration update that enabled the watchdog could land in between, so one more heartbeat could go out after set_heartbeat_watchdog_timeout_s() had already returned. Read always_send in the same critical section as the watchdog state. is_any_system_connected() stays outside it: it takes _mutex, and acquiring that under _heartbeat_mutex would invert the _mutex -> _heartbeat_mutex order. Connectivity is meant to be evaluated live per tick anyway. _always_send_heartbeats and _heartbeat_watchdog_timeout_s are now only accessed under _heartbeat_mutex, so they no longer need to be atomic. Move them next to the watchdog state they belong with.
Left over from an earlier heartbeat watchdog design that rode on TimeoutHandler. Every caller discards the value, so restore the original void signature.
The out-of-line definition was added to fix a TSan report about packing injected heartbeats, but MAVLINK_HELPER resolves to 'static inline' here (MAVLINK_SEPARATE_HELPERS is not defined), so mavlink_get_channel_status()'s status array is per translation unit. The library and the test binary cannot share it, which means one shared channel allocator cannot have been what fixed the race. Restore the original and let CI show whether TSan still complains, so the actual racing pair can be found.
MavsdkImpl owned a unique_ptr<Time> plus a public Time& alias, needed a
private section wedged between public ones so the two were declared in the
right order, and carried a default argument constructing a Time in the
caller's module. All of that existed only so the heartbeat watchdog tests
could inject a FakeTime, and it also forced MAVSDK_TEST_EXPORT onto the
whole MavsdkImpl class.
Restore the plain `Time time{}` member and the single-argument constructor,
and drop MAVSDK_TEST_EXPORT: with no FakeTime to inject, the tests no longer
need MavsdkImpl at all and now drive the public Mavsdk API instead. They
count heartbeats via subscribe_raw_bytes_to_be_sent() rather than the
deprecated intercept_outgoing_messages_async(), which -Werror rejects.
The cost is real-time waits: the heartbeat tests go from 8.6 s to 49 s.
All watchdog timeouts are now 2 s rather than the 1 s minimum, because at
1 s a single feed and the 1 Hz tick can be nearly in phase, making
"does the next heartbeat still make the deadline" a coin flip. That is
genuine behaviour, not something the tests should race against.
…keTime
The watchdog state machine lived inline in MavsdkImpl, whose Time is an owned
member. That left no way to drive it with a FakeTime other than injecting the
Time into MavsdkImpl, which meant real-time tests once that injection was
reverted: 49 s for the heartbeat tests alone.
Move the state machine into a small HeartbeatWatchdog class that takes a
Time&, exactly like TimeoutHandler and CallEveryHandler. The unit test then
follows the same pattern as timeout_handler_test.cpp and
call_every_handler_test.cpp - '#define Time FakeTime' and construct one - and
covers all the transitions in 0 ms instead of 49 s.
MavsdkImpl keeps its plain 'Time time{}' member and needs no export; only
the new leaf class carries MAVSDK_TEST_EXPORT. The class also owns the
"timeout actually changed" check that set_configuration_locked() open-coded,
and is now the single source of truth for the validation rule, which
Mavsdk::is_valid_heartbeat_watchdog_timeout_s() delegates to.
The wiring that a unit test cannot reach - that the 1 Hz tick consults the
watchdog, and that concurrent reconfiguration during discovery does not
deadlock - moves to src/system_tests/heartbeat_watchdog.cpp, where real-time
waits belong. Unit suite is back to 4.6 s; the three system tests add 10.6 s.
Covers what the watchdog is for, the C++ usage, and how often to feed: MAVSDK sends heartbeats at 1 Hz and checks the deadline when one is due, so feeding exactly once per timeout period leaves no phase margin and a single late feed drops a heartbeat. At the 1 s minimum that is a coin flip, which is why the tests all use 2 s. Kept out of general_usage.md as an advanced topic, and the mavsdk_server option and RPCs are behind a note that they only apply to wrappers driving MAVSDK through mavsdk_server over gRPC.
… from c/
The watchdog is only reachable from gRPC-based language wrappers, which drive
MAVSDK through mavsdk_server, so the C wrapper over libmavsdk does not need
it: revert the three additions to c/src/cmavsdk/. Nothing in mavsdk_server
went through them anyway - it links libmavsdk and calls the C++ API directly.
Replace mavsdk_server_run_with_mavlink_ids_and_options() with
mavsdk_server_set_heartbeat_watchdog_timeout(server, timeout_s)
called before run(). Threading options through the run() call meant a new
exported symbol per option, and it forced a 3-argument setMavlinkIds()
overload carrying a heartbeat timeout behind a name about MAVLink IDs; both
are gone. setMavlinkIds() replaces the whole configuration, so it now carries
over a previously set timeout, which makes the two setters independent of
call order. Verified end to end: with --heartbeat-watchdog-timeout 2 and no
feeding, a udpout peer sees 0 heartbeats, against 5 in the same window
without the flag or with an explicit 0.
The setter validates by using the bool that Mavsdk::set_heartbeat_watchdog_
timeout_s() already returns, and SetHeartbeatWatchdogTimeout does the same
instead of pre-validating. That leaves no caller outside libmavsdk for
Mavsdk::is_valid_heartbeat_watchdog_timeout_s() or
Mavsdk::heartbeat_watchdog_min_timeout_s(), which were only ever added to
serve mavsdk_server and the CLI, so remove them from the public API;
HeartbeatWatchdog::is_valid_timeout_s() is the source of truth. mavsdk_server_
bin.cpp no longer needs to include mavsdk.hpp and is a pure C-API consumer
again. API reference regenerated with doxygen 1.9.8.
…to at upstream MAVSDK-Proto#415 is merged upstream, so restore the mavlink/MAVSDK-Proto URL in .gitmodules. The submodule already pins 1a17660, which is now the upstream tip, so the pinned commit is unchanged. Raise HeartbeatWatchdog::min_timeout_s from 1.0 to 2.0. Heartbeats are sent at 1 Hz and the deadline is only checked when one is due, so at a 1 s timeout a feed and the tick can land almost in phase and a single slightly late feed drops a heartbeat. That showed up concretely while porting the tests: a case using a 1 s timeout with one feed failed 2 of 3 runs purely on phase, and every test had to move off 1 s. A minimum the documentation tells you not to use is a trap, so remove it rather than document around it. Also drop the std::atomic from FakeTime, which was only needed while MavsdkImpl held a FakeTime shared with the io thread. The remaining FakeTime users are single-threaded, so mavsdk_time.hpp is back to matching main and the .cpp keeps only the llround fix for the truncating millisecond cast. Note: core.proto still documents "Minimum 1 when enabled" and needs a small follow-up PR. No Result message is needed there - core.proto deliberately uses none (SetMavlinkTimeout returns an empty response too), unlike the plugin protos, so reporting the rejection as INVALID_ARGUMENT stays.
Picks up MAVSDK-Proto#421, so the documented minimum in core.proto matches the value HeartbeatWatchdog enforces. Comment-only upstream, so no generated code changes.
Complement the expiry warning so operators can see heartbeats resume when the watchdog transitions from NeedsFeed back to Armed. (cherry picked from commit eb4b6e4; applied in HeartbeatWatchdog::feed(), where this state machine now lives.)
Collaborator
Author
|
@tpayne-censystech I did an iteration with Claude on this. Are you ok with this? Happy to force push to your branch as well to keep your PR authorship. |
I'd prefer to keep the PR authorship if you don't mind. |
Collaborator
Author
|
Force-pushed to existing PR, closing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Alternative to #2895, which this builds on the same feature but is reworked after review by Claude. @tpayne-censystech's commits are preserved.
Includes mavlink/MAVSDK-Proto#421.
Original description
When enabled, this disables MAVSDK's internally generated HEARTBEAT sending and drives emission from the client via a runtime-configurable deadman timeout. The client refreshes within the timeout to keep heartbeats flowing; if it crashes, disconnects, or stops servicing the path, MAVSDK stops sending, so PX4 sees the GCS/companion component go offline and triggers the configured lost-link failsafe (e.g. RTL). MAVSDK continues to own heartbeat content (sysid, compid, mav_type, etc.); the client only controls whether emission continues. Default behavior is unchanged.
API:
For gRPC wrappers:
mavsdk_server --heartbeat-watchdog-timeout 2, or theSetHeartbeatWatchdogTimeout/FeedHeartbeatWatchdogRPCs.What changed relative to #2895
Refactored the heartbeat function to one call_every loop which then checks all conditions before sending (or not) the heartbeat. I think this is cleaner than starting and stopping that loop based on two conditions.
The watchdog is its own class. for easier testing.
Minimum timeout is 2 s, not 1 s. to avoid aliasing.
Fixed a mutex issue that could leak a heartbeat.
C API and mavsdk_server surface. The watchdog is only reachable from gRPC wrappers, so the additions to
c/(the C wrapper over libmavsdk) are dropped.Reverted unused bool the
boolreturn added toTimeoutHandler::refresh()**Reverted the instance stuff for
MavlinkChannels. I don't understand why that would be required.Docs
New guide page at `docs/en/cpp/guide/heartbeat_watchdog.md