Opened 4 years ago

Last modified 3 years ago

#6003 assigned defect

Code generation bug

Reported by: p.van.der.hulst@… Owned by: Karim Adbdelhak
Priority: high Milestone:
Component: Backend Version: v1.14.1
Keywords: Cc:

Description

Hi,

For a project at my new job I decided to move to openmodelica. My first experiment is an ideal time-division multiplexing system. It passes multiple signals through a single signal path during consecutive intervals.
To make the model more realistic, I added a low-pass filter in the signal path. Now I get a compilation error:
test_error_02nls.c: In function 'initializeStaticDataNLS14':
test_error_02nls.c:45:87: error: 'INTEGER_ATTRIBUTE {aka struct INTEGER_ATTRIBUTE}' has no member named 'nominal'

sysData->nominal[i] = data->modelData->integerVarsData[0].attribute /* MpxIndex.y */.nominal;

Previous reports of this error message seem to be related to an algebraic loop. Lo and behold: inserting a unit delay before the filter did solve the problem.
I find this strange, since at the model level it does not contain any loops.
The most surprising thing is that in any other simulation program, a low-pass filter breaks an algebraic loop because it places an integrator in the loop. Here it creates an algebraic loop!

I loaded the model in a trial version of dymola. Dymola has no problem with the model. Therefore I consider this an openmodelica bug.

I'll enclose a demo project (main model: test_error.mo)

Best regards,
Paul van der Hulst

PS I'm a newbie regarding (open)modelica, but have many years of experience simulating switched systems with simulink and various other simulation packages, including modification of an RK4 integration engine to properly handle state-events.

Attachments (1)

Modelica_test.zip (2.4 KB ) - added by p.van.der.hulst@… 4 years ago.
Demonstration of openmodelica generating erroneous code

Download all attachments as: .zip

Change History (11)

by p.van.der.hulst@…, 4 years ago

Attachment: Modelica_test.zip added

Demonstration of openmodelica generating erroneous code

comment:1 by Martin Sjölund, 4 years ago

Owner: changed from Lennart Ochel to Karim Adbdelhak
Status: newassigned

Running the code through the transformational debugger says this variable is defined in the initial system as:

(residual) CryoMultiplexer.y - CryoMultiplexer.u[MpxIndex.y] = 0

That is, it's trying to figure out its value by looking through some discrete indexes of the input?

Adding an initial equation for the output seems to work, but I'm not sure myself how and why these things should work.

The equations for CryoMultiplexer.y were:

inital:

(assign) CryoMultiplexer.y := LPfilter.uu[1] * LPfilter.u_nominal

continuous-time:

(assign) CryoMultiplexer.y := CryoMultiplexer.u[MpxIndex.y]

So somehow Modelica thinks that this equation is somehow a good initial value for MpxIndex.y :) Someone else can explain why.

comment:2 by Karim Adbdelhak, 4 years ago

The basic problem is that MpxIndex.y has no initial value and needs to be determined during intialization. The algorithm is not smart enough to figure out by itself that it needs to be fixed (this is a heuristic so you need to help it a little bit).

Since you already provided a start value for this discrete value you seem to know what it should like at the start. Just fix it with fixed=true and everything works fine.

In general: each state and each discrete variable needs to have either a fixed start value or an initial equation (which does not necessarily have to contain it, e.g. steady state initialization der(x) = 0.0. it just to be logically connected). I would not rely on the tool to figure out these things on its own, that can be weird sometimes.

The only thing i changed to get it running is updating line 5 and 6 in test_error.mo to be

TDMclock MpxIndex(nr_channels = nr_channels, period = T_sample, y(fixed=true)) annotation (
    Placement(visible = true, transformation(origin = {-68, 86}, extent = {{-10, -10}, {10, 10}}, rotation = 0)));

fixing MpxIndex.y. This should be the most correct way to do it.

Also consider fixing this value inside TDMclock itself. I do not see an application where you would use this unfixed.

Version 0, edited 4 years ago by Karim Adbdelhak (next)

comment:3 by p.van.der.hulst@…, 4 years ago

Hi,

Thanks for the suggestions.
Since the model compiles now, I can continue. Priority can be lowered as far as I'm concerned.
However, I do not understand if you consider this a bug or not. Dymola ran without complaints. Although I immediately accept that my model isn't very good, I don't think that the model is in violation with the language.
Openmodelica itself also did not detect any errors.
To say the least: the (compiler) error message was less than helpful.

Best regards,
Paul

in reply to:  3 comment:4 by Karim Adbdelhak, 4 years ago

Replying to p.van.der.hulst@…:

Hi,

Thanks for the suggestions.
Since the model compiles now, I can continue. Priority can be lowered as far as I'm concerned.
However, I do not understand if you consider this a bug or not. Dymola ran without complaints. Although I immediately accept that my model isn't very good, I don't think that the model is in violation with the language.
Openmodelica itself also did not detect any errors.
To say the least: the (compiler) error message was less than helpful.

Best regards,
Paul

As i said, it is a heuristic. It can probably be improved by forcing discrete variables to be solved explicitly and if it is not possible they have to be fixed by the compiler. Right now it only prefers discrete variables to be solved explicitly but does not force the fixing if it is not possible. In this particular case it should prevent the problem.

We did not really focus on problems like these since it only matters in fringe cases, most of the time discrete variables have an explicit way to be solved. But it is an enhancement that could be made for sure. For starters, i could add a better error message which points to the discrete variable which the compiler tries to solve implicitly and propose setting fixed = true, or providing a proper explicit initial equation.

comment:5 by Francesco Casella, 4 years ago

@Karim, do we have some warnings about missing initial equation for discrete variables? We have this for continuous states, but I am not sure about discrete ones.

If we don't, we should, and we should make clear that this is a potentially critical situation, that you should avoid by explicitly setting fixed = true or adding initial equations.

comment:6 by Francesco Casella, 4 years ago

Component: Code GenerationBackend
Milestone: 1.17.0

in reply to:  5 comment:7 by Karim Adbdelhak, 4 years ago

Replying to casella:

@Karim, do we have some warnings about missing initial equation for discrete variables? We have this for continuous states, but I am not sure about discrete ones.

If we don't, we should, and we should make clear that this is a potentially critical situation, that you should avoid by explicitly setting fixed = true or adding initial equations.

No i don't think that we have something like that. Probably easy to add though. But since you scheduled that for v1.17.0 i will not focus on it now.

comment:8 by Francesco Casella, 4 years ago

Agreed. I'd focus on 1.16.0 backend blockers first.

comment:9 by Francesco Casella, 4 years ago

Milestone: 1.17.01.18.0

Retargeted to 1.18.0 because of 1.17.0 timed release.

comment:10 by Francesco Casella, 3 years ago

Milestone: 1.18.0

Ticket retargeted after milestone closed

Note: See TracTickets for help on using tickets.