Issue2884 pid autotuning

[mwetter]

This merges the PID autotuning to the master.

[SenHuang19]

@mwetter Do we need to close this PR? I am not using it anymore because 1) it is merging into the master branch 2) I completely redesigned the model and it will be very hard to track the review history. A new PR has been made in #3089

[mwetter]

@SenHuang19 Since the PR #3089 is also for this branch, we can leave it, and the comments should get resolved if the respective section of the code changed.

[SenHuang19]

@mwetter thanks for your prompt response. I will respond to your comments in this PR.

[SenHuang19]

@SenHuang19: This requires considerable more work. Please also see the email I sent.

@mwetter: I have tried my best to address your comments. Please take a look at the responses to your comments. I believe your comments in the email have also been reflected. The new implementation has been reviewed by Jianjun and currently resides in issue2884_PID_autotuning. Could you please let me know if you have more comments?

[JayHuLBL]

@SenHuang19 In the composed block Buildings.Controls.OBC.Utilities.PIDWithAutotuning.FirstOrderAMIGO, there is no detailed documentation of how to use the controller. For instance, by only looking into the model, the user will not know how the parameters yHig, yLow, deaBan, etc. affect the results and how to set the parameters.

Also, does the controller have the difference for direct acting and reverse acting?

[SenHuang19]

@SenHuang19 Please see my inline comments.

@JayHuLBL Could you please let me know if your earlier comments are addressed?

[mwetter]

Next: @JayHuLBL to review.

[JayHuLBL]

@mwetter It will be ready for your review when CI tests passed.

[mwetter]

@SenHuang19 : Please let me know if you need anything else from me regarding the above feedback.

[SenHuang19]

@SenHuang19 : Please let me know if you need anything else from me regarding the above feedback.

@mwetter Thanks for checking with me. I was just back from a personal leave this week and started to work on it yesterday. I will let you know if I need any more guidelines.

[mwetter]

@SenHuang19 : Please let me know if you need anything else from me regarding the above feedback.

@mwetter Thanks for checking with me. I was just back from a personal leave this week and started to work on it yesterday. I will let you know if I need any more guidelines.

Great, thanks for the update.

[SenHuang19]

@SenHuang19 : Please let me know if you need anything else from me regarding the above feedback.

@mwetter Thanks for checking with me. I was just back from a personal leave this week and started to work on it yesterday. I will let you know if I need any more guidelines.

Great, thanks for the update.

@mwetter can you please take a look at the updated model based on your comments?

[JayHuLBL]

@SenHuang19 I see the CI tests are failing. I can fix some of them and will let you know if there is any help from you.

[SenHuang19]

@SenHuang19 I see the CI tests are failing. I can fix some of them and will let you know if there is any help from you.

@JayHuLBL Thanks a lot! Please do let me know the issue so I can avoid it in the future.

[SenHuang19]

@JayHuLBL thank you so much for you edits. There are indeed a lot of details to be addressed. Can you please let me know if you need me to do anything at this point?

[JayHuLBL]

@SenHuang19

I did some editing on the documentation and comments. However, I have some additional comments:

• [ ] For the class Buildings.Controls.OBC.Utilities.PIDWithAutotuning.Relay.BaseClasses.Validation.HalfPeriodRatio,
  • the documentation indicates that at 0.1 s, the output switches from On to Off. So What output are you referring? Do you mean triSta becomes true? However in fact, both of these happen at 0.2 s.
  • the documention indicates that at 0.7 s, the output switches to On. But in the plot, there is no change either triSta or triEnd.
  • in the documentation of Buildings.Controls.OBC.Utilities.PIDWithAutotuning.Relay.BaseClasses.HalfPeriodRatio, it indicates that The tuning period is triggered to end when either tOn or tOff changes after they become positive. However, in the test model, at 0.7 s, the tOn changes from 0.1 to 0.7 but the triEnd remains false.
• [ ] For the class Buildings.Controls.OBC.Utilities.PIDWithAutotuning.Relay.BaseClasses.Validation.OnOffPeriod, the tOn remain zero even though the on is true. According to the documentation in Buildings.Controls.OBC.Utilities.PIDWithAutotuning.Relay.BaseClasses.OnOffPeriod, it calculates the length of the on perid. So, shouldn't the tOn output will increase from 0 to 0.06. rather than remains 0 and step changes to 0.06?
• [ ] it would be better to improve the comments for trigger and yOn in Buildings.Controls.OBC.Utilities.PIDWithAutotuning.Relay.Controller. I think the trigger is True: enables the relay controller and the yOn is True: tuning starts.
• [ ] in class Buildings.Controls.OBC.Utilities.PIDWithAutotuning.Relay.ResponseProcess, what is the difference between yOn and triSta and triEnd? If my thought below is correct, then why do we need triSta and triEnd?
  • when yOn is true, the triSta is true and triEnd is false.
  • when yOn is false, the triSta is false and triEnd is true.

[JayHuLBL]

@mwetter It will be ready for your review when the tests are passed.

[JayHuLBL]

@mwetter It's ready for your review.

[SenHuang19]

@mwetter there seems to be a bug which causes the following issue.

• [ ] Explain why in Buildings.Controls.OBC.Utilities.PIDWithAutotuning.Validation.PIDWithFirstOrderAMIGO, the quanity PIDWitTun.con.Td differs after the first and second tuning period. Shouldn't it be the same?

Do you want to me to work on this?

[mwetter]

@SenHuang19 : It would be great if you can work on this. I wonder if the plant has too low an order so that the identification is indeterminate?

[SenHuang19]

@SenHuang19 : It would be great if you can work on this. I wonder if the plant has too low an order so that the identification is indeterminate?

@mwetter

Hi Michael, initially I thought this was caused by the fact that some samplers are not re-initialized but I believe you are right.

Specifically, it is associated with

When τ is close to 1, L/T is very sensitive to the change of τ.

Should I re-design the validation case to avoid this scenario? Maybe just use the following case?

• [ ] Add a test that uses it with an actual control loop, such as WetCoilCounterFlowPControl which could be copied and adjusted to provide a realistic example.

[mwetter]

@SenHuang19 : I suggest you redesign the validation case, as having a small simple validation case is useful. You may want to see if the 1st order room model that is used in the OptimalStart validation works better; and/or add two first order filters rather than the discrete time delay at the output of the autotuning controller. Such filters would approximate a series of control volumes.

[JayHuLBL]

The results difference between OpenModelica and Dymola is due to the results different of PIDWitTun.resPro.TunMon.tInc.us. See the one from OpenModelica: And from Dymola:

[SenHuang19]

The results difference between OpenModelica and Dymola is due to the results different of PIDWitTun.resPro.TunMon.tInc.us. See the one from OpenModelica: And from Dymola:

Should I add a re-init to resPro.TunMon.samAddtOntOff?

[JayHuLBL]

The root cause of the OpenModelica and Dymola results difference is from the instance PIDWitTun.resPro.TunMon.gretOnAndtOff.

See the OpenModelica results:

And the Dymola results:

In both simulation, the CDL.Reals.Greater has the same inputs (u1 and u2), with hysteresis set to be zero. The Dymola gives expected results, but the OpenModelica gives wrong output. This could be an issue in OpenModelica.

To reproduce, run following code in OpenModelica will give wrong result:

model Greater_test "Validation model for the Greater block"
  Buildings.Controls.OBC.CDL.Reals.Greater gre
    "Greater block, without hysteresis"
    annotation (Placement(transformation(extent={{40,-10},{60,10}})));
  Buildings.Controls.OBC.CDL.Reals.Sources.Pulse pul(period=1, shift=0.1)
    annotation (Placement(transformation(extent={{-40,10},{-20,30}})));
  Buildings.Controls.OBC.CDL.Reals.Sources.Constant con(k=0)
    annotation (Placement(transformation(extent={{-40,-40},{-20,-20}})));
equation 
  connect(con.y, gre.u2) annotation (Line(points={{-18,-30},{20,-30},{20,-8},{38,
          -8}}, color={0,0,127}));
  connect(pul.y, gre.u1) annotation (Line(points={{-18,20},{20,20},{20,0},{38,0}},
        color={0,0,127}));
  annotation (
    experiment(
      StopTime=1.5,
      Tolerance=1e-06,
      __Dymola_Algorithm="Dassl"));
end Greater_test;

[mwetter]

@JayHuLBL : If it is an issue with OpenModelica, can you submit a bug report with instructions for how to reproduce it.

[JayHuLBL]

@mwetter I will submit a bug report to OpenModelica.

[JayHuLBL]

There was two discussion about the OpenModelica issue, in their ticket 8600 and 11049, and the issue we created 11468. The comparison between two real variables would cause results difference between OpenModelica and Dymola. Two avoid the issue, we could change the input u2 of PIDWitTun.resPro.TunMon.gretOnAndtOff from previous 0 to 1e-5, which is usually greater than the solver's tolerance. The test shows there is no more different PIDWitTun.resPro.TunMon.gretOnAndtOff.y .

However, the unit test with OpenModelica will still give difference results at the very beginning of the simulation, see below:

Both the instances uniDel1 and derivative1 have parameter y_start and they have been default set to 0. Dymola did not show the start value but the OpenModelica showed it.

[mwetter]

@JayHuLBL : Did you test whether your correction leads to the same results, other than the initialization? I still see substantial differences when I run the unit test with OpenModelica (1.23.0~dev-103-gd230b4a) toward the end of the simulation, for example for Validation.PIWithFirstOrderAMIGO.

@SenHuang19 : Please add in the model info section of TuningMonitor what the mul instance is doing. It multiplies two inputs with units of time, so the output is s^2. That does not seem to be right, or it needs a clear explanation.

[JayHuLBL]

@mwetter I further improved the real comparison in the class Relay.BaseClasses.TuningMonitor, now we can only see the results difference during the initialization.