braincore / pid-rs

A PID controller for Rust projects.
Apache License 2.0
92 stars 26 forks source link

API reimplementation #9

Closed Owez closed 1 year ago

Owez commented 3 years ago

Closes #8

The current todo list is included within files, the current main checklist is:

- [ ] Speed testing - [x] General testing - [ ] Expanded testing for edge cases like integral handling - [x] Finishing calculations - [x] Redoing examples for README.md - [x] Documentation

braincore commented 3 years ago

Generally, the PidSegment abstraction isn't necessary for the (modest) complexity of the pid. There might be some performance implications as well (you do point out speed testing) which may impact those running this on no_std microcontroller environments.

My suggestion is to start with the existing code and simply add the p(), i(), d() functions that mutate kp, ki, kd.

Owez commented 3 years ago

The equation listed on the current master readme is:

PID independent form

Which includes a - instead of +; I assume this is a mistake?

braincore commented 3 years ago

It's negative intentionally. We're using the change in process variable rather than change in error in this Pid, which has the desirable property of mitigating "derivative kick." I'll try to find you my original source that explained these optimizations clearly, but here's a quick source I found that does the job:

Owez commented 3 years ago

Ahh alright thanks -- I feel I looked at the code, saw p + self.integral_term + d and got confused

braincore commented 3 years ago

Can you make this diff the bare minimum to change what's been discussed? You can follow up with other commits later to tweak the docs, etc... (happy to give you push access)

Owez commented 3 years ago

Done with edbd3f2 :)

Yeah push access will be nice to tweak the rest as you say

braincore commented 3 years ago

In preparation for rebase, please squash everything down to one commit.

braincore commented 3 years ago

Just that final comment. It's close!

braincore commented 3 years ago

In addition to squashing the commits, make sure to give a condensed commit message. Something like "New API for configuring Pid" for the headline. For the body of the message, you can add the rationale for doing so: improved clarity of specification, make pid more approachable, improve developer experience, ...

Owez commented 3 years ago

Apologies for the gap, final week and a half of school! Will add changes and push now

Owez commented 3 years ago

(Bump)

quietlychris commented 2 years ago

@braincore Just wanted to check if this API change was still being considered! I was hoping to add a PID controller to a simulation I'm writing, and am definitely still interested in using this library.

Owez commented 1 year ago

Bump @braincore

braincore commented 1 year ago

Great! Thanks a ton for this work and apologies again that it took so long. I squashed and committed your branch outside of the PR flow (rebase workflow) so I'll close this PR manually.