-
Notifications
You must be signed in to change notification settings - Fork 163
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
Updates to the Motor Drive Package #4103
base: issue2948_MotorDriver_update
Are you sure you want to change the base?
Updates to the Motor Drive Package #4103
Conversation
…hermoFluid/package.oder
…lanced/Loads/Examples/MotorDrive/InductionMotors/BaseClasses directory
…lanced/Loads/Examples/MotorDrive/InductionMotors/Examples directory
…lanced/Loads/Examples/MotorDrive directory
…lanced/Loads/MotorDrive/Coupled/Examples directory
"Specific heat capacity of medium 2 at default medium state"; | ||
|
||
equation | ||
chi.P = tauChi*Buildings.Utilities.Math.Functions.smoothMax(spe.w,1e-6,1e-8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ViswanathanGanesh01 Can we create a base class in the package ThermoFluid
, to do the calculation? The base class would have inputs P
, and w
, output tau
, and parameter for the 1e-6
and 1e-8
.
This will avoid missing the equations and connections in the equation
section, and avoid repeat the pattern among Chiller
, HeatPump
, and Pump
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Included a baseclass function.
annotation (Placement(transformation(extent={{66,28},{86,48}}))); | ||
equation | ||
// Assign values for motor model calculation from electrical interface | ||
theta_s = PhaseSystem.thetaRef(terminal.theta) "phase angle"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to create base class to do the calculations as showing in these equations? it seems that we have same pattern as in the SquirrelCageDrive
?
"Mechanical connector" | ||
annotation (Placement(transformation(extent={{90,-10},{110,10}}))); | ||
|
||
BaseClasses.MotorMachineInterface torSpe( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the missing comments. All the instances should have the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Included comments for all instances.
@@ -0,0 +1,185 @@ | |||
within Buildings.Electrical.AC.ThreePhasesBalanced.Loads.MotorDrive.InductionMotors.BaseClasses; | |||
model MotorModel1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No Dr. Hu. I have removed the class and updated the package.order
points={{60,0},{100,0}}, | ||
color={0,0,127}, | ||
thickness=1)); | ||
annotation (Icon(coordinateSystem(preserveAspectRatio=true, extent={{-60, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check the icon layer diagram. The connectors are in wrong position.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the Icon Layer and connectors to be aligned,
@@ -0,0 +1,193 @@ | |||
within Buildings.Electrical.AC.ThreePhasesBalanced.Loads.MotorDrive.InductionMotors.BaseClasses; | |||
model MotorModel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comments to the class and all the instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Included Comments to the class and all instances.
91.35,90},{91.35,89.9},{88.7,89.9}}, color={0,0,127})); | ||
connect(v_qs, i_qs_block.v_qs) | ||
annotation (Line(points={{-160,100},{64,100},{64,99.8}}, color={0,0,127})); | ||
annotation ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the class documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added Documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ViswanathanGanesh01 Please see inline comments.
- avoid mixing the equations and connections. You may create base classes.
- check if all the instances have comment.
- check if all the class have the documentation. Please make sure each class has a description comment.
- add documentation to each
package.mo
file.
…nductionMotors/BaseClasses/MotorModel1.mo Duplicate file.
Removed MotorModel1
Run the unittest with Optimica. We can following warnings:
It seems that there are some self-connecting connector. Please double check the models and the instanced classes. |
…nductionMotors/BaseClasses/computeElectricalParameters.bak-mo
…hermoFluid/BaseClasses.mo
@ViswanathanGanesh01 Please let me know when it is ready for review. |
@JayHuLBL Hello Dr. hu, the package is ready for review. Sorry for the delay. |
The updated package includes