[nicolas-rabault]

A gap in #142 and #143, found in a review on the LeRobot side.

The problem

Bus::set_torque, change_id, change_baudrate and the torque off and lock opening ahead of the last two write through write_register, which reads the motor's answer and drops its error field. A motor that refuses a write still answers: a Dynamixel asked to change its id with the torque on reports an access error, a value out of range a range error. The call then reports success, and in set_torque the motor is not listed as failed.

What changes

• ServoDefinition::refuses(status) tells a refusal from an alarm: an error number on protocol v2 (the alert bit alone is a latched hardware fault), an instruction, checksum or range error on protocol v1, and never on a servo whose status byte only reports alarms. The new ServoInfo.status_alarms_only says so for the Feetech servos (STS3215, SCS0009, SCS0043), whose status bits are input voltage, angle, overheating, over-current and overload, as in Feetech's SDK. Python reads it as ServoDefinition.status_alarms_only.
• These writes fail when the motor refuses them, with a new StatusPacketError carrying the motor's id and its StatusError (RuntimeError on Python, with "motor 3 refused the instruction with status error 0x07"). An alarm does not fail them: a Feetech lock still follows the torque of an overloaded motor, and a motor in an alarm can still be given a new id or baud rate.
• set_torque lists a refusing motor with the ones that failed, and goes on with the next one.
• Retrying would get the same answer, so retrying leaves a StatusPacketError alone, as it does a RegisterError.

Tests

• refuses on an STS3215 (0x08 and 0x20 are alarms), an AX (instruction, checksum and range errors refuse, voltage and overload do not) and an XL330 (an error number refuses, the alert bit alone does not).
• set_torque on an STS3215 answering both writes with an overload alarm: nothing fails, and the lock follows the torque.
• set_torque on an XL330 refusing the torque write with an access error: listed as failed with a StatusPacketError, written once with retries = 2, and the next motor still written.
• change_id: an STS3215 in an over-current alarm takes its new id; an XL330 refusing the id write fails with StatusPacketError { id: 9, .. }.
• cargo test (96 + 11), clippy with no warning, and the Python build. Not run on hardware: no motor here refuses these writes on demand.

The other half of that review, typed errors for a timeout and for a port that is gone, is in #151 as a question, since nothing uses it yet; a Python exception class for StatusPacketError belongs there too.

[nicolas-rabault]

Checked on hardware that healthy motors are not reported as failed now: two SO-101 arms, six STS3215 each (firmware 3.10).

• 120 register reads across both arms: every status byte is 0.
• set_torque([1..6], False), three times per arm: {} every time, in 4.5 to 4.8 ms.
• set_torque([1, 2, 3, 20, 4, 5, 6], False, retries=2) with id 20 absent: only {20: 'Operation timed out'}, the six others written.
• change_id(STS3215, 6, 6) and change_baudrate(STS3215, 6, 1_000_000) (the values already there): no MotorError; torque and lock read 0 afterwards.

I could not make a motor refuse a write on demand without moving the arm, so the MotorError path itself stays covered by the unit tests only.

[pierre-rouanet]

The code is clean and tests pass (cargo test 89 + 11 green, clippy count unchanged from develop, test packet checksums correct). One thing to change before merging, though: counting every non-zero status byte as a failure brings in a new partial-state bug.

"Any error bit means the write failed"

On Feetech and Dynamixel v1, the status byte also carries ongoing conditions: voltage 0x01, overheating 0x04, overload 0x20. On v2, Alert (0x80) stays set for as long as a hardware fault is latched. None of these mean the write was refused. Two problems follow:

1. set_torque now leaves the lock out of step with the torque (src/bus.rs, the retrying closure). When the torque write comes back with an error, ? returns early and the lock write never happens. Before this PR both writes went through.

   • Feetech motor in an overload alarm, set_torque(True): torque on, lock left open, so the EEPROM is unprotected.
   • Same motor, set_torque(False): torque off, lock stays closed.

   The new test uses exactly this case (0x20 = overload) and asserts the lock is left alone, so it locks the problem in rather than catching it.

2. change_id / change_baudrate refuse any motor with a latched alarm. The torque-off write in open_eeprom now fails, so a motor reporting low voltage, or a v2 motor with Alert set, can't be given a new id or baud rate, which is arguably when you need it most.

I get the LeRobot-parity argument, but I think it's the wrong default for the library. Suggestions:

• Minimum: in set_torque, always do the lock write, then report the first error, so the lock always follows the torque.

• Better: fail only on bits that mean the instruction was refused, per protocol:

  • v2: v2_instruction_error() != 0, ignoring Alert;
  • Dynamixel v1: instruction (0x40), range (0x08), checksum (0x10);
  • Feetech: probably nothing, since as far as I know its status byte only reports alarms (worth checking).

  That keeps the useful case this PR is after: a Dynamixel refusing an id change with an access or range error.

Minor

• Python callers only get a message string, so they can't read the id or status byte without parsing it. Fine for now; a dedicated exception class would fit with the typed-error question in #151.
• MotorError is broad for what is a status-packet error; something like StatusPacketError might age better. Not a blocker.

[nicolas-rabault]

@pierre-rouanet thanks, you were right: a status byte is not a verdict on the write. Changed in 53da259:

• Only a refusal fails now. ServoDefinition::refuses reads an error number on v2, ignoring the alert bit, an instruction, checksum or range error on Dynamixel v1, and nothing on Feetech, whose status byte only carries alarms. I checked the Feetech bits in their SDK (ERRBIT_VOLTAGE 0x01, ERRBIT_ANGLE 0x02, ERRBIT_OVERHEAT 0x04, ERRBIT_OVERELE 0x08, ERRBIT_OVERLOAD 0x20): 0x08 is an over-current there, not a range error, so the protocol alone cannot tell. The definitions say it instead, with status_alarms_only on the STS3215, SCS0009 and SCS0043.
• The lock now follows the torque of a Feetech motor in an alarm, and a motor in an alarm can be given a new id or baud rate. The test you pointed at asserts the opposite of what it did: an overload alarm, both writes made, the lock included. Since a Feetech never refuses and a Dynamixel has no such lock, the lock only falls out of step when the bus itself fails, so I did not add the minimum fix on top.
• MotorError is now StatusPacketError. The Python exception class goes with Typed errors: a motor that does not answer vs a serial port that is gone #151.

I also merged develop (fd4a455) for the Changelog, now under Unreleased since 1.12.0 is out. On the LeRobot side, read, write and sync read follow the same rule (huggingface/lerobot#4672).

[pierre-rouanet]

Thanks, this addresses both points: an alarm no longer fails the write, the lock follows the torque, and a motor with a latched alarm can still be given a new id or baud rate. The Feetech SDK check settles the 0x08 question. On the PR head, tests pass (96 + 11) and clippy shows no new warnings. Orbita keeps the Dynamixel v1 rule, which is fine since its firmware runs an older rustypot.