pi place and tests by albheim · Pull Request #502 · JuliaControl/ControlSystems.jl

albheim · 2021-05-13T19:58:57Z

Named it placePI to conform to the other methods there.

JuliaControlBot · 2021-05-13T20:06:55Z

This is an automated message.
Plots were compared to references. 2/11 images have changed, see differences below.
After pulling this PR, please update the reference images by creating a PR to ControlExamplePlots.jl here.

Difference	Reference Image	New Image
✔️ 0.0
✔️ 0.001

src/pid_design.jl

codecov · 2021-05-13T20:27:55Z

Codecov Report

❗ No coverage uploaded for pull request base (dev@64f8214). Click here to learn what that means.
The diff coverage is n/a.

@@          Coverage Diff           @@
##             dev     #502   +/-   ##
======================================
  Coverage       ?   84.56%           
======================================
  Files          ?       31           
  Lines          ?     3181           
  Branches       ?        0           
======================================
  Hits           ?     2690           
  Misses         ?      491           
  Partials       ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64f8214...c5161a7. Read the comment docs.

olof3 · 2021-05-13T20:50:14Z

src/pid_design.jl

+Default is to return the parameters for the parallel form, but if `series=true`
+they will be returned on series form.
+"""
+function placePI(P, ω₀, ζ; series=false)


I guess the kwarg should be time in analogy with the pid function? Although I feel that something more descriptive than time would be very good.

pid has series as keyword well, in this case it's about series / parallel form.

Hmm, according to https://en.wikipedia.org/wiki/PID_controller (and it seems consistent with Advanced PID Control), there are

standard form: Kp*(1 + 1/Ti/s + Td*s)

parallel form: Kp + Ki/s + Kd*s

series form: Kc*(1 + 1/τi/s)*(τd*s + 1)

So for the pid function I guess one should just have to specify form = :standard/:parallel/:series? Currently there are two boolean inputs time and series for specifying one of four different forms. I.e. one combination is nonstandard, and it is not really clear to me how those combinations map to the above, although I could guess.

For the PI case that we are discussing here it is about standard/parallel? So the kwarg should actually be parallel=false.

Two completely different control systems (one European one Chinese) we are working with at work both define a PI controller as

Kp + 1/(s*Ti)

which is somewhere inbtween. In fact, I believe most robot systems out there define PI controllers like this. It's a parallel form but the integral parameter treated as integral time rather than gain

Hmm, seems like a lot of cases to cover. I also recognize the ones that Olof linked and was a bit confused with the time keyword.

Would it be easier to just add that as a fourth form and do form = :standard/:parallel/:series/:parallel_time or something like that instead of having two keywords? I guess supporting all of them should not be a lot of extra work since most transformations are easy. Can just do all calculations on one form in the toolbox and add a pi_form_convert method that converts from that oen to any of the other ones.

A function that converts between different conventions is perhaps a good idea, it would be a mess to support all conventions in every single function that handles pid parameters :/

Two completely different control systems (one European one Chinese) we are working with at work both define a PI controller as
Kp + 1/(s*Ti)

In that case Ti doesn't correspond to integral time.
I've actually also encountered that formulation in the wild :P

If this is something that you want then we should give it a name that avoids accidental usage, perhaps :nonstandard ?

Yes, some conversion functionality from standard or (parallel) form would be good.

olof3 · 2021-05-13T20:50:18Z

src/pid_design.jl

+    if !series
+        ki /= kp 
+    end
+    return kp, ki, C


Should the parameters be returned? Anyway, I would say that the controller is the most interesting object and should be first?

Also, if we add a PID place, the controller would be the fourth element which seems a bit inconsistent.

Perhaps the PI parameters should be in a tuple?

If the function is used to tune a PI controller that is implemented in hardware, you want the parameters. One option is to return a named tuple, (; kp, ki) and (; kp, ki, kd), they destructure like tuples but also have the self-documenting names in there, so you know what you get back without reading the docstring.

Okay, so we should keep the parameters and return them in a tuple.

Named tuple seems convenient, but what about (kp, ki) vs (kp, Ti)? I guess we would need a Val argument to avoid type instability?

Val arguments are almost never required anymore since the compiler is much more aggressive with constant propagation, so if the option

placePI(..., form=:Ti)

is hard coded, the compiler will likely treat the form as a compile-time constant and propagate it

Returns as named tuple now, with different names depending on selected form.

test/test_pid_design.jl

Co-authored-by: olof3 <olof3@users.noreply.github.com>

albheim · 2021-05-14T10:12:08Z

If we prefer this way, using form and returning named tuples, we should make the interface similar for all other PI methods as well

albheim · 2021-05-14T15:25:41Z

Okay to merge this, and leave additional fixes for later?

src/pid_design.jl

olof3 · 2021-05-14T15:40:20Z

Okay to merge this, and leave additional fixes for later?

Ok with me. Would be good to have pid work in the same way. Seems like most of the issues have been resolved.

src/pid_design.jl

Co-authored-by: olof3 <olof3@users.noreply.github.com>

* Avoid unnecessarily large realization for feedback of TransferFunction (#485) * Avoid unnecessarily large realization for feedback of TransferFunction * Fix and added more tests. * Change to numpoly, denpoly * add dev brach to PR CI * Switch u layout (#480) * switch u layout for lsim * Update src/timeresp.jl Co-authored-by: Fredrik Bagge Carlson <baggepinnen@gmail.com> * More updates, one error in test_timeresp * Fix tests * Change to AbstractVecOrMat * Catch CuArray in matrix conversion * General zero vectors for x0 to support GPUs * Update src/timeresp.jl Co-authored-by: Mattias Fält <mfalt@users.noreply.github.com> * Update src/timeresp.jl Co-authored-by: Mattias Fält <mfalt@users.noreply.github.com> * Move f outside lsim * Remove GPU compatible x0, save for later * Fix doctest * add kwargs * Remove variable and generalize type Co-authored-by: Fredrik Bagge Carlson <baggepinnen@gmail.com> Co-authored-by: Mattias Fält <mfalt@users.noreply.github.com> * Gangof4 fixes (#486) * QOL improvements for plotting * remove spurious getindex * update docstring * multiple Ms in nyquist * bugfixes * make rings appear in all subplots * Minor fixes to gang-of-four functionality. * Fixes to Nyquist plots. * Updated the nyquistplot docstring Co-authored-by: Fredrik Bagge Carlson <baggepinnen@gmail.com> * add frequency in Hz to dampreport (#488) * updates to nyquistplot (#493) * updates to nyquistplot * Update src/plotting.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> * Update src/plotting.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> Co-authored-by: olof3 <olof3@users.noreply.github.com> * fix traces in rlocus (#491) * fix traces in rlocus * Update src/pid_design.jl Co-authored-by: Albin Heimerson <albin.heimerson@control.lth.se> Co-authored-by: Albin Heimerson <albin.heimerson@control.lth.se> * let lsim handle arguments in lsimplot (#492) * bugfix: avoid creating continuous system with Ts=0.0 (#496) * Deactivate _preprocess_for_freqresp (#497) until hessenberg is properly used * allow balance when converting tf to ss (#495) * allow balance when converting tf to ss * use zeros(T) instead of fill(zero(T)) * Update src/types/StateSpace.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> * Update src/types/conversion.jl Co-authored-by: Albin Heimerson <albin.heimerson@control.lth.se> Co-authored-by: olof3 <olof3@users.noreply.github.com> Co-authored-by: Albin Heimerson <albin.heimerson@control.lth.se> * Better handling of problematic cases for delay systems (#482) * delay error should be correct now * warn limit * Add tustin c2d/d2c method (#487) * add Tustin discretization * no indexing after c2d * implement f_prewarp in tustin and test vs. matlab * fixe use wrong Ts * f_prewarp -> w_prewarp * w_prewarp in tests * correct handling of x0 in lsimplot (#498) * move eye def to framework.jl (#499) * Fix UndefVarError: T not defined (#501) * add axes for ss (#504) * pi place and tests (#502) * pi place and tests * Fix test * Add ending space in file * Update numvec denvec * Fixed error * Update test/test_pid_design.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> * Update test/test_pid_design.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> * Update forms * Fix test * Fix test * Update src/pid_design.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> * Update src/pid_design.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> * Update src/pid_design.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> Co-authored-by: olof3 <olof3@users.noreply.github.com> * Conver to tf in placepi (#507) * Use nstates instead of nx in lsimplot (#508) * Use nstates instead of nx in lsimplot * Add predictor (#511) * up innovation_form and add noise_model * keep innovation_form, add predictor * export predictor * add hats * updates to `obsv` (#517) * updates to `obsv` All computing an arbitrary number of rows in the observability matrix and accept `AbstractStateSpace` * Update src/matrix_comps.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> Co-authored-by: olof3 <olof3@users.noreply.github.com> * `hz` keyword in `nyquistplot` similar to in `bodeplot` (#518) * Fixes to place (#500) * Add tests for place. * Removed luenberger and exented place instead. Co-authored-by: Fredrik Bagge Carlson <baggepinnen@gmail.com> * allow AbstractStateSpace in several places (#520) * allow AbstractStateSpace in several places * Maintain type for zpk in c2d (#522) * maintain zpk type in c2d * Fix type in typeconversion for c2d tf * Fix type in c2d tf/zpk * Remove assertion on tf/zpk SISO for c2d * Test for c2d(zpk(..)..) unbroken * Keep MIMO assertion c2d tf * Add comment and fix test * Fix test * h to Ts * Fix test * remove assert * split into methods * Remove asserts (#523) * Replace asserts * add error types * fix conversion for custom types (#514) * fix conversion for custom types * special numeric_type for AbstractSS * fix conversion from ss to tf without type * more abstract statespaces (#521) * omre abstract statespaces * even more ASS * remove simple feedback in favor of mor egeneral version * propagate timeevol * add block diagram to feedback docstring * Updates to docstring * fix docstring formatting * delete redundancy in feedback docstring Co-authored-by: olof3 <olof3@users.noreply.github.com> * add controller function (#512) * add controller function * rename controller and predictor * Move plot globals to runtests (#531) * Move plot globals to runtests * Move plot globals to framework.jl * return similarity transform from `balreal` (#530) * display error when covar fails * return similarity transform from balreal * Update src/matrix_comps.jl Co-authored-by: olof3 <olof3@users.noreply.github.com> Co-authored-by: olof3 <olof3@users.noreply.github.com> * remove incorrect warning in pzmap (#535) * support hz keyword in sigmaplot similar to bodeplot (#537) * change formatting in dampreport (#536) * change formatting in dampreport * update dampreport to use ± * even better formatting * up formatting for complex systems * add additive identity element for statespace and TF (#538) * add additive identity element for statespace and TF * rm zero from type. Test MIMO zero * Fix struct_ctrb_obsv (#540) * Fix struct_ctrb_obsv, closes #409 * Update src/simplification.jl Co-authored-by: Fredrik Bagge Carlson <baggepinnen@gmail.com> * add to docstring Co-authored-by: olof3 <olof3@users.noreply.github.com> * Yet another fix for struct_ctrb_states. Closes #475 (#541) * bugfix for negative real pole in damp (#539) * Fix #546 * minor typographic changes * optional epsilon in dcgain (#548) * optional epsilon in dcgain * Update analysis.jl * bugfix: use correct type for saving dde solutions (#549) Would be good to have a regression test for `BigFloat` data which was the reason I found this bug. That seems slightly involved to fix though. I assume that this would also have failed for `ComplexF64`, so eventually a test for that too would be good. * bugfix dcgain (#551) * correct type of initial state in step (#553) * remove version checks * Fix spacing in type printing * write `struct_ctrb_states` in terms of `iszero` instead of `== 0` (#557) * write `struct_ctrb_states` in terms of `iszero` instead of `== 0` The reason is that `iszero` always returns a bool, whereas `== 0` may return anything. The difference appears for symbolic variables where ```julia julia> q0 == 0 q0(t) == 0 julia> iszero(q0) false ``` * Update simplification.jl * remove `issmooth` (#561) * remove issmooth * drop extra `]` * Return a result structure from lsim (#529) * Return a result structure from lsim This is by design a non-breaking change to the lsim interface since the structure allows both getindex and destructuring to behave just like if lsim returned a tuple of arrays like before. Indeed, the tests for lsim were not touched in this commit. This commit also adds a plot recipe to the result structure. All plot recipes for lsimplot, stepplot, impulseplot have been replaced by the new recipe. This is a breaking change since the names of the previous plots no longer exist. A slight change is that the plots for a step response no longer show the text "step response", but I think that's an acceptable change, the user can supply any title they prefer themselves. * remove automatic title * introduce additional abstract result type * do not destructure sys * remove LQG (#564) The functionality was very buggy and poorly tested. A much improved version with proper tests are available in https://github.com/JuliaControl/RobustAndOptimalControl.jl/blob/master/src/lqg.jl * pole->poles tzero->tzeros (#562) * update usage of plot for step simulation * add doctest filters * add release notes * Update README.md * Update README.md Co-authored-by: olof3 <olof3@users.noreply.github.com> Co-authored-by: Albin Heimerson <albin.heimerson@control.lth.se> Co-authored-by: Mattias Fält <mfalt@users.noreply.github.com>

pi place and tests

20ad24e

Fix test

665ef68

baggepinnen reviewed May 13, 2021

View reviewed changes

src/pid_design.jl Outdated Show resolved Hide resolved

albheim added 2 commits May 13, 2021 22:17

Add ending space in file

596b35b

Update numvec denvec

1cd870f

olof3 reviewed May 13, 2021

View reviewed changes

albheim and others added 5 commits May 13, 2021 23:02

Fixed error

4db6ed8

Update test/test_pid_design.jl

5c7d1f5

Co-authored-by: olof3 <olof3@users.noreply.github.com>

Update test/test_pid_design.jl

1a3c30e

Co-authored-by: olof3 <olof3@users.noreply.github.com>

Update forms

0c48589

Fix test

f34463e

Fix test

7c6b5ef

olof3 reviewed May 14, 2021

View reviewed changes

src/pid_design.jl Outdated Show resolved Hide resolved

olof3 reviewed May 14, 2021

View reviewed changes

src/pid_design.jl Outdated Show resolved Hide resolved

olof3 reviewed May 14, 2021

View reviewed changes

src/pid_design.jl Outdated Show resolved Hide resolved

albheim and others added 3 commits May 14, 2021 19:59

Update src/pid_design.jl

9d5c20e

Co-authored-by: olof3 <olof3@users.noreply.github.com>

Update src/pid_design.jl

fb240de

Co-authored-by: olof3 <olof3@users.noreply.github.com>

Update src/pid_design.jl

c5161a7

Co-authored-by: olof3 <olof3@users.noreply.github.com>

albheim merged commit a99ad57 into dev May 14, 2021

baggepinnen deleted the pi_place branch May 15, 2021 01:08

albheim mentioned this pull request May 16, 2021

Pid parameters updates #509

Merged

Conversation

albheim commented May 13, 2021

Uh oh!

JuliaControlBot commented May 13, 2021

Uh oh!

Uh oh!

codecov bot commented May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

albheim commented May 14, 2021

Uh oh!

albheim commented May 14, 2021

Uh oh!

Uh oh!

Uh oh!

olof3 commented May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented May 13, 2021 •

edited

Loading

olof3 commented May 14, 2021 •

edited

Loading