fix(unitree): stale movement watchdog can no longer stop a newer command - #3243
Open
cowsking wants to merge 1 commit into
Open
fix(unitree): stale movement watchdog can no longer stop a newer command#3243cowsking wants to merge 1 commit into
cowsking wants to merge 1 commit into
Conversation
The auto-stop watchdog ran on a threading.Timer thread. Timer.cancel() cannot stop a callback that has already started, so an expired watchdog from the previous move() could publish a zero twist after a newer command was sent, halting it. The stale stop_movement() also cancelled the newer command's freshly armed timer, killing its watchdog. Move the watchdog onto the WebRTC connection event loop (loop.call_later): movement publishes, watchdog arm/cancel and expiry now all run on the loop thread, so they are strictly ordered and a cancelled deadline can never fire after a replacement command. Duration moves re-arm the deadline on every publish, so a duration longer than cmd_vel_timeout is no longer interrupted mid-stream by the watchdog. Closes dimensionalOS#3046 Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01PcuXf2py6b6T28XfayWLMv
cowsking
requested review from
Dreamsorcerer,
leshy,
mustafab0,
paul-nechifor and
spomichter
as code owners
July 28, 2026 06:01
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
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.
Contribution path
Problem
The auto-stop watchdog runs on a
threading.Timerthread.Timer.cancel()has no effect once the callback has started, so an expired watchdog from a previousmove()can publish its zero twist after a newer command and halt it. The stalestop_movement()also cancelsself.stop_timer, which by then points at the newer command's timer, so the newer command loses its own watchdog too. The same root cause means a duration move longer thancmd_vel_timeoutgets a zero twist injected in the middle of its command stream.Solution
Move the watchdog onto the connection event loop, as suggested in the issue.
move()now publishes and re-arms the deadline inside the same coroutine. Expiry and cancellation run on the loop as well. Since the loop serializes publish, re-arm, cancel and expiry, a cancelled deadline can never fire after a replacement command. No locks or generation counter needed. Duration moves re-arm on every publish, so the watchdog no longer interrupts them mid-stream.Tests
Four new tests in
test_connection.py, reusing the existing stubbed driver fixture (no hardware, real event loop). Assertions are made on the recorded wire messages, not internal state.test_stale_watchdog_expiry_does_not_stop_newer_command: the regression test requested in the issue. The first expiry callback is gated with athreading.Event, so the test holds it in flight while a replacementmove()is issued from another thread. After releasing the gate, the newer command must be the last message on the wire (the stale zero may only land before it), and the replacement's own watchdog must still fire afterwards.test_watchdog_zeroes_movement_after_timeout: baseline. With no follow-up command, the watchdog publishes a zero twist aftercmd_vel_timeout.test_stop_movement_disarms_watchdog: an explicit stop publishes exactly one zero twist and leaves no armed deadline. The deadline check runs on the loop thread, where the handle is owned.test_duration_move_is_not_zeroed_mid_stream: a 0.3s duration move with a 0.1s timeout publishes only movement commands until the final stop. This test fails on current main because the old watchdog fires mid-stream.How to Test
uv run pytest dimos/robot/unitree/test_connection.pyAI assistance
Claude Code (Fable 5) assisted parts of the implementation and tests. Reviewed and understood line by line.
Checklist