fix: the re-zero is proven on hardware — and the arc was in the wrong frame (#35)#41
Merged
Merged
Conversation
…d reads retry, writes never do Observed live on hardware today: scservo_sdk's read2ByteTxRx can raise a bare IndexError internally on a short/corrupt packet (it indexes data[0]/ data[1] whenever result==COMM_SUCCESS, even when the buffer is short), escaping this repo's no-traceback-ever-leaks contract before the result/error check ever runs. FeetechBus reads (read_position, the shared _read_register path behind read_info/read_offset/read_torque_limit, and read_lock) now funnel through _sdk_read (converts any bare SDK exception into a CliError) and _retry_read (bounded 3-attempt retry, since a read is idempotent) — OverloadError is never retried or converted, it propagates on the first occurrence exactly as before. Second finding: a read issued immediately after an EEPROM write (the Lock dance) can return a silently-wrong-but-plausible value — a genuine offset-0/position-3387 write read back as position 0 about 0.2s later, then correctly and stably moments after. _set_lock now settles (_EEPROM_SETTLE_SECONDS) after closing the Lock, before returning control to a caller that might read straight back. Writes are never retried (a "failed" write may have landed — see #21's Lock bug and #38's id-transfer-window bug) — pinned by call-count assertions in the new tests.
…eads retry, writes never do
…ed in the wrong frame The re-zero shipped in PR #40 worked, but by luck, not by construction. Two hardware findings, follower /dev/ttyACM1, 2026-07-12: * The servos ship holding `Ofs = 85`, not the factory 0 the spike assumed — uniform across all six joints, so a vendor default. `Present = Actual − Ofs`, confirmed by a reversible probe. * `elbow_flex`'s far wall, measured for the first time by a torque-off hand sweep with the seam evicted: travel is 2196 ticks, reported 1034..3230 at `Ofs = 1073`. The same sweep CLOSES the spike's §4 caveat — `Ofs=0` gave `discontinuities: 1`, `Ofs=1073` gave `0`. The correction IS reduced mod 4096; the seam relocates. The bug: REZERO_ARCS held `elbow_flex: (126, 2020)`, and those ticks were read off a servo already holding Ofs=85 — they are REPORTED-frame ticks, used as if they were RAW. The offset register operates in the raw frame (the seam lands where Actual == Ofs), so the target was off by the pre-existing offset. It landed inside the true arc anyway, with ~866 ticks of margin — the most dangerous way for a frame bug to behave, because every read-back looked right. * `UnreachableArc` is RAW ticks, said loudly: a property of the joint's travel, independent of whatever is in the register. Arc corrected to the measured `(207, 2107)` — raw travel `[2107, 4095] ∪ [0, 207]`, which wraps. Midpoint 1157. * `plan_rezero` is frame-aware: reads the live offset, converts `raw = (reported + offset) mod 4096`, reasons entirely in raw. The `current in (0, target)` refusal is GONE — it blocked the verb outright on a factory-fresh servo, i.e. on every real SO-101. * The goal is "the seam is OUT of the joint's travel", not "the offset equals N". `UnreachableArc.evicts()` is the new question, and an offset already inside the arc is a NO-OP — our arm holds 1073 and is left alone rather than rewritten to 1157 for cosmetic centring. `SweepReport.rezeroed` asks the same question, so --verify on this arm PASSES instead of reporting "inconclusive". Every safety property holds: no motion on any path, clear_overload first, torque-off → unlock → addr 31 → re-lock, addr 9/11 never touched, wrist_roll still refused with its own distinct reason. 1222 tests pass (was 1202); four linters and teken --strict clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018vrVNSJ9kzhCQqwpA2e8u6
…ticks The 34 failing tests were not failing because the arc changed. They were failing because they had copied it. `REZERO_ARCS["elbow_flex"]` was re-derived on hardware today and given a deliberate margin — (207, 2107) -> (318, 2007), midpoint 1157 -> 1162 — because the first cut declared the arc AT the extremes of a single hand sweep and FALSE-REFUSED within minutes: the joint came to rest at raw 218, eleven ticks past an edge taken from a sweep the operator had simply stopped short of, and `arm rezero` correctly reported that the joint "cannot be where it says it is". A hand-found wall is not a crisp number (206..218 depending on push force), and at least one wall may be the TABLE rather than the joint's own mechanical stop. So the arc is a measurement that WILL be taken again — and re-taking it must cost ONE table edit. It cannot cost 34 broken tests every time, or the arm's own measurements start losing arguments with the test suite. Replaces ~125 arc-coupled literals across test_rezero.py and test_arm_rezero_cli.py with values derived from arm_spec: the target from `rezero_offset()` / `UnreachableArc.midpoint`, the bounds from `.low` / `.high`, the travel from `.travel_ticks`, the walls and inset from `_LOW_WALL_OBSERVED` / `_HIGH_WALL_OBSERVED` / `_ARC_MARGIN_TICKS`. Positions inside/outside the arc and "what the servo will then report" are computed (`reported_at`, `_full_sweep`), never baked in — including the sweep LENGTHS, which must scale with the travel or a re-measured arc turns a passing sweep into a silent INCONCLUSIVE. Kept literal on purpose, because each is a fact rather than a derived quantity: 85 (the factory offset every SO-101 ships with), 1073 (what our follower actually holds — it evicts the seam, so a re-zero is a NO-OP), 126 (where elbow_flex rests), 4096 (the encoder modulus), and the 2026-07-12 sweep's own readings (1034/3230 reported at Ofs=1073 -> raw 2107/207). No assertion was weakened, skipped or deleted, and no production code was touched. Two assertions were ADDED where derivation exposed a claim worth pinning: the arc may never contain a tick the joint has actually been seen at (the false-refusal, stated as an invariant), and the raw 4095->0 roll-over becomes a ONE-TICK step (issue #35's deliverable, previously only implied across parametrize rows). `test_OUR_ARM_at_1073_is_a_clean_NO_OP_ not_a_rewrite_to_1157` is renamed to `..._not_a_rewrite_to_the_arcs_midpoint` — it named a number that went stale the same day, and the claim was never about the number. Acid test: setting _ARC_MARGIN_TICKS to 150 (and, separately, to 300, and re-measuring BOTH walls so the midpoint moves 1162 -> 1195) leaves all 1242 tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018vrVNSJ9kzhCQqwpA2e8u6
… frame (v0.22.0) Refs #35. A live session on the follower with the operator present. THE RESULT. The re-zero WORKS. The STS3215 reduces the corrected position MODULO 4096, so a homing offset genuinely RELOCATES the encoder seam rather than merely relabelling positions. That was the one unproven assumption the whole of issue #35 rested on — the t5 spike said GO-WITH-CAVEAT precisely because no document states it — and it is now settled by measurement. Same joint, same hand sweep, before and after: offset 0 -> monotonic FALSE, 1 discontinuity, span 4093 offset 1073 -> monotonic TRUE, 0 discontinuities, span 2196 It survived a power cycle, read back cold. PR #21's ghost is laid. THE MECHANISM, not just the symptom. Every SO-101 ships with Ofs = +85, NOT the factory 0 the spike assumed (uniform across six servos => a vendor default). And that is exactly WHY elbow_flex wraps: the seam sits where Actual == Ofs, i.e. at raw 85, which is BELOW the unreachable arc and therefore INSIDE the joint's travel. We did not merely fix the bug; we now know why it happened. THE FRAME BUG. REZERO_ARCS held REPORTED ticks — read off a servo already carrying that factory offset — and used them as RAW. The old target landed inside the true arc anyway with ~866 ticks of margin, so it worked, and every read-back looked correct. That is the most dangerous way for a frame bug to behave. The arc is now RAW by construction, and raw = (reported + offset) mod 4096 on every path. The `current in (0, target)` guard is gone. It refused outright on a servo holding the factory 85 — i.e. on EVERY FRESH SO-101. The goal is now stated as a PLACE (the seam is out of the travel), not a magic number: an arm already holding a seam-evicting offset is a clean NO-OP, not a rewrite. THE ARM MEASURED ITS OWN WALLS. Once the axis was linear, gentle_move could be driven past the known travel and left to find each wall by feel — creep, watch the load, stop on contact. Both contacts saturated at the 500 torque cap. It out-measured the human on BOTH sides (raw 251 vs 218; 2061 vs 2107), because it presses to a fixed load every time instead of to whatever felt firm. The arc is INSET from those walls, so a harder push can never make the table contradict the arm — which is exactly what a tight arc did, within minutes. And the second wall-find crossed the raw 4095->0 boundary as a plain linear climb. That command would have rotated the elbow the LONG WAY ROUND into a wall yesterday. It is the whole day's work in one move. TESTS NO LONGER COPY THE TABLE. ~125 arc-coupled literals were hard-coded across two files, so re-measuring a table that EXISTS to be re-measured broke 34 tests. They now derive from arm_spec; changing a wall or the margin leaves the suite green. Same rule for prose: the docstrings and the explain catalog no longer quote ticks at all — and explain is what prints to whoever is standing at the arm. TWO BUS BUGS, both hit live on a healthy bus: - a dropped packet raised a bare IndexError from inside the vendor SDK instead of a CliError, leaking a raw traceback. Reads now convert and RETRY; writes are NEVER retried, because a failed write may in fact have landed — that ambiguity is exactly how #21 and #38 arose. OverloadError is never retried: it is a latched state, not a dropped packet. - a read immediately after an EEPROM write returned 0 while the servo genuinely held 3387. A plausible-looking wrong value is far more dangerous than an error. EEPROM writes now settle. 1242 tests pass (was 1202). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018vrVNSJ9kzhCQqwpA2e8u6
Contributor
Author
|
/agentic_review |
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
…nts (S5864, S125) S5864 — _sdk_read took 'fn: object', which Sonar correctly flags as not callable (and which needed a type: ignore[operator] to call). Typed as Callable[..., tuple[int, int, int]]; the ignore is gone. S125 x2 — Sonar read '# 251 + 100 = 351' as commented-out code. It is not, but it should go anyway, and the reason is the point of this whole PR: those comments NAME TICKS. They restate constants that exist to be re-measured, and they would go stale the next time the arm finds its own walls — which is exactly the failure mode this PR removes from the tests and the docstrings. Deleting them is consistent, not a concession. 1242 tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018vrVNSJ9kzhCQqwpA2e8u6
PR Summary by QodoFix rezero tick-frame handling and harden FeetechBus read robustness
AI Description
Diagram
High-Level Assessment
Files changed (15)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…hot of it qodo caught the sharpest possible version of this PR's own bug: the explain text CLAIMED 'this text does not repeat them (a doc that names a tick is a doc that goes stale)' — and then went on to name ticks. Raw ranges and a travel count, already stale by the time they were written, because the arm re-measured its own walls an hour later. A doc that lies about its own staleness is worse than one that is merely stale. Deleting the numbers was the easy fix; qodo suggested the better one, and it is the right one — RENDER them. _ARM_REZERO is now an f-string evaluated at import against arm_spec.REZERO_ARCS, so the operator standing at the arm gets numbers that are current BY CONSTRUCTION and physically cannot drift from the table. Change the margin or re-measure a wall, and # arm101-cli arm rezero <joint> Shift a joint's **encoder zero** — the servo's `Ofs` / `Homing_Offset` register (EEPROM addr 31) — so that the encoder's 4095->0 **seam** falls inside the arc the joint physically cannot reach. **Commands no motion, on any path.** ## The bug this fixes (issue #35) `elbow_flex`'s 12-bit encoder wraps *inside its own physical travel*. Driven far enough it crosses the raw 4095->0 seam and reads back near zero, so its reported position is **not monotonic with joint angle**: its two measured endpoints sort into a `[min, max]` pair describing exactly the arc it CANNOT reach, and every position comparison in this codebase — `gentle_move`'s arrival check, `clamp_goal`, the reachability map's ranges — is silently wrong for it. It currently rests at raw ~126, i.e. *past* its wrap. Move the seam into the joint's unreachable arc and every tick it can actually reach lies on one side of it. The tick axis is linear again — genuinely, not by assumption. ## Why it commands no motion — the bootstrap problem The tool that MAKES the axis linear cannot itself rely on the axis being linear. "Drive the joint to mid-travel, then centre it" is the natural procedure and it is exactly the one that must not run: from its rest position at raw ~126, a linear goal of 3121 (its mid-travel) looks like a modest move and is in fact a rotation *the long way round* — down through 0, across the whole 1894-tick arc the joint cannot reach, and into a wall. So this verb reads where the joint physically **is**, computes the offset from the joint's known unreachable arc (a measured table fact, in `arm_spec.REZERO_ARCS`), and writes it. No goal position is ever written. Torque is disabled before the EEPROM write and left off: a servo must not be *holding* while its own frame of reference changes underneath it. ## Which joints Only `elbow_flex`. Every other joint is refused **with the reason** — and there are two different reasons, which the verb keeps apart: - **`wrist_roll` — impossible.** A re-zero only *relocates* a seam; it can never *evict* one. Eviction needs an arc the joint cannot reach, and exploration found no wall anywhere in `wrist_roll`'s travel (measured free range `[21, 4073]`) — it turns freely all the way round, so every angle is reachable, including whichever one the seam is moved to. It is handled instead by a software **soft limit** (`arm_spec.SOFT_LIMITS`), already in force. - **The other four — unnecessary.** Their encoders do not wrap inside their travel, so there is no seam in the way and nothing to evict. ## `--verify` — the seam-eviction proof **Reading the offset back only proves it was APPLIED. It does not prove the seam MOVED.** One undocumented bit of firmware semantics decides which: Present = (raw - Ofs) mod 4096 seam RELOCATES -> the fix works Present = raw - Ofs (signed) seam STAYS -> the fix does NOTHING Every source (and LeRobot's shipped SO-101 calibration) implies the first; no primary Feetech source states it. `--verify` settles it: torque goes **off** and stays off, a **human hand-moves** the joint through its entire travel, and the verb polls `present_position` and asserts there is **no discontinuity anywhere**. A human arm is the right instrument precisely because it is the only actuator available that does not need a linear tick axis to work. It reports the range reached, whether the sweep was monotonic, and the largest single-sample jump — a seam crossing is ~1949-4095 ticks; sensor noise and a human hand are tens. It also measures `elbow_flex`'s **far wall** for the first time (nothing could see across the seam before). Four verdicts, because "did not fail" is not the same claim as "proved it works": - `seam-evicted` — re-zeroed, continuous, and the sweep actually covered the travel. The fix works. - `seam-not-evicted` — re-zeroed and **still** discontinuous. **STOP.** The re-zero achieves nothing; exit code 2, and the decision goes back to the user. - `seam-present-baseline` — not re-zeroed, discontinuous. The bug, photographed. Expected before the write; not a failure. - `inconclusive` — continuous, but either no offset was in force or the joint was not moved through enough of its travel for "no seam" to mean anything. `--verify` deliberately ends with the joint **limp** — the operator's hand is on it. If the arm is holding a pose it will sag: support it. ## Consent modes Same three-mode gate as `arm flex` / `arm explore` (1-step tier): 1. **TTY (interactive)** — confirm at a prompt. 2. **Non-TTY without `--apply`** — dry-run: prints the exact register writes and opens **no bus at all**. 3. **Non-TTY with `--apply`** — executes. ## Usage arm101-cli arm rezero elbow_flex # dry-run: the exact writes arm101-cli arm rezero elbow_flex --apply # write the offset arm101-cli arm rezero elbow_flex --verify --apply # prove the seam moved arm101-cli arm rezero elbow_flex --verify --duration 45 --apply arm101-cli arm rezero wrist_roll # refused, with the reason arm101-cli arm rezero elbow_flex --json ## After the write — what is NOT yet proven The read-back proves the offset was **applied**, not that it **persists**: PR #21 exists because id/baud EEPROM writes read back correctly and silently reverted on the next power-cycle. **Power-cycle the servo** (cut and restore bus power, not just the serial link), re-read with `arm read`, then run `--verify`. The full hand-run procedure is in `docs/hardware-rezero-procedure.md`. ## Exit codes - `0` success, clean abort, a non-TTY dry-run plan, or an informative sweep (baseline / inconclusive). - `1` user/usage error (an unknown joint, a joint that cannot be re-zeroed, a `--duration` too short to collect two samples). - `2` environment error (no port, SDK absent, comms failure), the offset failing to read back, the servo holding an unrecognised offset, the joint reporting a raw position inside its own unreachable arc — **and the `seam-not-evicted` verdict**, which is a stop condition, not a retryable error. ## Hardware / TTY behavior Requires a real motor bus and the Feetech SDK (the `[seeed]` extra). The result and the sweep report go to stdout; the prompt, the live sample feed, and every warning go to stderr. changes with it. Acid test: margin 100 -> explain says (351, 1961); margin 300 -> explain says (551, 1761). It tracks. Also removed the last stale literals: the ~126 rest position (the operator has since moved the arm), the 1900-tick arc width, and the 2196-tick sweep length (the arm's own wall-find measured 2286). 1242 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018vrVNSJ9kzhCQqwpA2e8u6
Contributor
Author
|
FIX — and this is the sharpest possible catch, because it is this PR's own thesis, violated in this PR's own text. The explain entry literally says:
…and then repeats them. Raw ranges and a travel count. A doc that lies about its own staleness is worse than one that is merely stale, and these were already wrong: an hour after I wrote them the arm re-measured its own walls ( Deleting the numbers was the easy fix. Your second suggestion is the better one and I took it: render them. Acid test, run both ways: It tracks. Same guarantee the tests now have (they derive from Also swept out the last three literals you didn't flag but which had the same rot: the Shipped in
|
|
This was referenced Jul 12, 2026
spec+plan: the arm measures its own joint travel — rolling the encoder seam out of the way (#43)
#44
Open
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
1 participant
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.
Refs #35. A live hardware session on the follower, operator present throughout.
The result: the re-zero works, and the spike's caveat is closed by measurement
The STS3215 reduces the corrected position modulo 4096 — a homing offset genuinely
relocates the encoder seam rather than merely relabelling positions. That was the one
unproven assumption the whole of issue #35 rested on. The t5 spike
said GO-WITH-CAVEAT precisely because no document anywhere states it. Now it's measured.
Same joint, same hand sweep, torque off, before and after:
And it survived a power cycle, read back cold. PR #21's ghost is laid.
We found the mechanism, not just the symptom
Every SO-101 ships with
Ofs = +85— not the factory0the spike assumed. (Uniformacross all six servos, so it's a vendor default, not a calibration.)
That is why
elbow_flexwraps. The seam sits whereActual == Ofs— at raw 85 — whilethe unreachable arc doesn't begin until raw 251. The seam was sitting inside the joint's
travel, by 166 ticks. Nothing in the issue, the spike, or the code knew that.
The frame bug
REZERO_ARCSheld reported ticks — read off a servo already carrying that factory offset— and used them as raw. The old target landed inside the true arc anyway, with ~866 ticks
of margin, so it worked and every read-back looked correct. That is the most dangerous way
for a frame bug to behave.
The arc is now raw by construction, and
raw = (reported + offset) mod 4096on every path.The
current in (0, target)guard is gone. It refused outright on a servo holding thefactory 85 — i.e. on every fresh SO-101. The goal is now stated as a place (the seam is
out of the travel), not a magic number: an arm already holding a seam-evicting offset is a
clean no-op, not a rewrite that spends an EEPROM cycle to slide a seam from one unreachable
tick to another.
The arm measured its own walls — better than the human did
Once the axis was linear,
gentle_movecould be driven past the known travel and left tofind each wall by feel: creep under torque, watch the load, stop on contact, back off, hold.
The second one is the whole day's work in a single move. Commanding 3400 crosses the raw
4095→0 boundary — and in the corrected frame it's a plain linear climb, which converged. That
exact command would have rotated the elbow the long way round into a wall yesterday.
A human pushes until it feels firm — successive sweeps gave 206, then 218. The arm presses
to a fixed load every time, so its walls are further out and repeatable. Both contacts
saturated at the 500 torque cap: the signature of a real wall, not of an operator's judgement.
The arc is inset from those walls, so a harder push can never make the table contradict the
arm — which is exactly what a tight arc did, within minutes of being declared.
The tests stopped copying the table
~125 arc-coupled literals were hard-coded across two test files, so re-measuring a table that
exists to be re-measured broke 34 tests. They now derive every expectation from
arm_spec.Acid test: change
_ARC_MARGIN_TICKSor either wall and the suite stays green.Same rule now applies to prose — the docstrings and the
explaincatalog no longer quoteticks at all. A doc that names a measurement is a doc that goes stale, and
explain arm rezerois what prints to whoever is standing at the arm.
Two bus bugs, both hit live on a healthy bus
IndexErrorfrom inside the vendor SDK instead of aCliError, leaking a raw traceback and violating the repo's contract. Reads now convert andretry; writes are never retried, because a failed write may in fact have landed — that
ambiguity is exactly how fix: persist EEPROM id/baud writes by opening the Lock register #21's Lock bug and feat: motion verbs release the arm on any abnormal exit (closes #33) #38's id-transfer bug arose.
OverloadErrorisnever retried: it's a latched state, not a dropped packet.
0while the servo genuinely held 3387.A plausible-looking wrong value is far more dangerous than an error. EEPROM writes now settle.
Gates
1242 tests pass (1202 on main). black / isort / flake8 / bandit clean.
teken cli doctor --strictpasses. Run-log:docs/hardware-rezero-run-2026-07-12.md.