[nicolas-rabault]

This is a proposal, and turning it down (all of it or some commits) is a perfectly fine answer. Each commit stands alone, and for each one I say what LeRobot would do instead if you'd rather not have it in rustypot.

Updated after review: rebased on develop (with #143), scan left as it was and the broadcast path moved to its own broadcast_scan, the change_baudrate docstrings say the port stays at the old rate, and a docs commit for the two points raised on #143. The resync of the broadcast ping is #144, in a follow-up PR.

Why we are asking

LeRobot's port to rustypot.Bus (huggingface/lerobot#4672) got a review asking LeRobot to keep only robot logic, so that its two vendor bus classes (FeetechMotorsBus, DynamixelMotorsBus) can merge into one. Going through it, most of what is left in those classes is not LeRobot logic: it is knowledge about the servos that LeRobot transcribed by hand from the manuals. The review also found two real problems in how LeRobot now finds and sets up motors:

• scan reads the model number of every id under a 5 ms timeout. A U2D2 (FTDI chip) holds a short answer up to 16 ms (its latency timer) before passing it on, so a motor that is there can read as absent. Lengthening the per-id timeout to cover that makes a sweep at one baud rate take about 5 s.
• baudrates() comes from a HashMap, so LeRobot tries the baud rates in a different order on every run.

This PR covers finding and setting up motors. #143 (merged) covered the servo facts and the torque.

What it proposes

1. DynamixelProtocolHandler::broadcast_ping (4f35736)

One ping to the broadcast id; every motor that hears it answers in turn, and the handler listens for one window sized like the vendor SDKs size it: the status packets of every possible id, 3 ms of turn per id and 16 ms of adapter latency (about 0.8 s at 1 Mbps). A status packet with a bad checksum is skipped, and the port's timeout is put back. A framing error still ends the listening; #144 tracks resyncing on the next header instead.

• For: the adapter's latency is covered once for the whole bus instead of once per id. It is how the Dynamixel SDK finds motors (broadcastPing), and how LeRobot found its STS motors before moving to rustypot.
• Against: the window is fixed: 0.8 s at 1 Mbps even when every motor answered in the first few milliseconds, since nothing says the last one has spoken. It also adds an instruction to maintain.
• If you'd rather not: LeRobot has no way to send a broadcast ping through the Python API, so it would have to give up the U2D2 case or sweep with a longer timeout (about 5 s per baud rate).

2. broadcast_scan, next to an unchanged scan (09765a9)

broadcast_scan(ids) / broadcast_scan_all() on ServoDefinition, the controllers and Bus (broadcast_scan(ids=None) on Python): one broadcast ping, then the model number of each id asked that answered, under the port's own timeout. scan is untouched, so checking whether one motor is there still takes 5 ms; a caller sweeping the whole bus picks broadcast_scan on purpose.

A definition says supports_broadcast_ping: true (default false): the STS3215 and the protocol v2 Dynamixel (XL320, XL330, XL430). On any other servo broadcast_scan is a new RegisterError::BroadcastPing (ValueError on Python) and nothing is sent, rather than falling back to the sweep behind the caller's back. The flag is also on ServoInfo, the controller classes (supports_broadcast_ping()) and the Python ServoDefinition, so LeRobot writes bus.broadcast_scan(d) if d.supports_broadcast_ping else bus.scan(d).

• For: the scan behind a U2D2 works, and nobody's existing scan changes speed.
• Against: a second scan method to know about. A firmware that claims the flag but does not answer would find nothing; I could only check the STS3215 on hardware, the Dynamixel side relies on the protocol 2.0 spec. I kept the flag explicit on every definition rather than implied by protocol v2, so a v2 servo that turns out not to answer can say so.

3. change_id and change_baudrate (ba79256)

On ServoDefinition and Bus (and the Python Bus): torque off, lock open where the servo has a lock register, then the write. The baud rate is given in bauds and written as the value the definition's baudrates lists for it; a rate it does not list is a new RegisterError::Baudrate (ValueError on Python) and nothing is sent. The port stays at its rate, and the docstrings now tell the caller to call set_baudrate to keep talking to the motor. Like scan, they take the definition and reach motors the bus does not have, since a new motor answers at its factory id.

• For: the EEPROM rules (torque off, Feetech lock open, baud rate as a code) live next to the registers they are about, instead of in every caller.
• Against: it is policy: the torque and the lock are left the way the write needs them, not put back. Writing 0 to the lock of an AX or MX changes nothing (theirs only clears at power-up), so it is harmless there, but it is a write they did not need.

4. baudrates() and models() in a fixed order (ab9abcf)

The two Python static methods of the controller classes now build their dict from a BTreeMap (baud rates slowest first, models by name), like the ServoDefinition getters from #143. Still a dict.

5. Docs: Bus and ServoDefinition as the higher-level API (cf861c5)

As suggested on #143. The README and the crate docs listed two APIs, the protocol handler as the low-level one and the controllers as the high-level one; they now list three, from the wire up, with Bus / ServoDefinition as a higher-level, family-agnostic API designed mostly for LeRobot, whose choices can be LeRobot conventions (the Feetech lock following the torque). The controllers are described as the per-servo API rather than "low-level", so the new text does not contradict the existing one; tell me if you prefer other words. The Bus docs (Rust and Python) say the same, the README's mixed-bus section shows what the higher layer carries, and factory_baudrate says it holds for the units made today.

Not included, on purpose

• A real PING per id instead of reading Model_Number: the review mentions Dynamixels with Status Return Level 0, which ignore reads. LeRobot cannot use such a motor anyway (it reads positions), so we left it.
• Decoding each model number with its own definition's byte order in a scan: LeRobot's branch now prints only the ids it finds, so it does not need it.
• A longer floor for the per-id sweep: an AX or MX behind a U2D2 may still be missed with the 5 ms floor of scan; tell me if you want it raised or made a parameter.

Tests

• Unit tests on the fake port: a broadcast ping on v1 (with a status packet that fails its checksum, skipped) and on v2, the window formula, broadcast_scan (one ping then one read, one instruction for a whole v2 bus, an SCS0009 refused with nothing sent), change_id (torque, lock and id writes, at an id the bus does not have), change_baudrate (the register value, and a refused rate sends nothing). The scan tests are the ones on develop, unchanged.
• On hardware (two SO-101 arms, six STS3215 each), before the split: the broadcast ping finds the six ids at 1 Mbps and none at 500 kbps or 115.2 kbps, in 790 ms; the ping-then-read path, now broadcast_scan, returns {1..6: 777} in 0.8 s (1.3 s with the sweep); on a leader arm, motor 6 taken to id 7 and back, then to 500 kbps and back to 1 Mbps, each step confirmed by a scan. After the split, on the same two arms (read only): 20 runs of broadcast_scan_all per arm all return exactly what the scan_all sweep returns ({1..6: 777}), in 0.8 to 0.9 s.
• Nothing on a Dynamixel: I have no U2D2 or X-series motor here.

[pierre-rouanet]

Thanks for splitting this into separate commits with the for/against for each one. That made it easy to review commit by commit.

1. broadcast_ping (fe7cd78): approved. One improvement doesn't need to block this PR, so I opened #144 for it: the listening loop stops at the first bad header or mid-packet timeout, which loses every motor that hadn't answered yet. The SDK resyncs on the next header instead.

2. scan with a broadcast ping (c538508): partly accepted.

• The supports_broadcast_ping entry in the definition (macro, ServoInfo, Python static method) is fine, please keep it.
• I'd rather not change what the existing scan does. Right now scan(&[1]) on an STS3215 or an XL takes about 5 ms. With this change it always takes the full window (about 0.8 s at 1 Mbps), whatever ids asks for. Anyone who uses scan to check whether one motor is there gets that slowdown with no change at the call site.
• Please put the ping-then-read path in a separate method instead (smart_scan, broadcast_scan, whatever name you prefer), on ServoDefinition, the controllers and Bus/Python, and leave scan as the plain sweep. Callers like LeRobot can then pick it on purpose. Your question about whether v2 should imply the flag fits that new method well.

3. change_id / change_baudrate (9e953da): fine with it. This is new API that changes nothing existing, so OK. One request: say in the change_baudrate docstrings (ServoDefinition, Bus and the Python Bus) that the bus's own port stays at the old rate, so the caller has to call set_baudrate to keep talking to the motor afterwards.

4. baudrates() / models() in a fixed order (b0a1444): nice, approved.

So the only change that blocks merging is moving the new scan behaviour into its own method, plus the docstring.

[nicolas-rabault]

Thanks for the review. I rebased on develop now that #143 is in, and pushed:

• scan is unchanged. The ping-then-read path is now its own method, broadcast_scan(ids) / broadcast_scan_all() on ServoDefinition, the controllers and Bus (broadcast_scan(ids=None) on Python), in 09765a9. The supports_broadcast_ping entry stays as it was. On a servo without the flag, broadcast_scan fails with RegisterError::BroadcastPing (ValueError on Python) before sending anything, instead of quietly falling back to the sweep, so nothing changes behind the caller's back and LeRobot picks one or the other explicitly. I kept the flag explicit on each definition rather than implied by protocol v2, so a v2 servo that turns out not to answer can say so. Happy to switch to "v2 implies it" if you prefer.
• change_baudrate docstrings (ServoDefinition, Bus and the Python Bus) now say the port stays at the old rate, and that the caller has to call set_baudrate to keep talking to the motor (ba79256).
• Your two docs points from Proposal: find a model by name, state more servo facts, and turn the torque of a whole bus #143 are in cf861c5. The README and crate docs now list three APIs, from the wire up, with Bus / ServoDefinition as the higher-level, family-agnostic API designed mostly for LeRobot. I called the controllers "the per-servo API" rather than "low-level", because the existing text already calls the protocol handler low-level and the controllers high-level. Tell me if you'd like other wording. factory_baudrate now says it holds for the units made today.
• broadcast_ping: resync on a bad header instead of ending the scan #144 is Resync a broadcast ping past bytes that do not frame a packet #147, stacked on this one.

The description is updated with the new hashes.

[pierre-rouanet]

LGTM, thanks @nicolas-rabault

[nicolas-rabault]

Ran this branch on hardware after the split: two arms with six STS3215 each, read only. On each arm, 20 runs of broadcast_scan_all all return exactly what the scan_all sweep returns ({1..6: 777}), in 0.8 to 0.9 s, and broadcast_ping finds nothing at 500 kbps or 115.2 kbps. Description updated.