Skip to content

Commit d81b91e

Browse files
Alpaca233claude
andcommitted
fix: address code review findings in modbus_rtu and servo_motor
- Don't retry Modbus exception responses (application-level errors) - Fix jog() far-position sign inversion - Replace magic hex constants with ControlWordBits expressions - Add abort checks to all polling loops (_wait_for_state/motion/homing) - Add bounds checking to move_relative() in real ServoMotor class - abort() now calls quick_stop() on all axes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 3fe61f3 commit d81b91e

2 files changed

Lines changed: 38 additions & 17 deletions

File tree

software/fluidics/control/modbus_rtu.py

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -196,17 +196,6 @@ def _send_receive(self, frame: bytes, expected_response_len: int) -> bytes:
196196
if not _verify_crc(response):
197197
raise ModbusError("CRC verification failed", slave_id=frame[0])
198198

199-
# Check for exception response
200-
if response[1] & 0x80:
201-
exception_code = response[2]
202-
raise ModbusError(
203-
f"Modbus exception response: FC=0x{response[1]:02X}, "
204-
f"code=0x{exception_code:02X}",
205-
slave_id=response[0],
206-
)
207-
208-
return response
209-
210199
except ModbusError as e:
211200
last_error = e
212201
logger.warning(
@@ -217,6 +206,17 @@ def _send_receive(self, frame: bytes, expected_response_len: int) -> bytes:
217206
time.sleep(FRAME_INTERVAL * 2)
218207
continue
219208

209+
# Exception responses are application-level errors — don't retry
210+
if response[1] & 0x80:
211+
exception_code = response[2]
212+
raise ModbusError(
213+
f"Modbus exception response: FC=0x{response[1]:02X}, "
214+
f"code=0x{exception_code:02X}",
215+
slave_id=response[0],
216+
)
217+
218+
return response
219+
220220
raise last_error
221221

222222
def __enter__(self) -> "ModbusRTUClient":

software/fluidics/control/servo_motor.py

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -373,6 +373,8 @@ def _get_state(self, slave_id: int) -> DriveState:
373373
def _wait_for_state(self, slave_id: int, target: DriveState, timeout: float):
374374
start = time.time()
375375
while time.time() - start < timeout:
376+
if self.is_aborted:
377+
raise RuntimeError("Operation aborted")
376378
state = self._get_state(slave_id)
377379
if state == target:
378380
return
@@ -388,6 +390,8 @@ def _wait_for_state(self, slave_id: int, target: DriveState, timeout: float):
388390
def _wait_for_motion(self, slave_id: int, timeout: float):
389391
start = time.time()
390392
while time.time() - start < timeout:
393+
if self.is_aborted:
394+
raise RuntimeError("Operation aborted")
391395
status_word = self._read_status_word(slave_id)
392396
if status_word & StatusWordBits.FAULT:
393397
error = self._client.read_register(slave_id, Reg.ERROR_CODE)
@@ -400,6 +404,8 @@ def _wait_for_motion(self, slave_id: int, timeout: float):
400404
def _wait_for_homing(self, slave_id: int, timeout: float):
401405
start = time.time()
402406
while time.time() - start < timeout:
407+
if self.is_aborted:
408+
raise RuntimeError("Operation aborted")
403409
status_word = self._read_status_word(slave_id)
404410
if status_word & StatusWordBits.FAULT:
405411
error = self._client.read_register(slave_id, Reg.ERROR_CODE)
@@ -541,6 +547,15 @@ def move_relative(
541547
):
542548
name, config, sid = self._resolve_axis(axis)
543549

550+
current_pulses = self._client.read_register_32bit(sid, Reg.POSITION_ACTUAL_VALUE, signed=True)
551+
current_mm = config.pulses_to_mm(current_pulses)
552+
target_mm = current_mm + distance_mm
553+
if not config.is_position_valid(target_mm):
554+
raise ValueError(
555+
f"Target position {target_mm:.2f}mm out of range "
556+
f"[{config.stroke_min}, {config.stroke_max}]"
557+
)
558+
544559
self._client.write_register(sid, Reg.MODES_OF_OPERATION, OperationMode.PROFILE_POSITION)
545560

546561
if velocity_mm_s is not None:
@@ -572,19 +587,19 @@ def jog(self, velocity_mm_s: float, axis: str | None = None):
572587
self._client.write_register(sid, Reg.MODES_OF_OPERATION, OperationMode.PROFILE_POSITION)
573588
self._client.write_register_32bit(sid, Reg.PROFILE_VELOCITY, config.velocity_mm_to_pulses(abs(velocity_corrected)))
574589

575-
far_position = -10000000 if velocity_corrected > 0 else 10000000
590+
far_position = 10000000 if velocity_corrected > 0 else -10000000
576591
self._client.write_register_32bit(sid, Reg.TARGET_POSITION, far_position, signed=True)
577592

578-
self._write_control_word(sid, 0x000F)
593+
self._write_control_word(sid, ControlWordBits.CMD_ENABLE_OPERATION)
579594
time.sleep(0.001)
580-
self._write_control_word(sid, 0x001F)
595+
self._write_control_word(sid, ControlWordBits.CMD_ENABLE_OPERATION | ControlWordBits.NEW_SET_POINT)
581596

582597
def stop(self, axis: str | None = None, wait: bool = True):
583598
_, config, sid = self._resolve_axis(axis)
584599

585600
current_pos = self._client.read_register_32bit(sid, Reg.POSITION_ACTUAL_VALUE, signed=True)
586601
self._client.write_register_32bit(sid, Reg.TARGET_POSITION, current_pos, signed=True)
587-
self._write_control_word(sid, 0x010F)
602+
self._write_control_word(sid, ControlWordBits.CMD_ENABLE_OPERATION | ControlWordBits.HALT)
588603

589604
if wait:
590605
start = time.time()
@@ -596,7 +611,7 @@ def stop(self, axis: str | None = None, wait: bool = True):
596611

597612
final_pos = self._client.read_register_32bit(sid, Reg.POSITION_ACTUAL_VALUE, signed=True)
598613
self._client.write_register_32bit(sid, Reg.TARGET_POSITION, final_pos, signed=True)
599-
self._write_control_word(sid, 0x000F)
614+
self._write_control_word(sid, ControlWordBits.CMD_ENABLE_OPERATION)
600615

601616
def home(self, axis: str | None = None, wait: bool = True):
602617
name, config, sid = self._resolve_axis(axis)
@@ -621,7 +636,7 @@ def home(self, axis: str | None = None, wait: bool = True):
621636
time.sleep(0.01)
622637
self._write_control_word(sid, ControlWordBits.CMD_ENABLE_OPERATION)
623638
time.sleep(0.01)
624-
self._write_control_word(sid, 0x001F)
639+
self._write_control_word(sid, ControlWordBits.CMD_ENABLE_OPERATION | ControlWordBits.NEW_SET_POINT)
625640

626641
if wait:
627642
self._wait_for_homing(sid, MOTION_TIMEOUT)
@@ -661,6 +676,12 @@ def set_deceleration(self, decel_mm_s2: float, axis: str | None = None):
661676

662677
def abort(self):
663678
self.is_aborted = True
679+
if self.is_connected:
680+
for name in self._axis_configs:
681+
try:
682+
self.quick_stop(name)
683+
except Exception:
684+
pass
664685

665686
def reset_abort(self):
666687
self.is_aborted = False

0 commit comments

Comments
 (0)