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 function #13

Closed
mgulan opened this issue Apr 5, 2018 · 13 comments
Closed

PID function #13

mgulan opened this issue Apr 5, 2018 · 13 comments
Assignees
Labels
AutomationShield common Common functionality for the entire library enhancement New feature or request

Comments

@mgulan
Copy link
Collaborator

mgulan commented Apr 5, 2018

Here is the version of the digital PID controller that you could implement, see attachment.
If you needed help/ideas with the code, let me know.

img_7575

Think of what will be the inputs/outputs of the function, whether it shall internally contain anti wind-up, saturation, pre-filtering, etc.

@gergelytakacs
Copy link
Owner

gergelytakacs commented Apr 5, 2018

Thanks for your contribution and greetings from the train...:) Here is my take:

  • I think it should-eventually-be both incremental and absolute formulation. For now any one of them is fine, have to start somewhere.
  • Anti-wind up should be part of the PID method as it concerns the internal variables of the PID algorithm. However, it is worth cosidering using a stand-alone method for it, which could aid maintainability of the code.
  • Do not forget, this is mainly an educational tool, so we don't want to make it too self-closed and possibly complex. So, imho pre-filtering and saturation should be outside to increase didactic value (and improve code maintainability).
  • The method should take, in the most basic version, current error (past should be handled internally), tuning parameters and return input.
  • For educational value, tuning should be in the original form (prop., Ti, Td) and the re-calculation to q0,q1 etc shall be internally. This, of course, wastes resources but it is better educationally.

@gergelytakacs gergelytakacs added AutomationShield common Common functionality for the entire library enhancement New feature or request labels Apr 5, 2018
@mgulan
Copy link
Collaborator Author

mgulan commented Apr 5, 2018

OK, I agree:-)
To correct myself, the form I sent is in fact the absolute one, yet can be easily used as incremental when only the input increment is used.

@gergelytakacs
Copy link
Owner

Haven't seen the picture on my cell, yes - this is the one. Absolute is a bit more intuitive, incremental could be an alternate option...

@Gabor7697
Copy link
Contributor

d0948aa

@gergelytakacs
Copy link
Owner

gergelytakacs commented Apr 7, 2018

  • I don't see any reason why PID needs its own class, do you @Gabor7697 ? Why can't it be AutomationShield.PID() ??? Remember, you are writing something fairly universal not just a will do for now improvised thing.
  • I don't really understand the logic behind your new class with its own comp method etc. Very-very confusing, obscure and not sure about your motivation.
  • Ts should be considered as a globally accessible constant, defined somewhere outisde of PID. Just initialize it right at the beginning in the header and assume it is there long int Ts. PID should just assume it is available, or if not, default to 1 or throw an error. There is no good reason to have it entered inside the PID method again.
  • What is a "reverse acting system"? Don't you just mean the sense/sign of the feedback loop? But that is just saying u=-u...
  • If something you want to do does not work because of e.g. scope issues, re-think your approach and study up how it's done.
  • Don't really understand at the first look what's different between the two versions, you really need to comment your code.

@gergelytakacs gergelytakacs added this to First try in PID Apr 7, 2018
@Gabor7697
Copy link
Contributor

pid1() is based on @mgulan s difference equation method.

@gergelytakacs
Copy link
Owner

Ok, I see! In that case it needs to be properly tetsed and thought out how it will be included in the library.

@kulho
Copy link
Contributor

kulho commented Apr 8, 2018

I've been trying to commit my code for PID for at least two hours, but I cannot seem to figure out what's wrong. Do I need a permission to do that or I am still doing something wrong?

@gergelytakacs
Copy link
Owner

gergelytakacs commented Apr 9, 2018

@kulho You haven't accepted the invite yet. Sent a mail about it.
image

@gergelytakacs
Copy link
Owner

@Gabor7697 deleted your new PID branch, because I merged to master anyways. The "old" PID is there only until tomorrow, until we discuss how to continue. Also I want to review @kulho changes and show how it is not a good idea to to rewrite someone's code;)

@gergelytakacs
Copy link
Owner

gergelytakacs commented Apr 17, 2018

  • Unify coding style, variable names etc. between all the PID versions. The PID functions should not look entirely differently and should be readable. Remember, this is a didactic tool.
  • @rkoplinger and @Gabor7697 should agree on unifying the two PID versions (the absolute ones).
  • Which style of tuning to use and why? You should look up which one is prefferred, eg. in Matlab PID tuning functionality, Simulink blocks etc. and use that. Should it be Kp, Ki, Kd or Kp, Ti, Td? Why one or theo other?
  • documentation in the WiKi
  • think about improving integral windup

-~~~leave pid alone and/or combine it with @rkoplinger version~~~
-~~~rename pid1 to pidInc (any better ideas?)~~~
-~~~direction is not needed~~~
-~~~integral widndup for the absolute pid~~~

This was referenced Apr 24, 2018
@gergelytakacs
Copy link
Owner

Just a quick idea: pick a way to input tuning to PID and stick with it. But then, you could do a function that translates between the two...

@gergelytakacs
Copy link
Owner

There has been lots of improvement from the original PID function and the redesign helped a lot as well, see #46

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
No open projects
FloatShield
  
To do
PID
  
In Progress
Development

No branches or pull requests

7 participants