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

FixedRotation uses the non SI unit Angle_deg #3158

Open
otronarp opened this issue Oct 18, 2019 · 5 comments
Open

FixedRotation uses the non SI unit Angle_deg #3158

otronarp opened this issue Oct 18, 2019 · 5 comments
Assignees
Labels
L: Mechanics.MultiBody Issue addresses Modelica.Mechanics.MultiBody

Comments

@otronarp
Copy link
Member

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.

@beutlich beutlich added the L: Mechanics.MultiBody Issue addresses Modelica.Mechanics.MultiBody label Oct 18, 2019
@tobolar
Copy link
Contributor

tobolar commented Oct 21, 2019

@MartinOtter Any concern to change this?

@MartinOtter
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

tobolar commented Dec 3, 2019

@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
Copy link
Contributor

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Mechanics.MultiBody Issue addresses Modelica.Mechanics.MultiBody
Projects
None yet
Development

No branches or pull requests

5 participants