fix: tolerant arrival detection in move_to_target#489
Conversation
A single speed == 0 sample during travel currently exits the move loop. This can happen mid-travel when the controller momentarily decelerates between reference input writes, or when the speed reading races a write. Over BLE proxies the variance is even larger because ATT round-trips can spike to hundreds of milliseconds, so transient zero-speed readings are common. Tighten arrival detection to require multiple consecutive ``speed == 0`` readings together with the height being within a small tolerance of the target. If the desk stalls away from the target, allow a bounded number of stall recoveries by re-sending the reference input rather than exiting immediately. Reduce the reference input write interval from 200ms to 100ms so the controller is less likely to decelerate between writes. The existing test_move_abort_when_no_movement is preserved by the stall-retry guard: after _MOVE_MAX_STALL_RETRIES runs of consecutive zero-speed readings without progress, the loop exits.
|
Thanks for opening the PR! This may take me a week to review, I'm traveling right now. I'll review when I get home, or if I can find a stable WiFi connection. |
| # Stalled away from the target. Allow a bounded | ||
| # number of stall recoveries before giving up; this | ||
| # lets the loop ride out transient pauses while still | ||
| # exiting if the desk is physically stuck. | ||
| stall_retries += 1 |
There was a problem hiding this comment.
This doesn't reset after a stall condition is cleared, it's one retry counter for the entire movement. Should this get reset to zero when a stall condition is cleared?
I'll trust your judgement either way. I haven't been able to reproduce this stall at home, I need to get some ESPHome relays to reproduce for the future.
There was a problem hiding this comment.
Good catch @newAM - you're right that this should reset. I've seen the DPG1C decelerate to a brief stop several times during a single long move (most visibly through ESPHome Bluetooth proxies where ATT latency varies), so a lifetime cap of 3 would prematurely abort otherwise-healthy moves.
Pushed a809bf7 that resets stall_retries = 0 whenever a non-zero speed is observed (forward progress clears the stall budget), and added test_move_to_target_recovers_from_multiple_stalls that exercises multiple stall-recover-stall cycles within one move.
A long move that pauses multiple times along the way (e.g. the DPG1C controller briefly decelerating between reference input writes over a Bluetooth proxy) would otherwise exhaust the stall budget and abort before reaching the target. Reset `stall_retries` whenever a non-zero speed is observed, so the budget only tracks consecutive stall attempts without intervening movement. This matches the intent: bail out when the desk is physically stuck, not when it has paused several times in an otherwise-healthy move.
|
Thanks for the fix! This is included in the v0.13.0 release. |
Summary
Make
move_to_targetresilient to transientspeed == 0readings during travel by:speed == 0readings before treating the desk as stoppedWhy
The current loop exits as soon as
get_speed()returns 0:That works most of the time over a direct BLE connection, but in practice the desk controller can briefly read
speed == 0mid-travel — for example when it momentarily decelerates between reference input writes, or when the speed read races a write. Exiting on the first zero stops movement early and leaves the desk short of its target.The effect is much worse over Bluetooth proxies (e.g. ESPHome Bluetooth proxies used by the Home Assistant
idasen_deskintegration), where ATT round-trips can spike to hundreds of milliseconds, transientspeed == 0samples become common, and the 200ms write interval can starve the controller of reference input and cause it to physically stop and restart several times per move.Changes
In
move_to_target:get_height_and_speed()instead ofget_speed()so both values are available_MOVE_CONSECUTIVE_ZERO_SPEED = 2consecutivespeed == 0readings before considering stopping_MOVE_HEIGHT_TOLERANCE = 0.005m of the target; otherwise increment a stall counter and continue_MOVE_MAX_STALL_RETRIES = 3stalls, give up to preserve the existing "abort when the desk does not move" behavior_MOVE_LOOP_INTERVAL = 0.1sAll new constants are module-level so they are easy to find and tune.
Tests
test_move_to_target,test_move_abort_when_no_movement,test_move_stop)test_move_to_target_already_at_target_with_tolerancecovers the tolerance path on the initial height checktest_move_to_target_tolerates_transient_zero_speedexercises a transientspeed == 0mid-travel followed by recoverytest_move_to_target_stalled_away_from_target_abortscovers the stall-retry guard ensuring the loop exits when the desk never reaches the targetLocal CI checks:
Notes
move_to_targetsee the same interface and the sameis_movingsemantics.idasen_deskintegration matches this pattern: see home-assistant/core#155912 and abmantis/idasen-ha#38 for downstream context.