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

Remove legacy initialization of Modelica.Blocks.Continuous.{PID, LimPID} #2898

Merged
merged 1 commit into from
Jun 30, 2019

Conversation

beutlich
Copy link
Member

@beutlich beutlich commented Apr 12, 2019

@HansOlsson This is what I tried to remove the now invalid modifier Init.DoNotUse_InitialIntegratorState. But no matter what I did, it did not work out, since enumeration simplification did not run when converting ModelicaTestConversion4.Blocks.Issue2892.

convertModifiers({"Modelica.Blocks.Continuous.LimPID",
                  "Modelica.Blocks.Continuous.PID"},
                  {"initType"}, {"initType=if Integer(%initType%) < 5 then %initType% else Modelica.Blocks.Types.Init.NoInit"}, true);

Do you have any hint how to do it?

Closes #2892.

@beutlich beutlich added the L: Blocks Issue addresses Modelica.Blocks label Apr 12, 2019
@beutlich beutlich added this to the MSL4.0.0 milestone Apr 12, 2019
@beutlich
Copy link
Member Author

@HansOlsson Friendly reminder.

@HansOlsson
Copy link
Contributor

@HansOlsson Friendly reminder.

I haven't forgotten it. But I think it will take some time to look at and I haven't had that time.

@dietmarw dietmarw requested review from AHaumer and MartinOtter and removed request for dietmarw May 13, 2019 12:06
@HansOlsson
Copy link
Contributor

@HansOlsson This is what I tried to remove the now invalid modifier Init.DoNotUse_InitialIntegratorState. But no matter what I did, it did not work out, since enumeration simplification did not run when converting ModelicaTestConversion4.Blocks.Issue2892.
convertModifiers({"Modelica.Blocks.Continuous.LimPID",
"Modelica.Blocks.Continuous.PID"},
{"initType"}, {"initType=if Integer(%initType%) < 5 then %initType% else Modelica.Blocks.Types.Init.NoInit"}, true);

The problem with the above is that it uses the non-existent enumeration element. IF the value is literal I assume it would be possible to find some solution, but if initType is non-literal (e.g. a propagated parameter) I don't see how it could be done in one go with current conversions.

I see the following possibility

Just convert the enumeration element - not the modifier! I tried the following alternatives:

convertClass("Modelica.Blocks.Types.InitPID.DoNotUse_InitialIntegratorState",
"Modelica.Blocks.Types.Init.NoInit");
convertElement("Modelica.Blocks.Types.InitPID","DoNotUse_InitialIntegratorState",
              "NoInit");

And even if the second may seem more correct (even if neither is really good), the first worked better in Dymola; and I can understand that it is similar for other tools.

I didn't fully understand if that handled the conversion completely or not. If it doesn't something more advanced is needed.

Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments in conversation. The completed part seems good.

@beutlich
Copy link
Member Author

beutlich commented Jun 24, 2019

This PR is now ready for review.

And even if the second may seem more correct (even if neither is really good), the first worked better in Dymola; and I can understand that it is similar for other tools.

I didn't fully understand if that handled the conversion completely or not. If it doesn't something more advanced is needed.

It required all three commands

convertClass("Modelica.Blocks.Types.Init.DoNotUse_InitialIntegratorState",
              "Modelica.Blocks.Types.Init.NoInit");
convertClass("Modelica.Blocks.Types.InitPID.DoNotUse_InitialIntegratorState",
              "Modelica.Blocks.Types.Init.NoInit");
convertClass("Modelica.Blocks.Types.InitPID",
              "Modelica.Blocks.Types.Init");

to successfully run the conversion of ModelicaTestConversion4.Blocks.Issue2892 in Dymola 2020.

@beutlich beutlich marked this pull request as ready for review June 24, 2019 10:04
@beutlich beutlich requested a review from HansOlsson June 24, 2019 10:04
@beutlich beutlich added the task General work that is not related to a bug or feature label Jun 24, 2019
@beutlich beutlich assigned beutlich and unassigned HansOlsson Jun 24, 2019
@beutlich beutlich changed the title Remove legacy initialization of PID/LimPID Remove legacy initialization of Modelica.Blocks.Continuous{PID, LimPID} Jun 24, 2019
@beutlich beutlich changed the title Remove legacy initialization of Modelica.Blocks.Continuous{PID, LimPID} Remove legacy initialization of Modelica.Blocks.Continuous.{PID, LimPID} Jun 24, 2019
Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Convert enumeration type InitPID to Init
@beutlich beutlich removed the request for review from MartinOtter June 30, 2019 12:43
@beutlich beutlich merged commit 49460b0 into modelica:master Jun 30, 2019
@beutlich beutlich deleted the remove-InitPID branch June 30, 2019 12:45
beutlich added a commit to beutlich/ModelicaStandardLibrary that referenced this pull request May 14, 2020
In 49460b0 for modelica#2898 the default initType of PID was converted from InitPID.DoNotUse_InitialIntegratorState to Init.InitialState. For LimPID it was different, that is from InitPID.DoNotUse_InitialIntegratorState to Init.NoInit. The conversion rule was added to convert from InitPID.DoNotUse_InitialIntegratorState to Init.NoInit, matching the LimPID manual conversion.

It was reported that the conversion to Init.NoInit breaks the initialization and model structure of user models. Despite Init.NoInit being the default initType for PI, the default initialization for PID and LimPID should better be converted to Init.InitialState. No matter if Init.NoInit or Init.InitialState, the initialization of the converted PID or LimPID component behaves differently since there is no compatibility preserving conversion possible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Blocks Issue addresses Modelica.Blocks task General work that is not related to a bug or feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is InitPID.DoNotUse_InitialIntegratorState still needed?
3 participants