Opened 11 years ago

Closed 2 years ago

Last modified 2 years ago

#2565 closed defect (fixed)

Problems with code generation - package constants in ExternalMedia

Reported by: Francesco Casella Owned by: Per Östlund
Priority: critical Milestone: 1.21.0
Component: New Instantiation Version: trunk
Keywords: Cc: Adrian Pop

Description

I am trying to make the ExternalMedia library to run in OMC. The library can be checked out from the MA SVN repo

https://svn.modelica.org/projects/ExternalMediaLibrary/trunk

When I try to compile the model

ExternalMedia.Test.TestMedium.TestBasePropertiesExplicit

I get the following errors from the gcc compiler:

ExternalMedia.Test.TestMedium.TestBasePropertiesExplicit.c:81: error: '$PMedium$PfluidConstants$lB1$rB$PcriticalTemperature' undeclared

ExternalMedia.Test.TestMedium.TestBasePropertiesExplicit.c:90: error: '$PMedium$PfluidConstants$lB1$rB$PcriticalPressure'

etc.

Can you have a look at that? These are all package constants.

Change History (34)

comment:1 by Adrian Pop, 11 years ago

Component: Code GenerationFrontend
Owner: changed from Lennart Ochel to Adrian Pop
Status: newassigned

This is front-end and not back-end. That constant should be replaced during flattening.

comment:2 by Francesco Casella, 11 years ago

I have found a very simple workaround: I just redeclare all those package constants, providing them directly in the package declaration by means of literal Real constants:

package TweakedExternalMedia
  package TestMedium
    extends ExternalMedia.Media.TestMedium(
      externalFluidConstants = FluidConstants(
       iupacName=  "unknown",
       casRegistryNumber=  "unknown",
       chemicalFormula=  "unknown",
       structureFormula=  "unknown",
       molarMass=  0.028,
       criticalTemperature= 623.0,
       criticalPressure=  221e5,
       criticalMolarVolume= 15,
       acentricFactor=  0,
        triplePointTemperature=  280.0,
        triplePointPressure=  500.0,
        meltingPoint=  280,
        normalBoilingPoint=  380.0,
        dipoleMoment=  2.0));
  end TestMedium;

  model TestStatesSat 
    extends ExternalMedia.Test.TestMedium.TestStatesSat(
       redeclare package Medium = TweakedExternalMedia.TestMedium);
  end TestStatesSat;
end TweakedExternalMedia;

The model TweakedExternalMedia.TestStatesSat is now flattened correctly. This is not very elegant, because the medium model should get these constants from the external C code, but it is no big deal to temporarily provide this data manually. I'll keep the ticket open, because eventually the front end should take care of this automatically.

comment:3 by Adrian Pop, 11 years ago

I don't think is needed to be done like this.
I think it will work if you directly define inside:
ExternalTwoPhaseMedium

String mediumName
String libraryName
String systemName

and not in the extends.

comment:4 by Francesco Casella, 11 years ago

Resolution: fixed
Status: assignedclosed

OK, I removed the redeclaration of mediumName from the extends clause (see r7296 of the svn.modelica.org/projects/ExternalMediaLibrary/trunk repo) and I no longer get the problem I reported.

The only minor issue is that I cannot change the mediumName string compared to the base class PartialTwoPhaseMedium, otherwise I get an error because the elements are not identical. This is no big deal, as this string will be changed in further redeclarations, when the medium-specific strings are set (see, e.g., ExternalMedia.Examples.WaterIF97).

Thank you Adrian for the suggestion!

comment:5 by Adrian Pop, 11 years ago

Resolution: fixed
Status: closedreopened

I'll keep this opened until I really fix the issue.

comment:6 by Francesco Casella, 11 years ago

I'm sorry, I fumbled up with the testing. The reason why I din't get the problem reported is that I ran a different test model which did not use package constants. Unfortunately the problem is still there if you try to use those constants.

You can now reproduce the error by using ExternalMedia.Test.TestMedium.TestConstants, which is as simple as possible.

There is one change in the behaviour of the compiler, though. Using yesterday's nightly build (r19175), I get messages = "Failed to build model: ExternalMedia.Test.TestMedium.TestConstants", while a previous nightly (r19047) led to the C-code errors originally reported in this ticket. This holds irrespective of the change in the library made in r7296. Apparently the front end is now able to recognize that there is some problem, but it still cannot handle it. BTW, I would suggest to provide a bit more information in the error message.

Anyway, it should be possible to break the circular dependency in the extends clause by splitting the declaration of ExternalTwoPhase medium in two stages, using inheritance. I'll work on that a bit and see what I can do.

comment:7 by Francesco Casella, 11 years ago

OK, it seems that the change in ExternalMedia r7296 actually works, but then there is another problem with code generation so the link to the Include annotation is lost for functions which are evaluated at compile time for package constant binding (or something like that), see #2412

Temporary workaround: copy externalmedialib.h to C:\OpenModelica1.9.1Nightly\include\omc\c. A fix is already in the pipeline, and eventually also the circular dependency should be handled. Then the ticket will be closed :)

Version 1, edited 11 years ago by Francesco Casella (previous) (next) (diff)

comment:8 by Martin Sjölund, 11 years ago

Adrian; can you check this on Windows and link the error-messages?

On Linux the following works fine (after I got some hints from Francesco on how to build it):

loadModel(ExternalMedia);getErrorString();
simulate(ExternalMedia.Test.TestMedium.TestConstants);getErrorString();

Gives:

true
""
record SimulationResult
    resultFile = "/home/martin/tmp/ExternalMedia.Test.TestMedium.TestConstants_res.mat",
    simulationOptions = "startTime = 0.0, stopTime = 1.0, numberOfIntervals = 500, tolerance = 1e-06, method = 'dassl', fileNamePrefix = 'ExternalMedia.Test.TestMedium.TestConstants', options = '', outputFormat = 'mat', variableFilter = '.*', measureTime = false, cflags = '', simflags = ''",
    messages = "",
    timeFrontend = 3.160652631,
    timeBackend = 0.005224445,
    timeSimCode = 0.009258942000000001,
    timeTemplates = 0.003776748,
    timeCompile = 0.351857477,
    timeSimulation = 0.008373283,
    timeTotal = 3.539239611
end SimulationResult;
""

comment:9 by Martin Sjölund, 11 years ago

So for the libraries, I get on Linux:

clang -shared -o ExternalMedia_getMolarMass_0.so ExternalMedia_getMolarMass_0.o ExternalMedia_getMolarMass_0_records.o -Wl,-rpath="/home/martin/.openmodelica/libraries/ExternalMedia 3.2.1/Resources/Library" "-L/home/martin/.openmodelica/libraries/ExternalMedia 3.2.1/Resources/Library" -lExternalMediaLib -I"/home/martin/trunk/build/include/omc/c" "-I/home/martin/.openmodelica/libraries/ExternalMedia 3.2.1/Resources/Include" -fPIC  -L"/home/martin/trunk/build/lib/omc" -Wl,-rpath,'/home/martin/trunk/build/lib/omc'    -lOpenModelicaRuntimeC -llapack -lblas -lm -rdynamic -lm

While Francesco on Windows gets no includes to the library

comment:10 by Adrian Pop, 11 years ago

I used this script:

loadModel(Modelica); getErrorString();
loadFile("ExternalMedia 3.2.1/package.mo"); getErrorString();
simulate(ExternalMedia.Test.TestMedium.TestConstants); getErrorString();

So maybe is loadFile that has the issue?

comment:11 by Martin Sjölund, 11 years ago

I saw ==== Log /tmp/omc-rtest-build/openmodelica/typed-API/log-UriLookup.mos in the OM_WIN logs...

This has to work or the include is not used!

uriToFilename("modelica://ExternalMedia/Resources/Include");getErrorString();

comment:12 by Adrian Pop, 11 years ago

Aha! I'll check that.

comment:13 by Martin Sjölund, 11 years ago

Or actually... The internal command that does uri lookup has to work. It's different from that API call :(

comment:14 by Francesco Casella, 11 years ago

I get

true
""
true
""
("D:\\Lavoro\\ModelicaSVN\\ExternalMediaLibrary\\Modelica\\ExternalMedia 3.2.1/Resources/Include","")
""

I guess too many back-forward slashes around...

comment:15 by Adrian Pop, 11 years ago

realpath implementation changed a while back in Windows and returns backwards slashes.
I'll see if I can make it return forward slashes as GCC might have issues otherwise.

But I think that the problem is somehow not there as I don't see
any include other the default (include/omc/c) in the makefile or .log.

I'll do some debugging tomorrow.

comment:16 by Adrian Pop, 11 years ago

Bug #2412 (with the IncludeDirectory/LibraryDirectory) is now fixed.

I'll still leave this bug open until I commit my changes for better handling of package constants.

comment:17 by Francesco Casella, 10 years ago

Tried this with r22673. The model now compiles and simulates successfully, but it takes 4 minutes to compile and it generates over 600 MB of binary stuff, which is totally crazy for such a model. In comparision, Dymola takes 8 seconds to compile it, and generates 6 MB of binaries.

comment:18 by Martin Sjölund, 10 years ago

Which of the models is it that takes so long to compile?

comment:19 by Francesco Casella, 10 years ago

ExternalMedia.Test.TestMedium.TestBasePropertiesExplicit, as stated initially.

comment:20 by Martin Sjölund, 10 years ago

For me the object-files for that are only 2.6MB. And translation+compilation was around 30 seconds. Linking failed due to the library though. Tried reverting it, but ended up with no library. It's a little boring to try recompiling the library for Linux again.

I am fairly sure it should work better than it does for you, but it is hard for me to try debugging it...

comment:21 by Francesco Casella, 10 years ago

Cc: Adrian Pop added

I've retried with r22761 but I still get 79 dlls to compute the damn package constants. All the way from ExternalMedia_getMolarMass_0_records.c/.dll to ExternalMedia_getCriticalMolarVolume_79.c/.dll

I'm not sure if this is Windows-specific, I thought code generation would be platform-independent, thanks to MinGW, but I might be wrong.

As to the library compilation, you can ask Jorrit Wronski (jowr@…) - he's testing the library under Linux so he can help you with that. Maybe he could just commit the library binaries on SVN, though I don't know if static libraries are universal in Linux or rather installation- or version-specific.

comment:22 by Martin Sjölund, 10 years ago

The simulation executable does not depend on the dll's though. They are temporary ones in order to evaluate constants that depend on an external call during the compilation process.

comment:23 by Adrian Pop, 10 years ago

I did not push all my changes into the trunk/ so the way package constants that depend on external functions are handled is the same as before.
The next step is to not evaluate any constant or parameter if they are not structural or have annotation(Evaluate=true) and I'm working on it.

comment:24 by Martin Sjölund, 10 years ago

Milestone: 1.9.11.9.2

This ticket was not closed for 1.9.1, which has now been released. It was batch modified for milestone 1.9.2 (but maybe an empty milestone was more appropriate; feel free to change it).

in reply to:  19 comment:25 by Francesco Casella, 10 years ago

Priority: highcritical

After the last fix that affected ExternalMedia (r23968) I still get 79 dll's (more than 500 MB) and over four minutes of compilation time when trying to compile the very basic ExternalMedia.Test.TestMedium.TestBasePropertiesExplicit model under Windows/MinGW.

We have workarounds to avoid this phenomenon for our projects (hard-coding those constants as literals in the Modelica code for each separate medium model), but this is still quite inconvenient and it would be nice to have the problem fixed once and for all.

Is there any progress on this front?

comment:26 by Martin Sjölund, 10 years ago

Milestone: 1.9.21.9.3

Milestone changed to 1.9.3 since 1.9.2 was released.

comment:27 by Martin Sjölund, 9 years ago

Milestone: 1.9.31.9.4

Moved to new milestone 1.9.4

comment:28 by Martin Sjölund, 9 years ago

Milestone: 1.9.41.9.5

Milestone pushed to 1.9.5

comment:29 by Martin Sjölund, 9 years ago

Milestone: 1.9.51.10.0

Milestone renamed

comment:30 by Martin Sjölund, 8 years ago

Milestone: 1.10.01.11.0

Ticket retargeted after milestone closed

comment:31 by Martin Sjölund, 8 years ago

Milestone: 1.11.01.12.0

Milestone moved to 1.12.0 due to 1.11.0 already being released.

comment:32 by Francesco Casella, 7 years ago

Component: FrontendNew Instantiation
Milestone: 1.12.02.0.0
Owner: changed from Adrian Pop to Per Östlund
Status: reopenedassigned

I am now resuming work on ExternalMedia, see https://github.com/modelica/ExternalMedia. We'll need to see if these problems are solved with the new front end as soon as it is capable to deal with Modelica.Media stuff.

comment:33 by Francesco Casella, 2 years ago

Milestone: 2.0.01.21.0
Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.