Sakshi pid controller and AK80 motor

[sakshishirole01]

Summary

Describe the main changes in this PR:

---

Code Quality Checklist

Before requesting review, ensure:

• All CI/CD pipelines pass
• Code includes docstrings and type hints
• I added new tests where necessary
• I ran make tree to update the README.md

---

Additional Notes

• Anything else reviewers should know?
• Attach outputs, plots, logs, or GIFs here.

[NGierden]

@claude-review

[claude]

Claude finished @NGierden's task in 2m 50s —— View job

---

Code Review — PR #60: Sakshi PID Controller and AK80 Motor

Tasks:

• Load review tools
• Read changed files and git diff
• Review new files (PID controller, second-order LPF, motor control)
• Review modified files
• Post comprehensive review feedback

---

Overview

This PR adds a PID position controller, a second-order low-pass filter with pluggable integration strategies (Forward Euler, Backward Euler, Trapezoidal, RK4), and support for the AK80-6 motor variant. The structure is clean and the LPF implementation is well-documented, but there are several bugs that need fixing before merge.

---

🐛 Bugs

1. Swapped return values in SecondOrderLowPassFilter.run() [second_order_low_pass_filter.py:251]

step() is documented to return (filtered_output, output_derivative), but run() unpacks them in reverse order:

# step() returns (filtered_output, output_derivative)
y_dot, y = self.step(x=x, time_difference=dt)   # ← swapped!
y_array.append(y)       # actually gets output_derivative
y_dot_array.append(y_dot)  # actually gets filtered_output

The run() public API therefore returns (output_derivative_array, filtered_signal_array) — the opposite of what its docstring says. Any caller of run() will get wrong values. Fix this →

2. Class attribute access instead of instance attribute [motor_control_using_pid.py:78]

MAX_VELOCITY_RAD_S = PIDConfig.output_limits   # ← reads dataclass default, not instance value

PIDConfig.output_limits accesses the dataclass field default, not the configured instance's value. If PIDMotorController ever uses a custom PIDConfig with different limits, this will silently apply the wrong clamp. Should be self.pid.config.output_limits. Fix this →

3. Stale docstring parameter [motor_control_using_pid.py:46]

The move_to() docstring references :param velocity_rad_s: which doesn't exist in the method signature. Fix this →

4. Redundant error calculation in move_to() [motor_control_using_pid.py:63-76]

error is computed manually at line 64, and then compute_output() recomputes it internally. The early-exit guard abs(error) < 0.01 is fine, but velocity_cmd also has the output re-clamped at line 79 even though compute_output() already clamps to output_limits. The double-clamp is harmless but unnecessary.

---

⚠️ Design Issues

5. Dead PID instance embedded in CubeMarsAK606v3CAN [cube_mars_motor_can.py:174-184]

A PIDController is instantiated in CubeMarsAK606v3CAN.__init__() with a _pid_target_deg attribute, but no method in CubeMarsAK606v3CAN ever calls self.pid.compute_output() or reads self._pid_target_deg. This is dead code that adds memory overhead to every motor instance. The PID belongs in PIDMotorController, not in the low-level CAN driver. Fix this →

6. CubeMarsAK806v2 sets _motor_spec before calling super().__init__() but the UART parent never reads it [cube_mars_motor.py:430-447]

The UART CubeMarsAK606v3 class uses MOTOR_LIMITS and protocol constants from definitions.py, not self._motor_spec. Setting self._motor_spec = AK80_6_MOTOR_SPEC in the subclass before super().__init__() has no effect — the parent class doesn't reference it. The AK80-6 UART subclass therefore doesn't actually change any motor-specific behavior.

7. Hardcoded PID gains in PIDMotorController.__init__() [motor_control_using_pid.py:23-32]

The gains kp=2.0, ki=0.0, kd=0.3 are hardcoded in the constructor with no way to pass custom values, making the class hard to tune without subclassing or modifying the source. Consider accepting an optional PIDConfig parameter. Fix this →

8. hold() has no stop mechanism [motor_control_using_pid.py:100-109]

def hold(self, target_degrees: float, kp: float = 20.0, kd: float = 1.0):
    while True:
        self.motor.set_mit_mode(...)
        time.sleep(0.01)

This blocks forever with no cancellation mechanism other than KeyboardInterrupt. It also doesn't call self.stop() on exit.

9. No zero-dt guard in PIDController.compute_output() [pid_controller.py:68]

If time_difference <= 0 (timestamps repeat or go backwards), the integral update self._integral += error * 0 is harmless, but the LPF step() will receive time_difference=0, which silently produces an unstepped output. A guard or assertion would make this safer.

---

📝 Code Quality

10. PIDConfig.output_limits default uses unusual tuple syntax [definitions.py:451]

output_limits: "tuple[float, float] | None" = -7, 7

Two issues: (a) the value -7, 7 is valid Python but unusual without parentheses — prefer (-7, 7); (b) the type is written as a string annotation even though from __future__ import annotations is not imported here — the string quotes are unnecessary. Fix this →

11. MAX_ERROR recomputed every call [pid_controller.py:61]

MAX_ERROR = np.radians(60)  # computed inside compute_output() on every call

This should be a module-level constant since it never changes.

12. motor parameter type annotation dropped in verify_set_velocity.py

read_status(), _command_phase(), and run_phase() all had their motor type annotations removed (replaced with bare motor). Use BaseMotor or a Protocol to keep the type-checked behavior. The commented-out isinstance check at line 291 is also dead code that should be removed.

13. Missing tests

No unit tests were added for PIDController, SecondOrderLowPassFilter, or PIDMotorController. Given the multiple solver strategies and the bug discovered in run() (#1 above), tests are particularly important here.

14. Blank line formatting in pid_motor_test.py:116

There is an extra blank line between the communication check and the comment block, leaving inconsistent spacing.

---

✅ What's Good

• The SecondOrderLowPassFilter strategy pattern is clean and well-documented. Four integration methods are clearly explained, and the public API (step, run, reset) is well-specified.
• The AK80-6 support is a natural extension — MotorSpec, MITModeLimits, and the CubeMarsAK806v2CAN subclass are all minimal and correct.
• erpm_to_degrees_per_second in utils.py is now properly parameterized with MotorSpec instead of hardcoded CAN defaults — good fix.
• The motor model selector in __main__.py and verify_set_velocity.py follows the existing CLI patterns cleanly.
• run() in SecondOrderLowPassFilter correctly resets state before each call, making it safe for reproducible offline analysis.

---

Summary

Severity	Count
🐛 Bug (must fix before merge)	3
⚠️ Design issue	5
📝 Code quality	6

The most critical issues are the swapped return values in run() (#1), the class-level attribute access for output_limits (#2), and the dead PID instance in CubeMarsAK606v3CAN (#5). The PR checklist also shows no tests were added — given the complexity of the LPF, at minimum a test verifying that the four solvers produce monotonically converging outputs for a step input would be valuable.

[NGierden]

ignore the unneeded comments of claude, such as the embedding of the PID. Otherwise check Claude's comments please and change what is needed