Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PID redesign #46

Closed
gergelytakacs opened this issue Apr 26, 2018 · 2 comments
Closed

PID redesign #46

gergelytakacs opened this issue Apr 26, 2018 · 2 comments
Assignees
Labels
AutomationShield common Common functionality for the entire library enhancement New feature or request

Comments

@gergelytakacs
Copy link
Owner

gergelytakacs commented Apr 26, 2018

Redesign of the PID functionality to increase usability and scalability. This is, in essence a continuation of #13 .

@rkoplinger suggested (in person) to move PID functionality in its own class. This, I have initially rejected for I have thought that having something like AutomationShield.pid() would simplify the use of the library from the end users perspective. Then came all the versions for absolute and incremental inputs, then the discrepancies between the tuning constants (Kp, Ki, Kd or Kp Ti Td) and of course saturation, integration windup and all that. @rkoplinger was right, so I have changed my mind.

I think that the PID functionality needs to be redesigned for these reasons. Here are some ideas (not necessarily instructions) how to change it:

  • Move PID to its own class PIDClass (constructed as PID)
  • Let the PID functionality have its own .h and .cpp files for easier maintenance
  • The user could set gains or time constants independently, eg, there could be functions such as PID.tuningKp() and PID.tuningKi... (Can you do PID.tuning.Kp()? that would be even better)
  • The user should be able to choose (switch) between the incremental and absolute forms, for example by calling someting like PID.beginAbsolute() (Again, can it be PID.begin.absolute()`) and counterparts.
  • The hard saturation and windup function should be called independently and it would not only take the boundaries, but also initialize the proper PID version and code... (PID.windup(float hi, float low) )
  • These things could be a sort of an initialization, then the final call could be something like PID.compute(float error) or something like that.

These are just ideas, please contribute.

@gergelytakacs gergelytakacs self-assigned this Apr 26, 2018
@gergelytakacs gergelytakacs added AutomationShield common Common functionality for the entire library enhancement New feature or request labels Apr 26, 2018
@gergelytakacs
Copy link
Owner Author

You guys did an awesome job, love it. @rkoplinger / @kulho

There are two "leftover" branches. I created pull requests but you need to decide what can (and should) be pulled into master.

@gergelytakacs
Copy link
Owner Author

The leftover branches are cleared. This is a quite nice version now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutomationShield common Common functionality for the entire library enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants