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

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

Open
AndreaBartolini opened this issue Jul 23, 2024 · 12 comments
Assignees

Comments

@AndreaBartolini
Copy link
Contributor

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 OpenModelica/OpenModelica#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
Copy link
Contributor

AntoineGautier commented Jul 24, 2024

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

  • does not depend on a when clause,
  • better expresses the intent of the block, and
  • ensures passed=u if t=0.

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

  • Check that the proposed modification does not cancel previous enhancements: see issues in the revision notes of Buildings.Controls.OBC.CDL.Logical.Timer as well as Issue2101 timer #2104 (comment)

@AntoineGautier AntoineGautier self-assigned this Jul 24, 2024
@AndreaBartolini
Copy link
Contributor Author

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
Copy link
Contributor

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

  • converts these state events into time events (both with Dymola and Optimica),
  • resolves the discrepancy between OMC and Dymola,
  • ensures passed=u if t=0.

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
Copy link
Contributor

AntoineGautier commented Jul 25, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

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.

@AntoineGautier
Copy link
Contributor

@mwetter I've tested the Timer block with an input signal computed from a sine signal and an instance of GreaterThreshold. I've compared the existing implementation (Timer.log attached) with the simplified implementation below (TimerSimple.log attached).

block TimerSimple
...
initial equation 
  entryTime = time;
equation 
  when u then
    entryTime=time;
  end when;
  y = if u then time - entryTime else 0.0;
  passed = y >= t and u;

There are 2 state events logged in Timer.log, which are needed to compute the input signal from the sine signal. But the equations from the timer are solved with time events only.
In contrast, the simplified implementation requires 2 additional state events.
One thing I still struggle to understand is that Dymola still reports "Iterating to find consistent restart conditions" during the model time events. So I don't know if these iterations are really less costly than the ones occurring during the state events with the alternative implementation.

Timer.log
TimerSimple.log

@mwetter
Copy link
Member

mwetter commented Oct 18, 2024

@AntoineGautier @AndreaBartolini : In view of the above tests with Timer.log and TimerSimple.log we should stick with the current implementation that produces time rather than state events.
This also gives a higher chance that simultaneous events are indeed simultaneous, e.g., Timer has an event at 0.3 while TimerSimple has it at 0.3000000000002457 (due to it being a state event).

I don't know how to interpret "Iterating to find consistent restart conditions", perhaps that is an event iteration until no discrete variables change at the current event time.

@AndreaBartolini
Copy link
Contributor Author

I think that "Iterating to find consistent restart conditions" means that Dymola are looking if some other condition are changed after the last iteration, in order to decide if another iteration is needed or not.

@casella , can you contribute in this discussion?

@casella
Copy link
Contributor

casella commented Oct 20, 2024

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.

A square wave will trigger another event for the falling front, which will unnecessarily interrupt and slow down the continuous-time integration.

The event iteration mechanism is well defined in Appendix B of the Modelica Language Specification, so I think it is OK, per se.

We identified an issue with time events in OMC, namely that the fact they are not handled as such, but rather as state events, causes problems with simultaneous time-events, see #2152. This is a long-standing issue that needs to be fixed with high priority.

@AndreaBartolini, do you think that could be the cause of this issue also?

@AndreaBartolini
Copy link
Contributor Author

I think this case in another story, here there are two issues, one is the way in which the timerWithReset is written (see OpenModelica/OpenModelica#12746) and some proposal of modification are shown above, the other is that it seems that OMC skips an event iteration.

Maybe the fix of #2152 solves the problem but in my opinion the issue that involves OMC should be in any case investigated.

@casella
Copy link
Contributor

casella commented Oct 21, 2024

I think that "Iterating to find consistent restart conditions" means that Dymola are looking if some other condition are changed after the last iteration, in order to decide if another iteration is needed or not.

I think this is just refers to the event iteration mechanism - the iterations are stopped when the discrete variables do not change after re-evaluating the active conditions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants