From 122aefd3177abccb2253454f7af2e5bce974e12c Mon Sep 17 00:00:00 2001 From: Hongquan Li Date: Mon, 18 May 2026 01:06:01 -0700 Subject: [PATCH 1/2] feat: enable encoder closed-loop PID for filter wheel W/W2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- firmware/controller/src/commands/commands.cpp | 4 ++-- .../squid/filter_wheel_controller/cephla.py | 3 ++- software/tests/squid/test_filter_wheel.py | 17 +++++++++++++++-- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/firmware/controller/src/commands/commands.cpp b/firmware/controller/src/commands/commands.cpp index fdab2a570..262fa525c 100644 --- a/firmware/controller/src/commands/commands.cpp +++ b/firmware/controller/src/commands/commands.cpp @@ -132,11 +132,11 @@ void callback_configure_stage_pid() tmc4361A_init_PID(&tmc4361[axis], 25, 25, axes_pid_arg[axis].p, axes_pid_arg[axis].i, axes_pid_arg[axis].d, tmc4361A_vmmToMicrosteps(&tmc4361[axis], MAX_VELOCITY_Z_mm), 4096, 2); else if (axis == w) { if (enable_filterwheel == true) - tmc4361A_init_PID(&tmc4361[axis], 2, 2, axes_pid_arg[axis].p, axes_pid_arg[axis].i, axes_pid_arg[axis].d, tmc4361A_vmmToMicrosteps(&tmc4361[axis], MAX_VELOCITY_W_mm), 4096, 2); + tmc4361A_init_PID(&tmc4361[axis], 20, 20, axes_pid_arg[axis].p, axes_pid_arg[axis].i, axes_pid_arg[axis].d, tmc4361A_vmmToMicrosteps(&tmc4361[axis], MAX_VELOCITY_W_mm), 4096, 2); } else if (axis == w2) { if (enable_filterwheel_w2 == true) - tmc4361A_init_PID(&tmc4361[axis], 2, 2, axes_pid_arg[axis].p, axes_pid_arg[axis].i, axes_pid_arg[axis].d, tmc4361A_vmmToMicrosteps(&tmc4361[axis], MAX_VELOCITY_W_mm), 4096, 2); + tmc4361A_init_PID(&tmc4361[axis], 20, 20, axes_pid_arg[axis].p, axes_pid_arg[axis].i, axes_pid_arg[axis].d, tmc4361A_vmmToMicrosteps(&tmc4361[axis], MAX_VELOCITY_W_mm), 4096, 2); } } diff --git a/software/squid/filter_wheel_controller/cephla.py b/software/squid/filter_wheel_controller/cephla.py index d332c9e48..e13ce1e22 100644 --- a/software/squid/filter_wheel_controller/cephla.py +++ b/software/squid/filter_wheel_controller/cephla.py @@ -120,7 +120,8 @@ def _configure_wheel(self, wheel_id: int, config: SquidFilterWheelConfig): if HAS_ENCODER_W: self.microcontroller.set_pid_arguments(axis, PID_P_W, PID_I_W, PID_D_W) self.microcontroller.configure_stage_pid(axis, config.transitions_per_revolution, ENCODER_FLIP_DIR_W) - self.microcontroller.turn_on_stage_pid(axis, ENABLE_PID_W) + if ENABLE_PID_W: + self.microcontroller.turn_on_stage_pid(axis) @staticmethod def _delta_to_usteps(delta_mm: float) -> int: diff --git a/software/tests/squid/test_filter_wheel.py b/software/tests/squid/test_filter_wheel.py index 9ed8da461..5be23c49c 100644 --- a/software/tests/squid/test_filter_wheel.py +++ b/software/tests/squid/test_filter_wheel.py @@ -4,6 +4,7 @@ import squid.config import squid.filter_wheel_controller.utils +from control._def import AXIS from squid.config import FilterWheelConfig, FilterWheelControllerVariant, SquidFilterWheelConfig from squid.filter_wheel_controller.cephla import SquidFilterWheel @@ -121,13 +122,25 @@ def test_normal_init_calls_mcu_initialization(self, mock_microcontroller, squid_ mock_microcontroller.configure_squidfilter.assert_called_once() @patch("squid.filter_wheel_controller.cephla.HAS_ENCODER_W", True) + @patch("squid.filter_wheel_controller.cephla.ENABLE_PID_W", True) def test_normal_init_configures_encoder_pid(self, mock_microcontroller, squid_config): - """skip_init=False with HAS_ENCODER_W=True should configure encoder PID.""" + """HAS_ENCODER_W=True + ENABLE_PID_W=True should configure and engage PID.""" SquidFilterWheel(mock_microcontroller, squid_config, skip_init=False) mock_microcontroller.set_pid_arguments.assert_called_once() mock_microcontroller.configure_stage_pid.assert_called_once() - mock_microcontroller.turn_on_stage_pid.assert_called_once() + # turn_on_stage_pid takes axis only — passing extra args would TypeError. + mock_microcontroller.turn_on_stage_pid.assert_called_once_with(AXIS.W) + + @patch("squid.filter_wheel_controller.cephla.HAS_ENCODER_W", True) + @patch("squid.filter_wheel_controller.cephla.ENABLE_PID_W", False) + def test_encoder_without_pid_skips_turn_on(self, mock_microcontroller, squid_config): + """HAS_ENCODER_W=True + ENABLE_PID_W=False configures encoder but leaves PID off.""" + SquidFilterWheel(mock_microcontroller, squid_config, skip_init=False) + + mock_microcontroller.set_pid_arguments.assert_called_once() + mock_microcontroller.configure_stage_pid.assert_called_once() + mock_microcontroller.turn_on_stage_pid.assert_not_called() class TestSquidFilterWheelAbsoluteMove: From c54b009c2af57ef93fc16de32652199623fb4f47 Mon Sep 17 00:00:00 2001 From: Hongquan Li Date: Mon, 18 May 2026 01:15:33 -0700 Subject: [PATCH 2/2] test: parametrize encoder-PID tests over W and W2 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. --- software/tests/squid/test_filter_wheel.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/software/tests/squid/test_filter_wheel.py b/software/tests/squid/test_filter_wheel.py index 5be23c49c..99ffa5161 100644 --- a/software/tests/squid/test_filter_wheel.py +++ b/software/tests/squid/test_filter_wheel.py @@ -121,22 +121,29 @@ def test_normal_init_calls_mcu_initialization(self, mock_microcontroller, squid_ mock_microcontroller.init_filter_wheel.assert_called_once() mock_microcontroller.configure_squidfilter.assert_called_once() + @pytest.mark.parametrize( + "motor_slot,expected_axis", + [pytest.param(3, AXIS.W, id="W"), pytest.param(4, AXIS.W2, id="W2")], + ) @patch("squid.filter_wheel_controller.cephla.HAS_ENCODER_W", True) @patch("squid.filter_wheel_controller.cephla.ENABLE_PID_W", True) - def test_normal_init_configures_encoder_pid(self, mock_microcontroller, squid_config): + def test_normal_init_configures_encoder_pid(self, mock_microcontroller, motor_slot, expected_axis): """HAS_ENCODER_W=True + ENABLE_PID_W=True should configure and engage PID.""" - SquidFilterWheel(mock_microcontroller, squid_config, skip_init=False) + config = _make_squid_config(motor_slot=motor_slot) + SquidFilterWheel(mock_microcontroller, config, skip_init=False) mock_microcontroller.set_pid_arguments.assert_called_once() mock_microcontroller.configure_stage_pid.assert_called_once() # turn_on_stage_pid takes axis only — passing extra args would TypeError. - mock_microcontroller.turn_on_stage_pid.assert_called_once_with(AXIS.W) + mock_microcontroller.turn_on_stage_pid.assert_called_once_with(expected_axis) + @pytest.mark.parametrize("motor_slot", [3, 4], ids=["W", "W2"]) @patch("squid.filter_wheel_controller.cephla.HAS_ENCODER_W", True) @patch("squid.filter_wheel_controller.cephla.ENABLE_PID_W", False) - def test_encoder_without_pid_skips_turn_on(self, mock_microcontroller, squid_config): + def test_encoder_without_pid_skips_turn_on(self, mock_microcontroller, motor_slot): """HAS_ENCODER_W=True + ENABLE_PID_W=False configures encoder but leaves PID off.""" - SquidFilterWheel(mock_microcontroller, squid_config, skip_init=False) + config = _make_squid_config(motor_slot=motor_slot) + SquidFilterWheel(mock_microcontroller, config, skip_init=False) mock_microcontroller.set_pid_arguments.assert_called_once() mock_microcontroller.configure_stage_pid.assert_called_once()