search

modelica-3rdparty / OpenHydraulics

A free Modelica package providing components describing 1-dimensional fluid flow in hydraulic circuits.
BSD 3-Clause "New" or "Revised" License
39 stars 25 forks source link

Bugs in GenericOil, use of Streams.print() vs assert(), real-equality-with-0 #10

Closed harmanpa closed 11 years ago

harmanpa commented 11 years ago

GenericOil references a type, Density, and a variable, T, neither of which is defined. Density is presumably SI.Density, T should be Toperating as this is done in dynamicViscosity()? (Though I'm not sure if a function should access a parameter in the enclosing scope).

ConstVolumeSource contains an if-equation that compares a Real parameter q with 0, however since the right-hand-side of the resulting equation is multiplied by q the same effect can be achieved by forcing q to be evaluated.

assert(...,...,AssertionLevel.warning) can now be used in place of issuing warnings via Streams.print() giving tool-dependent handling of warnings, this is used in FluidPower2MechTrans and DoubleActingCylinder.

xogeny commented 11 years ago

There is a problem here which is the use of assert. While it is completely legal to add AssertionLevel.warning to the asserts, the problem is that AssertionLevel is no supported by Dymola. Since Dymola is the tool that Chris is using to develop the library, it meas that accepting this commit will break the library for him and any other Dymola users.

I completely agree that this sucks. I've sent another note to Dymola support asking about this feature.

I would propose that this be split into two pull requests. One containing f1259a7 and the other containing 589da52 and 81fcb78. We could accept the latter and now (assuming there weren't any other issues there, I'd like to test them first) and leave the other one open. Pete could maintain a master branch that merged all three as an alternative for those having tools that support AssertionLevel in the short term.

Sorry. :-(

xogeny commented 11 years ago

I spoke with Pete and it looks like he was receptive to this idea of splitting this into two pull requests.

thorade commented 11 years ago

I believe the newest Dymola does support level=AssertionLevel.warning, at least I am using it in my code, e.g. here: https://github.com/thorade/HelmholtzMedia/blob/master/Interfaces/PartialHelmholtzMedium/isothermalThrottlingCoefficient.mo and it prints the message as a warning, i.e. without stopping the simulation.

Correct me if I'm wrong.

xogeny commented 11 years ago

This is correct, but I was unaware of it when I spoke with Pete. I was just informed of this issue today by DS.

My concern was that not enough people would have the latest version to justify putting this in. Perhaps a quick poll of people watching this project would lead shed some light on this.

I do not (yet) have 2013 FD01. But I could probably get it easily enough. Anybody have the update link?