Improve feetech driver#64
Merged
Merged
Conversation
- Drop redundant read_pos_vel_cur sync wrapper; the public method now does the sync read directly with per-motor fallback - Rename _read_*_individual fallbacks to _read_*_per_motor_fallback so the intent is obvious at the call site - Centralize POSITION_DIRECTION via _raw_to_rad / _rad_to_raw helpers so the sign inversion stops leaking into read/write logic - Define MotorError + MotionTimeoutError; wait_for_motion_complete now raises on timeout instead of silently returning False - Add MotorClient.waits_for_motion class flag; OrcaHand.wait_for_motion skips the motor lock when the underlying client doesn't actually block - Strip the verbose AI-style comment from calibrate_offset - Add explicit parens to the sign-flip in write_positions_sync - Rename scripts/test.py -> scripts/stress_test.py and update docs
Configs no longer need to declare motor_type, port, or baudrate. At
connect time the client probes attached USB serial adapters and pings
each candidate (motor_type, baudrate) pair until one responds.
- Add probe() static method on DynamixelClient and FeetechClient that
opens a port at a given baudrate, sends a few pings, and reports
whether any motor responds — without enabling torque
- Add MOTOR_BAUD_RATES (constants.py): {dynamixel: [1M, 3M], feetech: [1M]}
- Add motor_type_for_port and find_single_usb_serial_port helpers in
utils for VID-based detection and lone-adapter fallback
- OrcaHandConfig.motor_type/port/baudrate become Optional; explicit
values still win and short-circuit the probe along that axis
- OrcaHand.connect now does:
1. resolve port (yaml > VID > sole USB adapter > interactive picker)
2. trial-probe to find motor family + baudrate
3. instantiate the right client and connect
An auto-detected port is persisted to config.yaml; motor_type and
baudrate are intentionally not persisted so configs stay driver-agnostic
- Strip motor_type/port/baudrate from all bundled v1/v2 model configs
- Delete orcahand_right_feetech model dir; orcahand_right is now used
for both Dynamixel and Feetech builds of the same hand
- Update stress_test.py defaults: --num-steps from constants (50),
--step-size from constants (0.01s), --hold default 2s — restoring
pre-rename motion timing instead of the way-too-fast 25 steps × 1ms
- Add tests/test_connect_resolution.py covering port lookup, single-USB
fallback, trial-probe at multiple baudrates, and pinned-motor_type
Brings the configure_motor_chain script and core calibration logic to parity with the Feetech build of the hand. Ported from fb/dev with adaptations for the auto-detect machinery introduced earlier on this branch. configure_motor_chain.py - Optional --motor-type with auto-detection at factory defaults (Dynamixel @ 57600 ID 1, Feetech @ 1M ID 1) when omitted - --reset to revert configured motors back to factory defaults - --baudrate to bulk-change baud without touching IDs - Safe power-cycle prompt for Feetech (unkeyed connectors → reverse polarity destroys the motor; force unplug-replug for each insert) - HLS3930 wrist + HLS3915 fingers split with substring model checks, matching the existing XC430/XC330 split for Dynamixel - _close_silently() helper used after each ID/baud mutation so the spurious "no status packet" disconnect logs go away - Pre-scan filter recognises HLS3930 at ID 1 as a configured wrist (not a factory-default to ignore) - Positional arg renamed model_path -> config_path for parity with every other script in the repo FeetechClient - scan_for_motors / change_motor_id / change_motor_baudrate - FEETECH_BAUD_RATE_MAP (host-baud -> register code) - FEETECH_MODELS populated for the three model_number values seen in our 17-motor builds: 4106 -> HLS3930, 6922/5130 -> HLS3915 Calibration core - OrcaHand.calibrate(joints=...) and _calibrate(joints=...) filter the calibration sequence by an explicit joint allow-list. Lets you re-run one finger or one joint without touching the others. The scripts/calibrate.py CLI for this is intentionally a follow-up commit. Bundled configs - Each model's config.yaml gets a commented-out `port:` / `baudrate:` hint at the top so users can find the override path quickly without hunting through docs utils - Re-export auto_detect_port and get_and_choose_port from the package root so scripts can `from orca_core.utils import ...` directly
Calibrate part of the hand without disturbing the rest of an existing calibration, and stop a partial re-run from leaving the YAML in a half-state. scripts/calibrate.py - --fingers (thumb / index / middle / ring / pinky / wrist) and --joints (individual joint names) flags, mutually exclusive - Resolves the selection to a flat joint list and forwards via hand.calibrate(joints=...) orca_core/hardware_hand.py - _calibrate accumulates direction measurements in a separate pending_limits dict; only commits a joint's [lower, upper] to motor_limits once BOTH directions have been measured - Removes the upfront wipe loop that nulled motor_limits for every targeted joint before any motion. Previous behaviour was that an interrupted partial calibration zeroed out the joint's data even though the new values weren't measured yet — now the old values stay in motor_limits.yaml until the new ones are fully captured - _wrap_offsets_dict reset moved into the per-step setup so it still gets cleared for joints we touch this run Mirrors fb/dev's e80f7ad applied on top of our hardware_hand.py structure (we have CalibrationResult / config dataclass; fb/dev had flat attrs).
main contains a revert of PR #59 (the original Feetech-driver work this branch is built on). Resolved by keeping our branch's content in feetech_client.py, motor_client.py, and hardware_hand.py — our branch is the desired new state for those files. The PR #60 revert was an "uncursing" mass-revert with no per-file rationale, so the conflicts are spurious to the actual goal here.
DynamixelClient - Mirror the sync-with-fallback pattern Feetech got in 1df95b3. DynamixelReader.read() now falls back to per-motor reads on bulk failure (entire packet) or on partial response (specific IDs missing from the bulk packet) - Default _read_per_motor_fallback is a no-op so subclasses without an override (e.g. _moving_status_reader) keep their stale-cache behaviour, matching the prior version - DynamixelPosVelCurReader._read_per_motor_fallback uses read4ByteTxRx for position/velocity and read2ByteTxRx for current, same unsigned_to_signed + scale conversion as the bulk path. A motor whose individual read also fails keeps its previously-cached value - DynamixelTempReader._read_per_motor_fallback does the same with read1ByteTxRx for ADDR_PRESENT_TEMPERATURE - Resolves Francesco's PR #62 L276 comment ("do the same in dynamixel so both clients are on par"). One bad cable / dropped motor on the daisy chain no longer blanks every joint reading on screen — only the actual offender shows stale data stress_test.py - Switch hand.enable_torque() -> hand.init_joints(force_calibrate= args.mock). init_joints handles enable-torque, control-mode setup, AND auto-calibration if the hand isn't calibrated yet, matching every other position-commanding script in scripts/. Running stress_test on a fresh hand will now run the full calibration routine first instead of commanding undefined joint positions
Collaborator
fracapuano
self-requested a review
May 7, 2026 13:30
fracapuano
approved these changes
May 7, 2026
fracapuano
left a comment
fracapuano
left a comment
Collaborator
There was a problem hiding this comment.
Ciao there 👋 Left a couple comments and leaving an approval conditioned on your assessment of them @maximilianeberlein! I think they could really improve UX, but the PR is in a great place to be merged and iterate
| """Get list of valid baudrates for the given motor type.""" | ||
| if motor_type == 'feetech': | ||
| return sorted(FEETECH_BAUD_RATE_MAP.keys(), reverse=True) | ||
| return sorted(DYNAMIXEL_BAUD_RATE_MAP.keys(), reverse=True) |
Collaborator
There was a problem hiding this comment.
Suggested change
| return sorted(DYNAMIXEL_BAUD_RATE_MAP.keys(), reverse=True) | |
| elif motor_type == "dynamixel": | |
| return sorted(DYNAMIXEL_BAUD_RATE_MAP.keys(), reverse=True) | |
| else: | |
| raise ValueError(f"Unknown motor type. Accepted motor types ['dynamixel', 'feetech'] (provided: {motor_type}).") |
Collaborator
There was a problem hiding this comment.
I feel this file is way to long for what it does, and because the ideal PR has very few lines of code, maybe we could trim it down significantly?
| # Max operating temp (°C) — conservative across XC330 (70°C) and XC430 (72°C) | ||
| MAX_TEMP = 70 | ||
|
|
||
| MAX_TEMP = 70 # °C — conservative across Dynamixel XC330/430 and Feetech STS3215 |
Collaborator
There was a problem hiding this comment.
we could also store a dictionary with two different values for clients, in case the maximal operating temperature is significantly different between the two clients
| # ----- _trial_probe ------------------------------------------------------- | ||
|
|
||
| @pytest.fixture | ||
| def mock_hand(mock_config_dir): |
Collaborator
There was a problem hiding this comment.
it'd be better if fixtures were in conftests.py to keep tests tiny
| monkeypatch.setattr( | ||
| feetech_client.FeetechClient, "probe", staticmethod(feetech_probe) | ||
| ) | ||
| OrcaHand_trial_probe(mock_hand, "/dev/cu.x") |
Collaborator
There was a problem hiding this comment.
nit: I don't like this helper defined outside of the function/fixture in here
Round of cleanups in response to Francesco's review comments on PR #64. API - New MotorRead NamedTuple (position / velocity / current) returned by read_position_velocity_current (renamed from read_pos_vel_cur). Named attribute access AND tuple unpacking so callers can keep doing `pos, vel, cur = client.read_position_velocity_current()`. Migrated every caller in hardware_hand, mock client, dynamixel demo block, and the three diagnostic scripts. dynamixel_client - dynamixel_sdk import hoisted to module level (PortHandler / PacketHandler / COMM_SUCCESS) so probe() doesn't pay name-resolution per call. Test fakes in conftest.py and scripts/common.py extended with COMM_SUCCESS. - _read_per_motor_fallback hook docstring made explicit so the template-method pattern isn't misread as duplicate dead code. feetech_client - FEETECH_MODELS comment trimmed to one line. - scan_for_motors signature uses Optional[list] = None with an explicit `if baud_rates is None` check rather than the truthy-fallback idiom. - Brief inline comment on the position clamp in write_positions_sync. calibrate.py - Mutex error message wording: "Use either one." per Francesco's suggestion. configure_motor_chain.py (significant rewrite) - Top-level FEETECH/DYNAMIXEL/MOTOR_TYPE_DEFAULTS/MOTOR_MODELS constants replace scattered string literals. - `mid` renamed to `motor_id` throughout. - print() routed through `logger.error` / `logger.info` for error and status messages so users can silence them via logging config. - get_valid_baudrates gets explicit elif/else with ValueError. - AI-slop comments trimmed; docstrings added for scan_already_configured_motors and the obscure id-walk + expected-sequence prints inside it. - Two near-identical port-wait helpers collapsed into one (`present=` kwarg). - Two near-identical bulk loops (baudrate change vs factory reset) collapsed into _process_motors_loop with an action callback. - Triple `client = ...; client.connect(); ...; _close_silently(client)` pattern replaced by a _config_session context manager. - Per-family branching (`if motor_type == FEETECH` etc.) replaced by MOTOR_TYPE_LABELS / MOTOR_MODELS / VALID_BAUDRATES dict lookups. - Decorative section dividers and unused colour constants dropped. - Net: 701 -> 544 lines (~22% smaller) with the actual duplication gone. yaml configs - Inline port/baudrate "uncomment to override" hints removed across all bundled configs — Francesco's "ship batteries-included" point. Override documentation goes in docs, not in every shipped config. tests - fake_serial_port + patch_comports + mock_hand fixtures moved from test_connect_resolution.py into conftest.py so test files stay focused. - Hop-over helper killed: tests now call OrcaHand._trial_probe(hand, port) directly to exercise the real implementation on a MockOrcaHand instance.
probe() now pings motor_ids[0] and motor_ids[-1] instead of the first two, so partial bench setups (e.g. only the last motor on the chain) get detected. Drops the unused ping_count parameter. Promotes DynamixelClient/FeetechClient imports in hardware_hand to module level to avoid per-call name resolution in _trial_probe and _create_motor_client.
port / baudrate / motor_type were removed from bundled configs when auto-detection landed, but the override path (drop them back into config.yaml when you need to) wasn't documented anywhere users would find it. Now covered in: - README "Serial port, baudrate, and motor type" section — replaces the outdated "Serial Port Path" block that told users to set the port in yaml. New section explains auto-detect + three override use cases (multiple adapters / non-default baud / explicit family). - setting-up-config.md General Settings — bundled yaml example trimmed, new "Optional driver overrides" subsection mirrors the README.
…robe When motor_type or baudrate is missing from yaml the connect-time probe fills it in, and the result is now written back to the file. Subsequent connects find the populated values and skip the probe entirely. Each field is only persisted if the yaml didn't have it — manual edits stick. To swap motor families on the same hand, clear the field(s) in yaml and reconnect to trigger a fresh probe. _persist_resolved_port becomes _persist_resolved_driver and handles all three fields uniformly; a single console line announces which were written so the yaml mutation isn't silent.
`port: str = '/dev/ttyUSB0'` and `id_range: tuple = (0, 252)` were vestigial — every caller passes both arguments explicitly. The Linux- path default was actively misleading on macOS / custom adapters, and the (0, 252) full-address-space default would silently make any unguarded call do a 253-ID scan. Made both required on FeetechClient and DynamixelClient; baud_rates stays Optional[list] = None since that fallback is legitimately useful. Also switched DynamixelClient.scan_for_motors's baud_rates fallback from `... or list(...)` to an explicit `if baud_rates is None`, matching the FeetechClient pattern. Extends Francesco's "I would not add this default, rather use a None" review note (PR #64) to the two other defaults in the same signature.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces several improvements to motor hardware support, configuration flexibility, and robustness in the ORCA hand control software.
The main changes include enhanced auto-detection and configuration of motor types and baud rates, improved fallback handling for hardware communication failures, and new utilities for managing Feetech motors.
These updates make the system more adaptable to different hardware setups and more resilient to communication issues.