Repository navigation
fix(motors): disable torque for every serial motor on disconnect - #4695
Hadrien-Cornier wants to merge 3 commits into
Conversation
mmporong
left a comment
There was a problem hiding this comment.
I checked this locally at 515ec97 (Windows 11, Python 3.12).
What I reproduced:
-
tests/motorson this branch: 38 passed.test_feetech,test_dynamixelandtest_damiaowere skipped here for missing optional deps (mock_serialisn't installed on Windows). -
With
motors_bus.pyfrom the merge base (d20a453), the four new cleanup tests fail and thedisable_torque=Falsetest passes, as the description says. -
The new tests patch out
disable_torque, so I also ran the realFeetechMotorsBus.disconnect()with a fakePortHandler, three motors (ids 1-3), andwriteTxRxreturning status0x20(overload) forTorque_Enableon id 2. Writes as(id, addr):- merge base:
[(1, 40), (1, 55), (2, 40)], port left open (is_connectedis stillTrue). - this PR:
[(1, 40), (1, 55), (2, 40), (3, 40), (3, 55)],closePort()called once, the sameRuntimeErrorre-raised, onelogger.errorline for that motor.
So the per-motor loop still writes the Feetech
Torque_Enable+Lockpair for every other motor. - merge base:
One suggestion, docs only: the new paragraph sits after Args:, but docs/source/writing_docstrings.mdx puts the free-form description before the sections. The re-raise could also go in a Raises: section, like connect() in the same file:
"""Close the serial port, optionally disabling torque on each motor first.
If disabling torque fails for one motor, the remaining motors are still attempted and the port is
still closed.
Args:
disable_torque (bool, optional): ...
Raises:
ConnectionError: A motor did not answer while torque was being disabled. The first failure is
re-raised after the port is closed.
RuntimeError: A motor reported an error while torque was being disabled. Same as above.
"""utils/check_docstrings.py passes on this branch either way, so this isn't blocking.
varun-projects
left a comment
There was a problem hiding this comment.
Took a look at this while working on an unrelated motors change, so I had the protocol SDKs set up. Three things that might be useful, plus one question.
It still fixes a live defect on current main. The branch is 54 commits behind main now, so I copied your tests/motors/test_motors_bus.py onto upstream/main at b9cb121c and ran it against the unmodified motors_bus.py:
FAILED test_disconnect_disables_torque_on_every_motor_before_closing
FAILED test_disconnect_continues_after_motor_failure_and_reraises[RuntimeError]
FAILED test_disconnect_continues_after_motor_failure_and_reraises[ConnectionError]
FAILED test_disconnect_logs_all_motor_failures_and_reraises_first
4 failed, 1 passed
So the behaviour you're fixing is still there; this just needs a rebase rather than a rethink.
Full motors suite on Linux with all protocol SDKs installed. @mmporong's Windows run had test_feetech, test_dynamixel and test_damiao skipped for missing mock_serial, so this may be a useful complement — uv sync --extra test --extra hardware --extra feetech --extra dynamixel --extra damiao, then:
$ uv run pytest tests/motors -q
114 passed, 3 skipped
against 109 passed, 3 skipped on main, i.e. your 5 new tests and nothing else moved.
One thing I'd flag: if closePort() raises, the motor failure is discarded entirely. The PR description says closure "takes precedence", which is a reasonable choice, but as written the motor exception isn't chained either — it's a local that never gets raised, and because the per-motor errors were caught rather than propagating, Python attaches no context. I replicated the control flow to check:
close_raises=False: raised RuntimeError: m1: overload
__context__ = None
__cause__ = None
close_raises=True: raised OSError: closePort failed
__context__ = None
__cause__ = None
So on a bus where a motor is overloaded and the port is wedged — plausibly the same underlying fault — the caller sees only the closePort error. The logger.error line still records the motor, so it isn't invisible, but it's gone from the traceback. If you want to keep closure taking precedence, chaining costs little:
finally:
try:
self.port_handler.closePort()
except Exception as close_error:
if first_exception is not None:
raise close_error from first_exception
raiseMinor: logger.debug(f"{self.__class__.__name__} disconnected.") sits before the raise, so the failure path logs "disconnected" and then raises. Harmless, since the port really is closed by then, but it reads oddly in a log.
One thing I checked that is not a problem, in case it comes up in review: moving from one disable_torque(num_retry=5) call to a per-motor loop doesn't multiply the retry budget. Both FeetechMotorsBus.disable_torque and DynamixelMotorsBus.disable_torque already iterate _get_motors_list(motors) and pass num_retry to each write, so the worst-case number of writes on a dead bus is the same as before.
Not a maintainer, so treat this as a community review rather than a sign-off.
515ec97 to
50b458d
Compare
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
|
Thanks for the feedback! Rebased onto main and addressed both reviews in f8d688a. If port closure also fails, its exception stays primary and the first motor error is now chained as the cause. I kept the cleanup-success log but clarified it to “serial port closed,” and updated the docstring ordering and Raises section. Added regression tests for motor-only, close-only and combined failures. One clarification: retries remain six per write, but attempting cleanup on three unresponsive motors can mean 18 attempts total. That’s intentional so one motor failure doesn’t skip the others. Full suite: 3,723 passed, 358 skipped. Lint, docs and CI are green. |
fix(motors): disable torque for every serial motor on disconnect
Summary / Motivation
If one motor fails during
SerialMotorsBus.disconnect(), shutdown currently skips later motors and serial-port closure. This change attempts torque disable on every configured motor, always attempts port closure, then re-raises the first motor error.This example assumes port closure succeeds. If closure raises, that exception takes precedence, with the original first motor exception chained as its cause. Closure is attempted, not guaranteed.
Related issues
What changed
disable_torque(motor, num_retry=5)for each configured serial motor, logging failures and continuing cleanup.finally, then re-raise the first motor exception if closure succeeds. If closure also fails, retain the close exception as primary and chain the first motor exception as its cause.serial port closedbefore re-raising a motor exception.Args, addRaises, and describe close failures and interrupts.disable_torque=Falseand ordinary_write()behavior.The shared serial implementation covers Feetech and Dynamixel. CAN buses are unchanged. No public API or configuration change is intended.
How was this tested (or how to run locally)
Review follow-up, 2026-10-10
Rebased on upstream
mainatb9cb121c. The PR head isf8d688af.clearPortfailure, and interrupts. Assertions check all-motor continuation, closure order, exception identity and cause, and logs.--log-level=DEBUG.make check-doctest-list check-docstrings doctestpassed with hardware and CUDA examples skipped; 3 doctests passed. Optional Unitree imports were skipped. Docstring coverage: 54.5%, required 52.0%.mainreproduced 4 failed, 1 passed. The new chaining and log assertions failed on the original PR and pass after the change.OSErrorwith the motorRuntimeErroras its cause. No physical robot hardware operated in this update. An independent code review found no blocking issue.Exact-commit CI: Quality, Fast Tests, Documentation, Security. Status: Quality: success; Fast Tests: success; Documentation: success; Security: success.
Original validation at
515ec97dThe results below refer to the original PR head, including Hadrien's physical validation.
Focused shutdown suite:
uv run --extra test --extra hardware pytest -q tests/motors/test_motors_bus.pypassed: 38 passed.Entire motor suite:
uv run --no-sync pytest -q tests/motorspassed: 81 passed, 2 skipped. Physical serial port environment variables were set to nonexistent device paths for automated testing.All configured lint, formatting, security, and typing hooks passed with
PATH=/opt/homebrew/bin:$PATH uv run --extra dev pre-commit run --all-files. No hooks were skipped and no tracked files were changed. Selecting Homebrew npm 11.17.0 resolved the earlier npm 11.12.1 bootstrap error.UV_NO_SYNC=1 make check-doctest-list check-docstringspassed; two optional Unitree modules were skipped due to unavailable dependencies.Documentation examples passed (
UV_NO_SYNC=1 SKIP_CUDA_DOCTEST=1 make doctest: 3 passed). Docstring coverage passed (uv run --no-sync --with interrogate interrogate --config=pyproject.toml: 52.6%, required 52.0%).Full suite on macOS arm64, Python 3.13.9: 2,834 passed, 316 skipped, 34 warnings in 634.03 seconds, exit code 0. Optional-dependency and platform skips remain; this is not a claim that every optional hardware or GPU configuration was tested.
The local setup required Git LFS fixtures (
git lfs pull --include='tests/artifacts/**'), the dataset extra, and FFmpeg 8 shared libraries. The first run without the dataset extra had 1,569 passes, 351 skips, six failures, and one setup error due to missingdatasets; the complete run above resolves those failures. No source changes were needed to fix the validation environment.The focused tests cover success, a middle motor raising
RuntimeErrororConnectionError, multiple failures, port closure, original exception identity, anddisable_torque=False. Against unchanged main, four new cleanup cases fail; the compatibility case passes.Physical SO-101 Feetech validation:
Torque_Enable, communication success, servo status0x20(Overload error). The patched disconnect logged the error, closed the port, and re-raised it.The gripper is last in the configured order, so the natural failure demonstrates port cleanup; the controlled elbow failure demonstrates continuation to later motors. The underlying overload cause is not established or fixed by this PR. Physical Dynamixel hardware was not tested.
Checklist (required before merge)
pre-commit run -a)pytest), with optional/platform skips reported aboveReviewer notes
Shutdown performs best-effort cleanup: one motor failure should not prevent attempts on later motors or port closure. Every motor error is logged, and the first is re-raised rather than introducing an
ExceptionGroup. A port-closure exception takes precedence and retains the first motor exception as its cause. If closure succeeds, the first motor exception is re-raised unchanged.The retry budget remains six attempts per register write. With three fully unresponsive motors, fake SDK I/O confirms six attempts on unchanged main and eighteen on this PR, because cleanup reaches each motor. That additional work is intentional and already present in the original PR. This review follow-up adds no retries.
Device-error retries, torque-register readback, delays, new configuration, and CAN behavior are outside this change. The existing device-error fast failure in
_write()is preserved.AI assistance was used for repository research, implementation drafting, and test design. I reviewed and understand the contribution and validated it with the software and physical checks described above.