ModelicaStandardLibrary icon indicating copy to clipboard operation
ModelicaStandardLibrary copied to clipboard

Controlled drives

Open AHaumer opened this issue 3 years ago • 9 comments

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 avatar May 10 '21 18:05 AHaumer

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

christiankral avatar May 24 '21 15:05 christiankral

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.

dietmarw avatar Jun 09 '21 10:06 dietmarw

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

TManikantan avatar Apr 05 '23 11:04 TManikantan

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

dietmarw avatar Apr 05 '23 11:04 dietmarw

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.

TManikantan avatar May 15 '23 13:05 TManikantan

https://github.com/modelica/ModelicaStandardLibrary/pull/3807#issuecomment-1547850161

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.

MartinOtter avatar May 16 '23 09:05 MartinOtter

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

AHaumer avatar May 16 '23 10:05 AHaumer

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.

dietmarw avatar May 17 '23 10:05 dietmarw

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?

casella avatar Jan 17 '24 00:01 casella