Modular multi-motor Manager

[sakshishirole01]

Summary

Describe the main changes in this PR:

---

Code Quality Checklist

Before requesting review, ensure:

• All CI/CD pipelines pass
• Code includes docstrings and type hints
• I added new tests where necessary
• I ran make tree to update the README.md

---

Additional Notes

• Anything else reviewers should know?
• Attach outputs, plots, logs, or GIFs here.

[NGierden]

@claude-review

[claude]

Claude finished @NGierden's task in 1m 47s —— View job

---

Code Review: Modular multi-motor Manager

Tasks

• Gather context and understand the PR
• Read changed files
• Analyze code quality, correctness, and design
• Post review feedback

---

Summary

This PR introduces MotorManager — a clean, label-aware multi-motor coordinator for the CAN interface. The design is solid overall, but there are a few correctness and safety issues worth addressing before merge.

---

motor_manager.py

Issues

1. Motors not stopped before close() in context manager exit

__exit__ calls only self.close(). If a motor is still running when the context block exits (e.g. after KeyboardInterrupt), close() releases the CAN interface without stopping the motor first. This is a safety concern for an exosuit actuator.

# motor_manager.py:161-168
def __exit__(self, ...) -> None:
    self.close()  # ← motors may still be spinning

The BaseMotor.__exit__ pattern likely calls stop() before close(). MotorManager should do the same — call stop_all() (suppressing errors) before delegating to close(). Fix this →

2. _closed is set to True before the ExceptionGroup is raised

# motor_manager.py:153-155
self._closed = True
if errors:
    raise ExceptionGroup("MotorManager.close failed", errors)

If close() raises, a subsequent call silently returns True without retrying the failed motors. This is arguably intentional (idempotent), but it means partial failure is unrecoverable from. At minimum, document this in the docstring.

3. discover() hardcodes the scan range

# motor_manager.py:85
for motor_id in range(1, 17):  # scan CAN IDs 0x01 to 0x10 (1-16)

The range 1–16 is a magic number that should either be a named constant in definitions.py or an optional parameter of discover(). Fix this →

4. discover() scans then re-creates connections (double open)

Each CubeMarsAK606v3CAN instance is opened in a with block just to check communication, then closed. After the scan, cls(motor_ids=discovered_ids, ...) re-creates all of them. Every discovered motor is opened twice. This adds latency and may cause issues if the CAN setup is slow. The current approach works but is worth noting.

5. No thread safety

_closed and _motors are accessed without locks. This is fine for single-threaded use, but if the motor manager is ever accessed from a control thread while another thread calls close(), there will be a race. Not a blocker, but worth a comment in the class docstring.

---

examples_can.py

6. Duplicate imports

# examples_can.py:8-14
from motor_python.definitions import MOTOR_LIMITS, TendonAction      # ← line 8
from motor_python.motor_manager import MotorManager
from motor_python.definitions import (                                # ← line 10 (duplicate)
    CAN_DEFAULTS,
    MOTOR_LIMITS,
    TendonAction,
)

MOTOR_LIMITS and TendonAction are imported twice. Merge into a single import block. Fix this →

7. multi_motor_can_example() doesn't use MotorManager

# examples_can.py:302-303
motor_left = CubeMarsAK606v3CAN(motor_can_id=left_can_id, interface="can0")
motor_right = CubeMarsAK606v3CAN(motor_can_id=right_can_id, interface="can0")

This standalone function manually manages two motors with a bare try/finally. Now that MotorManager exists, this is the obvious candidate for refactoring (or at least using with statements per motor). As a standalone example it still works, but it's inconsistent with the module's new idioms.

---

__main__.py

The refactoring is clean. MotorManager is used correctly throughout. The --discover flag wires up MotorManager.discover() nicely.

Minor note: the manager variable is created before the with manager: block, so if initialization raises (e.g. from MotorManager.__init__) the except Exception block at line 60 catches it — but the finally cleanup from the context manager is bypassed since manager may not have been fully initialized. The current code handles this correctly by catching in the try block, but it's worth ensuring no CAN resources are leaked if the constructor fails partway through.

---

Missing Tests

There are no tests for MotorManager. The PR checklist box for tests is unchecked. Given the safety-critical nature of this code, the following scenarios should have unit tests (with mocked CubeMarsAK606v3CAN):

• __getitem__ by ID, by label, unknown key → KeyError
• __contains__ by ID and label
• Duplicate IDs → ValueError
• Label referencing unknown ID → ValueError
• stop_all() / close() collect all errors into ExceptionGroup
• close() is idempotent (second call is a no-op)

---

Minor Nits

• __main__.py:21: # noqa: PLR0913 is present but the function signature is 6 params — fine, just verify this is still needed after the refactor.
• The PR description is completely empty. Please fill in the Summary section so reviewers have context.

---

Overall: The MotorManager design is well-structured — the dunder protocol, label-based access, error-collecting stop_all/close, and discover() classmethod are all good patterns. The main asks before merge are: (1) stop motors before closing in __exit__, (2) add tests, (3) fix the duplicate import.