Skip to content

Conversation

@HansOlsson
Copy link
Contributor

Fix the unit for the inductances.
Eliminates non-local unit errors based on modelica/ModelicaSpecification#3266 (combined with other unit-changes.)

@dietmarw dietmarw added the L: Electrical.Machines Issue addresses Modelica.Electrical.Machines label Oct 28, 2022
@dietmarw dietmarw added this to the MSL4.1.0 milestone Oct 28, 2022
@dietmarw dietmarw added the enhancement New feature or enhancement label Oct 28, 2022
@HansOlsson
Copy link
Contributor Author

Note that there are similar-looking examples of unit-errors in other classes that isn't corrected in this PR, like Modelica.Magnetic.FundamentalWave.BasicMachines.SynchronousMachines

It would likely be possible to correct them as well in a similar way (but then there are messier ones as well - as the SquirrelCage).

@christiankral
Copy link
Contributor

This PR shall actually affect the machine models of three different packages as they all have the same shortcomings:

  • Modelica.Electrical.Machines.BasicMachines
  • Modelica.Magnetic.FundamentalWave.BasicMachines
  • Modelica.Magnetic.QuasiStatic.FundamentalWave.BasicMachines

Addirtionally I would like to discuss some aspects of this PR with @AHaumer first: I see three different "simple" fixes of this issue:

Option 1

Follow the proposal of this PR but change final constant SI.Resistance unitR=1 to final constant SI.Impedance unitZ=1, as the inductances are calculated from impedances, not resistances.

Option 2

Add parameters of the reference phase voltage and current to the machine models, according to the documentation and the original intention of the used paramerterization:

image

   parameter Modelica.Units.SI.Voltage VsRef = 100 " RMS reference phase voltage";
   parameter Modelica.Units.SI.Current IsRef = 100 " RMS reference phase current";
   final parameter Modelica.Units.SI.Impedance ZsRef = VsRef / IsRef "Reference phase impedance";

Use parameter ZsRef instead of unitZ then.

However, option 2. has the drawback, that additional parameters are used in the machine models. Even though this parameters do not change the model behavior it will most likely happen that a user model then shows the new parameters VsRef = 100 and IsRef = 100 which might not match the actual user parameters. This is a bad side effect which could confuse the users.

The parameters VsRef and IsRef could be made protected, but then it is questionable if there still exists and advantage over option 1.

Option 3

Replace the division by 2*pi*fsNominal by the term 100*pi and thus fully ignore the units in the default paramters.

@dietmarw
Copy link
Member

I agree that Option 2 is suboptimal if the new reference parameter has no function other than creating a new ZsRef to fix the unit. Although having them might finally explain to the user better what the Lx calculations in the data records of the examples are based on. So in I think:

  • Option 1: Pragmatic solution but seems like a "hack" to please the tool.
  • Option 2: Gives a bit more background info to the meaning and origin of the parameter calculations, more user friendly but at the same time also dangerous to be confusing (since Vref and Iref don't "do" anything).
  • Option 3: Makes it very hard to understand the equations and their physical meaning.

@HansOlsson
Copy link
Contributor Author

An option 4 would be to use:
final parameter Modelica.Units.SI.Impedance ZsRef = 1 "Reference phase impedance based on nominal voltage 100 V and nominal current 100 A; per phase";

(Possibly with better description.)

It avoids the extra variables of option 2.
Compared to option 1 it isn't just a magic unit conversion, but properly using a reference impedance (which happens to have the value 1).

@christiankral
Copy link
Contributor

I agree with @HansOlsson. I think option 4 is the way to go.

@christiankral
Copy link
Contributor

In order to provide a consistent solution

  • the protected parameter ZsRef is moved into the partial model Modelica.Electrical.Machines.Interfaces.PartialBasicInductionMachine
  • the resistances Rs and Rr are also multiplied by ZsRef, as this was the original intention of this default parameter set

@christiankral
Copy link
Contributor

One more thing to think of: In the slip ring induction machine model the rotor locked rotor voltage is based on the stator reference voltage of 100 V:

  parameter SI.Voltage VsNominal(start=100)
    "Nominal stator voltage per phase"
    annotation (Dialog(enable=not useTurnsRatio));
  parameter SI.Voltage VrLockedRotor(start=100*(2*pi*
        fsNominal*Lm)/sqrt(Rs^2 + (2*pi*fsNominal*(Lm + Lssigma))^2))
    "Locked-rotor voltage per phase"
    annotation (Dialog(enable=not useTurnsRatio));

This units of the start value of VrLockedRotor is actually also not "correct" with respect to units and its original inention. Therefore, a much better and consistent implementation were:

  parameter SI.Voltage VrLockedRotor(start=VsNominal*(2*pi*
        fsNominal*Lm)/sqrt(Rs^2 + (2*pi*fsNominal*(Lm + Lssigma))^2))
  ...   

It is, however, not backwards compatible (if VsNominal is changed by the user, and VrLockedRotor isn't changed).

@AHaumer @HansOlsson @dietmarw Any comments here?

@TManikantan TManikantan assigned HansOlsson and unassigned HansOlsson Feb 15, 2023
@TManikantan
Copy link
Contributor

TManikantan commented Feb 24, 2023

This pull request requires second reviewer to approve the fix to merge it successfully.
@AHaumer would you please review fix for this ticket? Please also let us know if additional info or summary would help you expedite the review.

@TManikantan
Copy link
Contributor

@AHaumer Would you please review the fix?

@TManikantan TManikantan removed the request for review from christiankral April 18, 2023 09:37
Copy link
Contributor

@AHaumer AHaumer left a comment

Choose a reason for hiding this comment

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

fine with me.

@AHaumer AHaumer requested a review from christiankral April 27, 2023 17:13
@tobolar tobolar changed the title Unit correctness. Fix SI units by introducing reference phase impedance ZsRef May 2, 2023
@TManikantan
Copy link
Contributor

@christiankral would you look into the fix and give a yes so we can go ahead and merge the PR?

Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or enhancement L: Electrical.Machines Issue addresses Modelica.Electrical.Machines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants