nobleo / path_tracking_pid

Path Tracking PID offers a tuneable PID control loop, decouling steering and forward velocity
Apache License 2.0
130 stars 37 forks source link

Added derivative filtered error tracker abstraction. #59

Closed lewie-donckers closed 2 years ago

lewie-donckers commented 2 years ago

I noticed there was a lot of code duplication to keep track of error history in ControllerState. I have 3 PRs lined up to address this. This one is the third.

Added a derivative filtered error tracker abstraction. The remaining errors in ControllerState are also always tracked in pairs. The raw errors/filtered errors and their derivatives. The derivatives are always calculated in the same way. This abstracts that principle to reduce code duplication and noise in the Controller.

Timple commented 2 years ago

I think this refactor looks good code-wise. But could be much better conceptually.

Basically we only want to calculate a derivative because we're doing PID control.

Abstracting the PID controller completely makes much more sense than abstracting P, I and D seperate. Would also make the code much more readable.

lewie-donckers commented 2 years ago

I think this refactor looks good code-wise. But could be much better conceptually.

Basically we only want to calculate a derivative because we're doing PID control.

Abstracting the PID controller completely makes much more sense than abstracting P, I and D seperate. Would also make the code much more readable.

Sounds good but I'm lacking domain knowledge to do this myself. Do you want to finish these PRs for the small improvement they bring? Or cancel/close them and do the PID control abstraction instead (by someone else)?

Timple commented 2 years ago

I would go for the latter.

It will probably rework most of the stuff here. The one implementing this can still use this as a small example.

lewie-donckers commented 2 years ago

I would go for the latter.

It will probably rework most of the stuff here. The one implementing this can still use this as a small example.

@Timple just to be sure we're on the same page: I will merge the previous two PRs and close/cancel this one, right?

Timple commented 2 years ago

Yup