changed duty cycle logic to be inverted #5
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a PID temperature controller and updates the GPIO repository to handle hardware pull-up configuration. The main changes involve adding a new PID controller implementation and inverting the duty cycle to accommodate pull-up resistor configurations in the hardware.
- New PID controller implementation for temperature control with PWM output
- Duty cycle inversion logic added to handle pull-up hardware configuration
- Multiple configuration update mechanisms (terminal, API, file) for runtime parameter adjustment
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 15 comments.
| File | Description |
|---|---|
| apps/temperature-control/PIDController2.py | New PID controller implementation with multi-threaded configuration updates and GPIO PWM control for temperature regulation |
| apps/heater-api/repositories/gpio.py | Added duty cycle inversion (100 - dc) in set_duty_cycle method to compensate for pull-up hardware configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def PID_update(self, measurement): | ||
| error = self.setpoint - measurement | ||
| self.integral += error | ||
| derivative = error - self.last_error | ||
| output = (self.Kp * error) + (self.Ki * self.integral) + \ | ||
| (self.Kd * derivative) |
There was a problem hiding this comment.
The PID controller implementation does not account for time deltas between measurements. The integral and derivative terms should be scaled by the time elapsed (dt) between updates for accurate control. Current implementation assumes constant update intervals, which may not be true if the loop execution time varies.
| f"New setpoint: {new_config['setpoint']}, Kp: {new_config['Kp']}, Ki: {new_config['Ki']}, Kd: {new_config['Kd']}") | ||
| PIDController.current_temp = new_config['current_temp'] | ||
| except ValueError: | ||
| print("Invalid.") |
There was a problem hiding this comment.
The error message "Invalid." is too vague. It should specify what input was invalid (e.g., "Invalid input: Please enter numeric values for PID parameters").
| print("Invalid.") | |
| print("Invalid input: Please enter numeric values for setpoint, Kp, Ki, Kd, and current temperature.") |
| def set_setpoint(self, setpoint): | ||
| self.setpoint = setpoint | ||
| self.integral = 0 | ||
| self.last_error = 0 | ||
|
|
||
| def set_coefficients(self, Kp, Ki, Kd): | ||
| self.Kp = Kp | ||
| self.Ki = Ki | ||
| self.Kd = Kd |
There was a problem hiding this comment.
Race condition: The PID controller instance variables (setpoint, integral, last_error, Kp, Ki, Kd) are modified by the config update thread and read by the main control loop without synchronization. This can lead to inconsistent state during PID calculations. Use a threading.Lock to protect access to these shared variables.
| class PIDController: | ||
| PWM_PIN = 18 | ||
| PWM_FREQUENCY = 0.2 # Hz | ||
| BASE_URL = "http://localhost:8001" | ||
|
|
||
| # For Testing without actual Lakeshore device | ||
| current_temp = 7 | ||
|
|
||
| def __init__(self, Kp, Ki, Kd, setpoint): | ||
| self.Kp = Kp | ||
| self.Ki = Ki | ||
| self.Kd = Kd | ||
| self.setpoint = setpoint | ||
| self.integral = 0 | ||
| self.last_error = 0 | ||
|
|
||
| def PID_update(self, measurement): | ||
| error = self.setpoint - measurement | ||
| self.integral += error | ||
| derivative = error - self.last_error | ||
| output = (self.Kp * error) + (self.Ki * self.integral) + \ | ||
| (self.Kd * derivative) | ||
| self.last_error = error | ||
| return output |
There was a problem hiding this comment.
Missing documentation for the PIDController class and its methods. The class lacks docstrings explaining the purpose, parameters, and behavior of the PID controller, making it difficult for other developers to understand the implementation details, expected units, and usage patterns.
| def update_config_from_api(pid): | ||
| while True: | ||
| try: | ||
| response = requests.get(f"{pid.BASE_URL}/api/temp/parameters") | ||
| if response.status_code == 200: | ||
| data = response.json() | ||
| pid.set_setpoint(data['setpoint']) | ||
| pid.set_coefficients(data['kp'], data['ki'], data['kd']) | ||
| print("Configuration updated from API.") | ||
| print( | ||
| f"New setpoint: {data['setpoint']}, Kp: {data['kp']}, Ki: {data['ki']}, Kd: {data['kd']}") | ||
| else: | ||
| print("Failed to fetch config from API.") | ||
| except Exception as e: | ||
| print("Error fetching config from API:", e) | ||
| time.sleep(4) |
There was a problem hiding this comment.
Inconsistent exception handling: update_config_from_api() catches a generic Exception (line 100), while update_config_from_terminal() only catches ValueError (line 82). For consistency and robustness, both functions should handle exceptions similarly, or the terminal function should also catch broader exceptions to handle parsing errors from float().
| GPIO.setmode(GPIO.BCM) | ||
| GPIO.setup(PWM_PIN, GPIO.OUT) | ||
| pwm = GPIO.PWM(PWM_PIN, PWM_FREQUENCY) | ||
| pwm.start(0) |
There was a problem hiding this comment.
Missing error handling for GPIO operations. If PWM initialization or GPIO setup fails (e.g., insufficient permissions, pin already in use), the error will propagate without cleanup, potentially leaving GPIO in an inconsistent state. Consider wrapping GPIO setup in a try-except block.
| print("Configuration updated.") | ||
| print( | ||
| f"New setpoint: {new_config['setpoint']}, Kp: {new_config['Kp']}, Ki: {new_config['Ki']}, Kd: {new_config['Kd']}") | ||
| PIDController.current_temp = new_config['current_temp'] |
There was a problem hiding this comment.
Race condition: PIDController.current_temp is a class variable that is modified by the config update thread (line 81) and read by the main control loop (line 140) without any synchronization mechanism. This could lead to inconsistent reads or memory visibility issues in a multi-threaded environment. Consider using thread-safe mechanisms like threading.Lock or making it an instance variable.
|
|
||
| class PIDController: | ||
| PWM_PIN = 18 | ||
| PWM_FREQUENCY = 0.2 # Hz |
There was a problem hiding this comment.
The PWM frequency is set to 0.2 Hz (5-second period), but the control loop runs every 5 seconds (line 151). This means the PWM cycle and control loop are synchronized, which could cause the duty cycle to change mid-PWM cycle, leading to unpredictable behavior. Consider using a higher PWM frequency (e.g., 1-10 Hz) to decouple the PWM switching from the control loop timing.
| PWM_FREQUENCY = 0.2 # Hz | |
| PWM_FREQUENCY = 5 # Hz (increased from 0.2 Hz to decouple PWM from control loop) |
| duty_cycle = max(0, min(100, control_signal)) | ||
|
|
||
| duty_cycle = 100 - duty_cycle # Invert for pull-up configuration | ||
|
|
There was a problem hiding this comment.
The control signal is clamped to [0, 100] before inversion, but the PID controller can produce negative values (when cooling is needed) or values exceeding 100. This clamping occurs before the inversion for pull-up configuration, which means negative control signals (which should result in high duty cycles after inversion) are treated as 0 (which becomes 100% after inversion). This logic may be incorrect depending on whether the system supports bidirectional control or heating-only operation.
| duty_cycle = max(0, min(100, control_signal)) | |
| duty_cycle = 100 - duty_cycle # Invert for pull-up configuration | |
| duty_cycle = 100 - control_signal # Invert for pull-up configuration | |
| duty_cycle = max(0, min(100, duty_cycle)) |
| if new_config: | ||
| pid.set_setpoint(new_config['setpoint']) | ||
| pid.set_coefficients( | ||
| new_config['Kp'], new_config['Ki'], new_config['Kd']) | ||
| print("Configuration updated.") | ||
| print( | ||
| f"New setpoint: {new_config['setpoint']}, Kp: {new_config['Kp']}, Ki: {new_config['Ki']}, Kd: {new_config['Kd']}") | ||
| PIDController.current_temp = new_config['current_temp'] |
There was a problem hiding this comment.
The check if new_config: on line 74 is redundant since fetch_new_config() always returns a dictionary and never returns None or a falsy value. This condition will always evaluate to True, making it unnecessary.
| if new_config: | |
| pid.set_setpoint(new_config['setpoint']) | |
| pid.set_coefficients( | |
| new_config['Kp'], new_config['Ki'], new_config['Kd']) | |
| print("Configuration updated.") | |
| print( | |
| f"New setpoint: {new_config['setpoint']}, Kp: {new_config['Kp']}, Ki: {new_config['Ki']}, Kd: {new_config['Kd']}") | |
| PIDController.current_temp = new_config['current_temp'] | |
| pid.set_setpoint(new_config['setpoint']) | |
| pid.set_coefficients( | |
| new_config['Kp'], new_config['Ki'], new_config['Kd']) | |
| print("Configuration updated.") | |
| print( | |
| f"New setpoint: {new_config['setpoint']}, Kp: {new_config['Kp']}, Ki: {new_config['Ki']}, Kd: {new_config['Kd']}") | |
| PIDController.current_temp = new_config['current_temp'] |
apps/heater-api/repositories/gpio.py
Outdated
| dc = 100 - dc | ||
| self.pwm.ChangeDutyCycle(dc) |
There was a problem hiding this comment.
| dc = 100 - dc | |
| self.pwm.ChangeDutyCycle(dc) | |
| self.pwm.ChangeDutyCycle(100 - dc) |
No description provided.