core: raise heartbeat watchdog minimum timeout to 2 s - #421
Merged
Conversation
MAVSDK sends its periodic heartbeats at 1 Hz and only checks the watchdog deadline when a heartbeat is due. With a 1 s timeout a feed and the tick can land almost in phase, so a single slightly late feed already drops a heartbeat, leaving a 2 s gap on the wire. A minimum that cannot be used as documented is a trap, so raise it to two heartbeat periods. Also state the feeding frequency explicitly: feed at least twice per timeout period, not exactly once. "At least once per timeout period" reads as sufficient but leaves no margin for jitter. Implementation side: mavlink/MAVSDK#2895.
Collaborator
Author
|
@tpayne-censystech does this make sense? When we have the watchdog at 1 Hz, it can get out of sync and "just miss it". |
Contributor
|
@julianoes yeah, good catch. It's effectively an aliasing issue. The heartbeat watchdog timeout should be at least double the heartbeat period to prevent "aliasing". |
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.
Follow-up to #415, which added the heartbeat watchdog RPCs.
MAVSDK sends its periodic heartbeats at 1 Hz and only evaluates the watchdog deadline when a heartbeat is due. With a 1 s timeout, a feed and the tick can end up almost in phase, so a single slightly late feed already drops a heartbeat and leaves a 2 s gap on the wire.
This showed up concretely while reviewing the implementation side: a test using a 1 s timeout with a single feed failed 2 of 3 runs purely on feed/tick phase, and every test had to be moved off 1 s to be stable. A documented minimum that cannot actually be used is a trap, so raise it to two heartbeat periods.
Comment-only change, so no generated code changes.
prototool lint protospasses with the same image CI uses.Changes:
SetHeartbeatWatchdogTimeout: values below 2 are rejected, with the reason.SetHeartbeatWatchdogTimeoutRequest.timeout_s: minimum 2 rather than 1.FeedHeartbeatWatchdog: say to feed at least twice per timeout period. "At least once per timeout period" reads as sufficient but leaves no margin for jitter.The implementation change is in mavlink/MAVSDK#2895, which pins this repo as a submodule — that PR needs this one merged first so it can bump the submodule to a commit whose documented minimum matches the enforced one.