lbl-srg / modelica-buildings

Modelica Buildings library
249 stars 155 forks source link

Model Buildings.Templates.Plants.Controls.Utilities.Validation.TimerWithReset fails verification #3952

Open AndreaBartolini opened 1 month ago

AndreaBartolini commented 1 month ago

The variable noThrRes.passed of the model Buildings.Templates.Plants.Controls.Utilities.Validation.TimerWithReset fails the verification vs the Dymola reference file because Dymola and OpenModelica process in different way some concurrent events in the event iteration.

The complete analysis is available in the ticket https://github.com/OpenModelica/OpenModelica/issues/12746, the point that I need to understand is what is the expected behavior between the one produced by Dymola and the one produced by OpenModelica.

Please give me a feedback, thanks in advance.

AntoineGautier commented 1 month ago

The component noThrRes validates the limit behavior of the timer with t=0: it should then resolve to a direct signal pass-through. So, we expect noThrRes.passed=noThrRes.u — which is not strictly achieved with the current implementation as explained below.

Current implementation

At time=1.2 the variable noThrRes.reset transitions through false, true, false. With Dymola and Optimica, this yields 2 event iterations:

  1. The first one false, true fires this branch:
    elsewhen reset then
      entryTime=time;
      passed=false;
  2. The second one true, false fires this branch:
     when u and not reset then
       entryTime=time;
       passed=t <= 0;

    So, at time=1.2 the variable noThrRes.passed transitions through true, false, true. This gives the following trajectories — where res.y and passed.y are obtained by holding the variables noThrRes.reset and noThrRes.passed for a little time so that each instantaneous transition is visible in the plot.

image

On the contrary, OMC gives the following trajectory where the branch when u and not reset then is not activated at time=1.2 as explained in the analysis from the link:

image

Unless there is a part in the MLS that says that event iterations can be merged if they occur "at the same time", I think Dymola and Optimica are correct here.

Proposed modification in MBL

That being said, the current implementation (based on Buildings.Controls.OBC.CDL.Logical.Timer) could be made much simpler (and clearer) by extracting the equation for passed from the when clauses. This is because a generic formulation for the passed variable is possible, which

Below is the proposed modification, that should be used for Buildings.Controls.OBC.CDL.Logical.Timer as well. With this modification, OMC and Dymola give the same results. Nevertheless, the above general consideration regarding event iterations in OMC remains open.

Buildings.Templates.Plants.Controls.Utilities.TimerWithReset

initial equation 
  entryTime = time;
equation 
  when {u, reset} then
    entryTime = time;
  end when;
  y = if u then time - entryTime else 0.0;
  passed = u and y >= t;

I will open a PR in MBL to implement this change (but most likely on 8/5 or so).

To do

AndreaBartolini commented 1 month ago

I agree with the proposed modification as well with the necessity to investigate the missed iteration by OpenModelica.

My opinion about the opportunity to use a pulse train as signal in complex logic remains unchanged, a square wave is most robust and reduces a lot the risk of concurrent events.

AntoineGautier commented 1 month ago

I've just run additional tests with the proposed change. It turns out that extracting the equation for passed from the when clauses, results in additional state events. Integrating the equation for passed back into the when clauses as described below

Buildings.Templates.Plants.Controls.Utilities.TimerWithReset

initial equation 
  entryTime = time;
  passed = t <= 0;
equation 
  when {u, reset} then
    entryTime = time;
    passed = u and t <= 0;
  elsewhen u and time >= pre(entryTime) + t then
    entryTime = pre(entryTime);
    passed = true;
  elsewhen not u then
    entryTime = pre(entryTime);
    passed = false;
  end when;
  y = if u then time - entryTime else 0.0;
AntoineGautier commented 1 month ago

Regarding the use of a square wave signal for noThrRes.reset in the validation model, the issue is that control logic often uses "instantaneous triggers" to generate a sequence of events, e.g.

Timers shall reset to zero at the completion of every stage change.

The clause "at the completion of every stage change" is only true at a given time instant, but always false over any period of time. It would typically be computed with the change() or edge() operator. So, I believe that validating elementary blocks with Boolean signals generated by the sample() operator is still necessary.

AndreaBartolini commented 1 month ago

I've just run additional tests with the proposed change...

I still prefer your first change proposal (passed calculated out of the when condition), it is more clear and simple in terms of state machine, so more robust. The state events that you are detected is generated by the equation that calculates the passed value on the bases of the current state of the state machine, and this is correct. The reset of the state machine, in this case, is time driven because the reset is generated by using the sample() operator, but it is specific of this model.

"instantaneous triggers"

The meaning of "instantaneous" in a boolean logic depends on the type of the logic. If you are designing a sampled logic (like PLC) "instantaneous" means less enough than the sampling period, if you are designing a logic that is hardware-implemented (typically by using the C-MOS technology) "instantaneous" means less than the faster clock but larger enough to be able to trigger the C-MOS components (this minimum time depends on the C-MOS technology). The Dirac delta function is just an abstraction but it is not applicable in the real world.

At the end of the story, if you want to build a model that matches as much as possible the real behavior of the system the right (and physical and robust) way is to use square waves with a proper duty cycle, the sample() operator should be used to perform the sampling operation only.

AntoineGautier commented 1 month ago

Thanks for the valuable insight into the modeling of "instantaneous" events. I also prefer the first proposed modification. However, it implies revisiting some changes that were seen as enhancements of Buildings.Controls.OBC.CDL.Logical.Timer. I need to discuss with the LBL team before I go down this route. I suggest to put this issue on hold until @mwetter is back around 8/14.