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

Controlled drives #3807

Draft
wants to merge 21 commits into
base: master
Choose a base branch
from
Draft

Controlled drives #3807

wants to merge 21 commits into from

Conversation

AHaumer
Copy link
Contributor

@AHaumer AHaumer commented May 10, 2021

I'd like to enhance the examples Modelica.Electrical.Machines.Examples.ControlledDCDrives, including a UseresGuide with some explanations. The delays and the timing that have to be taken into account are explained in more detail.
For a proper solution, I'd need to merge #3806 first. I.e. after that, I'll replace the instance of the enhanced block "Mean" by the block from master.

@AHaumer AHaumer added enhancement New feature or enhancement L: Electrical.Machines Issue addresses Modelica.Electrical.Machines example Issue only addresses example(s) labels May 10, 2021
@AHaumer AHaumer added this to the MSL4.0.1 milestone May 10, 2021
@AHaumer AHaumer requested a review from christiankral May 10, 2021 18:44
@AHaumer AHaumer self-assigned this May 10, 2021
@beutlich beutlich modified the milestones: MSL4.0.1, MSL4.1.0 May 11, 2021
@AHaumer AHaumer marked this pull request as draft May 12, 2021 08:30
@AHaumer AHaumer marked this pull request as ready for review May 12, 2021 09:13
@AHaumer AHaumer requested a review from dietmarw May 12, 2021 09:13
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.

I have some comments here:

  • Is Modelica.Electrical.Machines.Examples.ControlledDCDrives.UsersGuide.Contact required, as we already provide the contact information in Modelica.Electrical.Machines.UsersGuide.Contact? I think this class can just be deleted.
  • I propose to merge Modelica.Electrical.Machines.Examples.ControlledDCDrives.UsersGuide.References with Modelica.Electrical.Machines.UsersGuide.References
  • In order to better understand Modelica.Electrical.Machines.Examples.ControlledDCDrives.UsersGuide it makes sense to directly refer to the used references in the text
  • Depending on the outcome of the discussion of Unify spelling of controllers #3814 the controllers may have to be spelled as, e.g., PI controller or PI-controller instead of PI - controller

AHaumer and others added 2 commits May 24, 2021 11:24
…Guide/DiscreteControl.mo

Co-authored-by: Christian Kral <[email protected]>
…Guide/CascadedControl.mo

Co-authored-by: Christian Kral <[email protected]>
AHaumer added 3 commits May 24, 2021 11:31
…chines/UsersGuide), moved References to Machines/UsersGuide/References
…aStandardLibrary into ControlledDrives

# Conflicts:
#	Modelica/Electrical/Machines/Examples/ControlledDCDrives/UsersGuide/References.mo
@christiankral
Copy link
Contributor

OK, the structure looks good so far. Let's wait for input from #3814 to fully approve this PR.

@dietmarw
Copy link
Member

dietmarw commented Jun 9, 2021

I have one more general comment of these types of examples that depend on new components that are then placed into a subpackage (e.g., Utilities). The problem here is that we in future say, well things inside Examples should not be extended from so we do not have to care about backward compatibility. So this leads to users getting broken models when making use of Utilities/* models. The question here is, shouldn't be the purpose of examples be to demonstrate what you can do with the respective library without having to come up with additional components (which then get parked away inUtilities or similar). And if an example is depending on such an additional component then does this not signal that the actually library is still missing something.

Personally I can think of many sophisticated examples that are based on MSL but are lacking one or two special components. But that should not be a reason to put those examples into the MSL. Rather make the MSL examples show what you can do with the available standard components. And if it turns out that there is such an important example that is missing some key-component, then maybe this key component should be added to the library the example is showing off. And if it turns out that it is not of such a general importance then maybe the example should be changed to not depend on such additional components.

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

@dietmarw would you please have a look at the fix provided?

@dietmarw
Copy link
Member

dietmarw commented Apr 5, 2023

@TManikantan Before sending these reminders it would help if you would read what was said in the ticket. Also it might perhaps be a better idea that you make yourself familiar with the people involved in the project.

@TManikantan
Copy link
Contributor

TManikantan commented May 15, 2023

We concur with @dietmarw response. Use of models inside the Utilities sub package is not an ideal way of showcasing the capabilities of MSL models. However, this is a recurring issue in multiple subpackages – listed in option 2. We suggest two options:
1.Given that this ticket deals with examples in ControlledDCDrives, and they seem to be working fine, let us close this ticket after #3814 is addressed.
2.There are multiple instances in other sub-packages too. Classes which are not backward compatible should be moved out of Utilities into respective packages if they are general standard component. However, these should be addressed carefully as a separate issue per package.

Few Examples which depend on new components placed in utilities :

  • Modelica.Blocks.Examples.Noise.Densities
  • Modelica.Clocked.Examples.Systems.ControlledMixingUnit
  • Modelica.StateGraph.Examples.ControlledTanks
  • Modelica.Magnetic.FluxTubes.Examples.MovingCoilActuator.ForceCurrentBehaviour
  • Modelica.Mechanics.MultiBody.Examples.Loops.EngineV6

Regarding the components in this Utilities package, here are some of our suggestions:

  • Modelica.Electrical.Machines.Examples.ControlledDCDrives.Utilities.LimitedPI looks similar to the component
    Modelica.Blocks.Continuous.LimPID. Suggestion would be to Reuse Modelica.Blocks.Continuous.LimPID and if not
    possible create a new component for controllers than working on top of a controller strategy.

  • Specific controllers i.e current, speed and position controllers have the potential to be reused and Suggestion would be to
    add these controllers in another dedicated location inside DC machines in
    Modelica.Electrical.Machines.BasicMachines.DCMachines or at a similar hierarchy.

@AHaumer AHaumer marked this pull request as draft May 16, 2023 05:03
@MartinOtter
Copy link
Member

#3807 (comment)

Since these are nice running examples, I would also accept them as they are now (suggestion (1)).
Afterwards, one should open a new ticket, to move sub-models below Utilities that are also useful in another context (LimitedPI, Battery,. ..) into a better place (maybe with improved documentation) and then modify these examples correspondingly.

@AHaumer
Copy link
Contributor Author

AHaumer commented May 16, 2023

Since this PR is open for a long time, some aspects have changed and I'd prefer to check and modify the examples beforehand.

@dietmarw
Copy link
Member

Since these are nice running examples, I would also accept them as they are now (suggestion (1)). Afterwards, one should open a new ticket, to move sub-models below Utilities that are also useful in another context (LimitedPI, Battery,. ..) into a better place (maybe with improved documentation) and then modify these examples correspondingly.

I'm sorry @MartinOtter I strongly disagree here. Exactly this kind of routine has led to an MSL that currently is on the brink of staying maintainable (given our lack of resources and engagement). So instead of adding new features that are not quite rightly implemented and require more work later on (and also much more involved because of conversion scripts, when, by whom) I rather suggest we do it right the first time. And if it is too much work now, then maybe not add this to the MSL then since it will definitely end up becoming more clean-up work later on.

@casella
Copy link
Contributor

casella commented Jan 17, 2024

Personally I can think of many sophisticated examples that are based on MSL but are lacking one or two special components. But that should not be a reason to put those examples into the MSL. Rather make the MSL examples show what you can do with the available standard components. And if it turns out that there is such an important example that is missing some key-component, then maybe this key component should be added to the library the example is showing off. And if it turns out that it is not of such a general importance then maybe the example should be changed to not depend on such additional components.

I respectfully disagree. It is (quite often) the case that one can re-use 70-90% of components provided by the MSL (or any other library) and requires to develop a few models on his/her own. This is a very common pattern and I can't see anything wrong in doing that in Examples. In fact, I find it of educational value. The components put there may not be general enough to deserve a place in the library, this is also perfectly legitimate.

I understand we have a potential problem with dependencies because people could start using the models found in Examples and then we create further backward compatibility issues. This could be resolved by placing Examples in a separate package, but that's not really practical. I think we should resolve this issue simply by having dedicated dependency analysis tools that simply skip packages named Examples (or possibly marked by a suitable ExamplePackage annotation).

Restricting the example to toy models that are exclusively built with basic components from the MSL seems an unnecessary restriction to me.

IMHO if this is the only reason why this PR is still not merged after 2 1/2 years, I would merge it right away. This is the current situation

  • one library officer is the proponent
  • the other library officer is OK with the PR
  • the current MAP-Lib leader is OK with the PR if there are no other issues than the examples
  • the previous deputy MAP-Lib leader is OK with the PR
  • the former MAP-Lib leader objects to having new models in Examples

To me the situation is pretty clear. Any comments?

@AHaumer AHaumer removed this from the MSL4.1.0 milestone Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement example Issue only addresses example(s) L: Electrical.Machines Issue addresses Modelica.Electrical.Machines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants