Skip to content

ADD: PID controller class - #88

Open
Mr-Msf wants to merge 2 commits into
controlfrom
dev/PID_controller
Open

ADD: PID controller class#88
Mr-Msf wants to merge 2 commits into
controlfrom
dev/PID_controller

Conversation

@Mr-Msf

@Mr-Msf Mr-Msf commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

This PR contains code for a PID controller class.

@Mr-Msf
Mr-Msf requested a review from wlodaralex as a code owner August 13, 2026 19:11
@Mr-Msf Mr-Msf linked an issue Aug 13, 2026 that may be closed by this pull request
import numpy as np

class PID:
def __init__(self, P_coeff: float = 0.0, I_coeff: float = 0.0, D_coeff: float = 0.0, integration_type: str = "R_rect"):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nitpicky but for consistency with the usual controls convention I'd like to see these coefficients changed to K_P, K_I, K_D and referred to as gains in the documentation. It'll make the code more searchable by adhering to commonly used nomenclature.

"""
return tuple(self.PID_coeffs)

def begin_sim(self, starting_state_vec: np.ndarray, desired_state_vec: np.ndarray, default_timestep: float = 1.0, init_time: float = 0.0):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I see the need for this. We're going to provide the current state every time we call step() anyway and the setpoint is something we should be able to change at any time. As far as init_time, if we want to use absolute time then you can just specify that in init(). To be honest though it shouldn't really matter because we only need to know the elapsed time between steps. We should be able to just initialize the class with its gains and run the computation at each timestep.

self.integrated_error = np.zeros(np.shape(self.goal)[0])
self.prev_integ_error = self.integrated_error.copy()

def _integrate_error(self, error: np.ndarray, prev_error: np.ndarray, step_duration: float):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this approach of directly modifying self.integrated_error is going to cause problems when used in an adaptive-step simulation integrator. I think this might need to be handled at a higher level such that the integrated term is only updated after the simulation has committed to a timestep. I don't love having to explicitly update outside the controller's own context, but I don't see another way right now. The result would be that you need a sort of candidate integrated error for the timestep and then a function that locks it in when the next simulation timestep is taken.

step_duration = self.prev_timestep
self.integrated_error = self.prev_integ_error.copy()

self.error = self.goal - state_vec

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a function to update self.goal. And I would prefer to refer to it as self.setpoint or self.target_state

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CONTROL - implement PID

2 participants