search

modelica / ModelicaStandardLibrary

Free (standard conforming) library to model mechanical (1D/3D), electrical (analog, digital, machines), magnetic, thermal, fluid, control systems and hierarchical state machines. Also numerical functions and functions for strings, files and streams are included.
https://doc.modelica.org
BSD 3-Clause "New" or "Revised" License
481 stars 169 forks source link

FixedRotation uses the non SI unit Angle_deg #3158

Open otronarp opened 5 years ago

otronarp commented 5 years ago

Modelica.Mechanics.MultiBody.Parts.FixedRotation uses the non SI unit Angle_deg:

parameter Cv.NonSIunits.Angle_deg angle = ..
parameter Cv.NonSIunits.Angle_deg angles[3] = ...

Using non SI units in the interface is bad practice since it spreads to the rest of you your model if you want to propagate the parameter from a higher hierarchical level.

tobolar commented 5 years ago

@MartinOtter Any concern to change this?

MartinOtter commented 5 years ago

I agree that it would be better to have SI units in the interface of FixedRotation (the component uses deg instead of rad, because when it was introduced, tools did not support automatic conversion to from deg to rad and it was annoying to always introduce the conversion manually).

However, I do not think that it is possible to provide a conversion script in a way that existing models are still correct. Looks therefore dangerous to change it. Maybe it is better to mark this component as obsolet (and move it to the obsolete libary) and provide a new component. The FixedRotation object has probably also too many options and it takes therefore some time to grasp it.

One could therefore add a new, much simpler component, say FixedTranslationAndRotation which has two parameters: r to translate frame_b and angles to rotate frame_a around x-, y-, z-axis with angles. In the very unlikely case that also the options from FixedRotation are needed, it is always possible to use the functions from MultiBody.Frames to map to angles.

@tobolar can you provide this?

HansOlsson commented 5 years ago

I don't see the problem with a conversion script multiplying a parameter by pi/180.

When converting Modelica.Mechanics.MultiBody from version 2 to 3 this was done for the parameters for setting start-values in several components (Rotor, Revolute) etc.

However, in that case the parameter-name was sort of changed as well. And we could explicitly change the parameter-name as well here, since any missed conversion will then be detected.

If we want to be really safe and support models using the values of those parameters in other models we could add "final parameter" for the existing ones, and computed them from the new radian-values.

tobolar commented 4 years ago

@HansOlsson

And we could explicitly change the parameter-name as well here

I guess the parameters' names (angle, angles) are fine. Would the conversion also work if we don't rename it? E.g. in case of something like:

  import SI = Modelica.SIunits;
  import Cv = Modelica.SIunits.Conversions;
  Modelica.Mechanics.MultiBody.Parts.FixedRotation fixedRotation;
  Cv.NonSIunits.Angle_deg angleInherited_deg = fixedRotation.angle;
  SI.Angle angleInherited_rad = Cv.from_deg(fixedRotation.angle);
HansOlsson commented 2 years ago

@HansOlsson

And we could explicitly change the parameter-name as well here

I guess the parameters' names (angle, angles) are fine. Would the conversion also work if we don't rename it? E.g. in case of something like:

  import SI = Modelica.SIunits;
  import Cv = Modelica.SIunits.Conversions;
  Modelica.Mechanics.MultiBody.Parts.FixedRotation fixedRotation;
  Cv.NonSIunits.Angle_deg angleInherited_deg = fixedRotation.angle;
  SI.Angle angleInherited_rad = Cv.from_deg(fixedRotation.angle);

If you are extracting the parameter value it would currently only work if you have a final parameter for the old parameter and conversion to the new one; but I assume it would be possible to handle this special case as well.