Repository navigation
Proposal: find a model by name, state more servo facts, and turn the torque of a whole bus - #143
Conversation
… Python A caller that knows a motor by its model name, as LeRobot's robot configs do (sts3215, xl330-m288), has to know which controller class covers it before it can reach the model number, the resolution or the registers. LeRobot keeps one bus class per vendor for little more than that list. find_model(name) walks the registry and returns the model number and the definition, comparing names without case, hyphens or underscores. Each definition now carries the models it covers, and the Python ServoDefinition exposes what the controller classes expose as static methods, so the definition a Bus is given is enough on its own.
Three facts about a servo that every caller transcribes by hand from the manuals, and gets wrong across families: - the homing offset is subtracted from the position on Feetech servos and added on Dynamixel ones, so centring a joint computes it the other way; - the operating mode register takes 0 for position control on a Feetech and 3 on a Dynamixel, where 0 is current control; - a new motor answers at 1 Mbps on Feetech and 57.6 kbps on the XL330 and XL430. A definition can now state them as factory_baudrate, homing_offset_sign and operating_modes (by name), filled in for the STS3215, SCS0009, XL330 and XL430. A sign or modes on a servo without the register they describe fails the build.
Releasing a robot means turning the torque off on all its motors. Written as a loop of writes, the first motor that does not answer raises and leaves every motor after it holding its position, which is the one thing the release was for. Bus::set_torque tries every motor, each with its own retries, and returns the ones that failed with their error instead of stopping at the first. Feetech servos also have a lock register guarding the EEPROM, which they only write to with the lock open; LeRobot closes it with the torque on and opens it with the torque off. A definition can now say eeprom_lock: true (STS3215, SCS0009) and set_torque moves the lock with the torque on those. The AX and MX lock is not that kind of lock (it only clears at power-up) and is left alone. Checked on an SO-101 follower: torque off on its six motors plus an id that is not there, in the middle of the list; the six went through and only the missing one came back, after three tries, in 0.16 s.
A Python caller could close a Bus but not ask whether it still held its port, so a test checking that a failed connection released the port could only trust its own bookkeeping. And releasing the port on every way out of a block took a try/finally at every call site. Bus.is_open says whether close() has run, and `with Bus(...) as bus:` closes it on the way out, an exception in the block included, which then goes on. Checked on an SO-101 follower: the port is released after the block, after an exception in it, and opens again straight away.
pierre-rouanet
left a comment
pierre-rouanet
left a comment
There was a problem hiding this comment.
Thanks, here's my review commit by commit. All four are accepted.
1. find_model and the Python definition getters (c9eeda5): accepted.
2. homing_offset_sign, operating_modes, factory_baudrate (f347124): accepted. One thing to keep in mind about factory_baudrate: it is a fact about current motors, not something that holds for every unit forever. Some older motors shipped with a different factory baud rate, a change that came with the protocol v1 → v2 transition. I don't expect it to happen again, so one value per definition is fine. Maybe mention it in the doc comment so nobody reads it as a guarantee for every unit ever made.
3. Bus::set_torque (fad79fa): accepted.
4. Bus.is_open and the context manager (ca8d69c): accepted.
A suggestion for the docs: between #142 and this PR, Bus and ServoDefinition are turning into a higher-level API, mostly meant for the LeRobot library (finding a model by name, servo facts like operating modes and homing sign, torque with the Feetech lock following it, setting up a motor). Could you say so explicitly, in the README and in the Bus doc? Something like "Bus / ServoDefinition give a higher-level, family-agnostic API, designed mostly for LeRobot; the per-servo controllers stay the low-level API". Then other users know which layer to pick, and it explains choices that are LeRobot conventions (e.g. the lock following the torque).
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.
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. What still keeps them apart is a short list of facts about the servos, transcribed by hand from the manuals:sts3215orxl330-m288, and that model's exact number;The same review found a gap in how LeRobot releases a robot: torque goes off motor by motor, and the first one that does not answer stops the loop, leaving the motors after it holding their position.
The question behind this PR is whether those facts belong in the servo definitions. I think they do, because they are about the servos and any caller of the raw or by-name API needs them, but you know better than me what rustypot should carry. #142 covers finding and setting up motors and is independent of this one.
What it proposes
1.
find_model(name), and a definition readable from Python (c9eeda5)find_model(name)walks the registry and returns(model number, definition), comparing names without case, hyphens or underscores, soxl330-m288findsXL330M288. EachServoDefinitionnow carries themodelsit covers, and the PythonServoDefinitiongets the getters the controller classes have as static methods (models,resolution,word_order,supports_sync_read,baudrates,registers(),register(name)). With that, a caller holding a definition, which is what aBustakes, needs no controller class.XL-330_M288matches too). The Python getters duplicate the controller static methods.ServoDefinitiongains a public field, so code building one by hand must add it.2.
homing_offset_sign,operating_modesandfactory_baudrate(f347124)Three optional facts a definition can state, ahead of
encoding:, onServoInfoand as Python getters:factory_baudratehoming_offset_sign(present = actual + sign * offset)operating_modesA sign on a servo without a
homing_offsetregister, or modes without anoperating_moderegister, fail the build, like theencoding:names already do.homing_offset_signis one way to put it; you may prefer to describe the offset some other way.3.
Bus::set_torque(ids, enabled, retries)(fad79fa)Turns the torque of every motor asked on or off, trying each one even after another fails (each with its own retries), and returns the ones that failed with their error (
{id: message}on Python, empty when all went through). On a servo whose definition sayseeprom_lock: true(STS3215, SCS0009), the lock follows the torque: closed with it on, open with it off. The AX and MXlockonly clears at power-up, so they do not set the flag and their lock is never touched.set_torquecould leave the lock alone and LeRobot would handle it with theeeprom_lockfact, or without it. Returning the failures instead of aResultis unusual for this API.4.
Bus.is_open, andBusas a context manager (ca8d69c)is_opensays whetherclose()has run;with Bus(...) as bus:closes it on the way out, an exception in the block included.Not included, and maybe not worth it
CommunicationErrorKind::TimeoutError, the same as a motor that did not answer. Worth a separate discussion if you are interested.Tests
find_modelby name with and without hyphens, the new facts of the STS3215, SCS0009, XL330, XL430 and MX,set_torque(torque then lock on an STS3215, torque only on an XL330, a motor that does not answer and an id not on the bus both reported while the others go through, the lock closing when the torque goes on).find_modeland the getters;set_torque(off)on its six motors plus an id that is not there, put in the middle of the list (the six went through, only the missing one came back, after three tries, in 0.16 s); thewithblock releases the port, also after an exception, and the port opens again straight away. I did not turn the torque on from these tests, since a motor would jump to whatever goal position it last had.Merging with #142
The two PRs add to the same places (the
Unreleasedsection of the changelog,src/bus.rs, the servo README), so whichever lands second will conflict there. I checked that the combination builds and passes the tests, and I will rebase the second one.