feat: enable encoder closed-loop PID for filter wheel W/W2#542
Open
hongquanli wants to merge 2 commits into
Open
feat: enable encoder closed-loop PID for filter wheel W/W2#542hongquanli wants to merge 2 commits into
hongquanli wants to merge 2 commits into
Conversation
Fixes a latent bug and tunes the existing-but-dormant W/W2 closed-loop path so it can actually be enabled via per-machine INI flags. - cephla.py: turn_on_stage_pid() takes axis only, but the call site passed (axis, ENABLE_PID_W). This was a TypeError waiting to fire the moment HAS_ENCODER_W=True. Gate the call on ENABLE_PID_W instead and drop the extra arg. - commands.cpp: widen W/W2 PID target_tolerance from 2 → 20 microsteps (≈0.9° physical with 4000 transitions/rev, 8 slots). 2 microsteps is overkill for slot-level filter selection and risks dither at rest. - test_filter_wheel.py: patch ENABLE_PID_W=True in the existing PID test and add a new test for the HAS_ENCODER_W=True / ENABLE_PID_W=False case (encoder configured, PID disabled). Assert the exact call signature to guard against the regression. Enabling the closed loop on a machine still requires setting has_encoder_w = True and enable_pid_w = True under [GENERAL] in the machine .ini, plus bench-verifying transitions_per_revolution and ENCODER_FLIP_DIR_W. Stacks on top of #540 (firmware reports failed moves; absolute MOVETO for filter wheel). #540 catches callback-level silent ack failures; this PR adds in-motion step-loss correction and idle-drift holding via the encoder, addressing the detent-less filter wheel's no-error- accumulation requirement.
Both encoder/PID configuration tests previously ran only against the default motor_slot=3 (W). Parametrize them over W and W2 so the W2 init path is also pinned against the turn_on_stage_pid signature regression. Mirrors the AXIS_PARAMS pattern used by the adjacent TestSquidFilterWheelAbsoluteMove class.
Contributor
There was a problem hiding this comment.
Pull request overview
Enables the previously-dormant W/W2 encoder closed-loop PID path on the filter wheel and fixes a latent host-side bug where turn_on_stage_pid was called with too many arguments. Firmware tolerance values are widened from 2→20 microsteps to avoid PID dither at rest while remaining well within slot-selection accuracy.
Changes:
- Firmware: widen W/W2
target_tolerance/pid_tolerancefrom 2 to 20 microsteps intmc4361A_init_PID. - Host: drop extra
ENABLE_PID_Warg fromturn_on_stage_pidcall and gate the call onENABLE_PID_W, allowing encoder-only configurations. - Tests: parametrize the PID-enable test over W/W2 with
assert_called_once_with(expected_axis), plus a new test forHAS_ENCODER_W=True / ENABLE_PID_W=False.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| firmware/controller/src/commands/commands.cpp | Widen W/W2 PID tolerances 2→20 microsteps. |
| software/squid/filter_wheel_controller/cephla.py | Fix turn_on_stage_pid arity bug; gate enabling on ENABLE_PID_W. |
| software/tests/squid/test_filter_wheel.py | Parametrize PID test over W/W2; add encoder-without-PID test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Summary
Enables the existing-but-dormant W/W2 encoder closed-loop path so the filter wheel physically reaches its commanded slot every move, with no error accumulation over many moves. Stacks on top of #540.
Why
The Cephla filter wheel has no detent — the motor itself must hold each slot position. With open-loop control and no encoder feedback (current default), two failure modes are not covered by #540:
CMD_EXECUTION_ERRORdoesn't catch this because the callback executed fine.Enabling the TMC4361A's hardware PID loop (already wired in firmware for X/Y/Z) closes both gaps. PID continuously drives
ENC_POS → X_TARGETduring the move and holds position at rest. Position error cannot accumulate because every commanded position is referenced to encoder counts, not to the previous (potentially drifted) XACTUAL.What changed
Firmware
commands.cpp:135,139— Widen W/W2target_tolerance/pid_tolerancefrom2 → 20microsteps. Tolerance is in microstep units (XACTUAL vs ENC_POS, after the chip scales encoder counts to match). With 200 fullsteps/rev × 64 microstepping = 12,800 microsteps/rev, 20 microsteps ≈ 0.56° physical angle — well inside the 45°-per-slot accuracy needed for filter selection. The previous value of 2 (≈0.056°) was overkill for slot-level positioning and risked PID dither at rest.Host
cephla.py:120-124— Bug fix.Microcontroller.turn_on_stage_pid(self, axis)takes a single positional arg, but the call site passed(axis, ENABLE_PID_W). This was a latentTypeErrorthat would fire the instantHAS_ENCODER_W=True. Fixed by dropping the extra arg and gating the call onif ENABLE_PID_W:— soHAS_ENCODER_W=True, ENABLE_PID_W=Falseis now a valid configuration (configure encoder for diagnostics, but leave PID disengaged).Tests
test_filter_wheel.py— PatchENABLE_PID_W=Truein the existing PID-enable test so it still exercises the call, and assertassert_called_once_with(expected_axis)to guard against the old(axis, ENABLE_PID_W)signature regression. Add a new test for theHAS_ENCODER_W=True / ENABLE_PID_W=Falsecell. Both tests are parametrized over W (motor_slot=3) and W2 (motor_slot=4), matching theAXIS_PARAMSpattern used by the adjacentTestSquidFilterWheelAbsoluteMoveclass.Bench validation (out of PR scope, machine-side)
Enabling on a real machine requires three machine-config / verification steps that don't belong in this PR:
has_encoder_w = Trueandenable_pid_w = Trueunder[GENERAL]in the machine.ini. The existing override loader incontrol/_def.pypicks these up.transitions_per_revolution = 4000(_def.py:1080,1087) matches the actual encoder CPR. If not, override per wheel in the multi-wheel YAML.MOVE_W, readENC_POSvia SPI/tools. If encoder direction is opposite to commanded delta, flipENCODER_FLIP_DIR_W = Truein_def.py:695.Relationship to #540
Complementary, not overlapping:
CMD_EXECUTION_ERRORENC_POS#540's design is "containment via absolute addressing + report errors honestly". This PR adds "active correction via encoder", which is what a detent-less filter wheel actually needs.
Test plan
pytest tests/squid/test_filter_wheel.py— 28 passed (24 existing + 4 parametrized: PID-on × {W, W2} and PID-off × {W, W2})black --config pyproject.toml --checkclean on edited Python filestransitions_per_revolutionandENCODER_FLIP_DIR_Wempiricallytarget_tolerancefurther before changing PID gains🤖 Generated with Claude Code