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

Eddy current sources #3976

Merged
merged 9 commits into from
Jun 10, 2022
Merged

Eddy current sources #3976

merged 9 commits into from
Jun 10, 2022

Conversation

AHaumer
Copy link
Contributor

@AHaumer AHaumer commented Apr 19, 2022

Just a small backwards compatible improvement of already existing components:
Modelica.Mechanics.{Rotational, Translational}.Sources.EddyCurrent{Torque, Force} would be much more versatile if they would enable (optional) influence of excitation.

@AHaumer AHaumer added enhancement New feature or enhancement L: Mechanics.Rotational Issue addresses Modelica.Mechanics.Rotational L: Mechanics.Translational Issue addresses Modelica.Mechanics.Translational labels Apr 19, 2022
@AHaumer AHaumer self-assigned this Apr 19, 2022
@AHaumer AHaumer enabled auto-merge April 19, 2022 17:24
Copy link
Member

@dietmarw dietmarw 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 but the unrelated (dummy) change should be removed from DemonstrateLightning.mo for this PR.

@dietmarw dietmarw added this to the MSL4.1.0 milestone Apr 20, 2022
@AHaumer AHaumer requested a review from dietmarw April 20, 2022 07:11
Copy link
Member

@dietmarw dietmarw left a comment

Choose a reason for hiding this comment

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

Still contains changes to DemonstrateLightning.mo unfortunately.

@AHaumer
Copy link
Contributor Author

AHaumer commented Apr 20, 2022

Still contains changes to DemonstrateLightning.mo unfortunately.

Could you please point me to the changes, or just revert them? I donÄt see more than two empty lines that I removed.

Copy link
Member

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

@beutlich
Copy link
Member

beutlich commented Apr 21, 2022

Please squash commits to a single one before merge!

removed unrelated changes (empty lines) in DemonstrateLightning.mo

More realistic modeling of influence of excitation

implemented optional influence of variable excitation
Copy link
Contributor

@tobolar tobolar left a comment

Choose a reason for hiding this comment

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

@AHaumer Very nice implementation!
Minor comments:

  1. To my understanding, excitation shall be between 0 and 1. If true, this shall be documented as well. Shall also ´excitation` be limited to these values in the eddy current brakes?
  2. Maybe the equations for tau and f can be given in the documentation as well - for better understanding.

Note: Changes in examples will trigger discrepances in regression tests.

@AHaumer
Copy link
Contributor Author

AHaumer commented Apr 26, 2022

@AHaumer Very nice implementation! Minor comments:

1. To my understanding, `excitation` shall be between 0 and 1. If true, this shall be documented as well. Shall also ´excitation` be limited to these values in the eddy current brakes?

2. Maybe the equations for `tau` and `f` can be given in the documentation as well - for better understanding.

Note: Changes in examples will trigger discrepances in regression tests.

You are right, negative excitation doen't make sense.
Excitation greater 1 does make sense, at least for a short period of time we could over-excite.
I'll consider to include the equations fpr tau and f in the documentation.
Bear in mind that the components were already implemented in MSL, this is just am enhancement with variable excitation.

@AHaumer
Copy link
Contributor Author

AHaumer commented Apr 26, 2022

One could even provide an auxilliary model consisting of resistance and inductance of the excitation winding, providing the excitaton signal (i.e. the magnetic flux of the inductance). This way one could model control of the excitation current as wel as the thermal analysis of the excitaton winding in a realistic way.

@beutlich beutlich disabled auto-merge May 1, 2022 18:55
@beutlich beutlich enabled auto-merge (squash) May 1, 2022 18:55
@beutlich
Copy link
Member

beutlich commented May 1, 2022

Please squash commits to a single one before merge!

I enabled auto-merge with squash to merge the changes as a single commit.

@beutlich beutlich merged commit 569c525 into modelica:master Jun 10, 2022
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.

On my opinion negative currents shall be allowed, as the sign of the current does not affect the generated torque. Additionally, overloading the eddy current source shall be possible. So I am in favor of not limiting the range of the current input.

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: Mechanics.Rotational Issue addresses Modelica.Mechanics.Rotational L: Mechanics.Translational Issue addresses Modelica.Mechanics.Translational
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants