Enhance frictional models based on ExternalCombiTable1D

[beutlich]

@christoff-buerger This adds your proposal as PR. Note that I swapped the components of table_signs to match the columns of mu_pos.

Since CombiTable1Ds supports more interpolation smoothness kinds we also could get rid of

ModelicaStandardLibrary/Modelica/Mechanics/Rotational/Components/Clutch.mo

Lines 102 to 103 in 81dd1b6

	second column = mu). Currently, only linear interpolation in
	the table is supported.

, but might lose mu0 being a parameter again.

Closes #3661

[otronarp]

May I suggest a name change? Calling things Foo2 is not helpful to the user unless the two signifies that there are two of something within the model (and as far as I understand it that's not the case here, it's an alternative implementation right?).

[beutlich]

May I suggest a name change? Calling things Foo2 is not helpful to the user unless the two signifies that there are two of something within the model (and as far as I understand it that's not the case here, it's an alternative implementation right?).

It is an slightly improved implementation. I only did so for the sake of consistency since we already have ElastoBacklash and ElastoBacklash2 in Rotational.Components (and even other component models like Diode and Diode2).

[tobolar]

It is an slightly improved implementation.

Actually, this reimplementation shall have the same functionality as the original clutch model. So why is it necessary to double it?

[tobolar]

We shall also reimplement Mechanics.Rotational.Components.Brake and Mechanics.Rotational.Components.OneWayClutch accordingly.

[MartinOtter]

Very good proposal (the "current" friction elements had been implemented at a time, where no table interpolation was available and therefore a quick implementation with a Modelica function was made;).

However, it would be good to use the new friction elements at least in one example (maybe I missed this).

[beutlich]

However, it would be good to use the new friction elements at least in one example (maybe I missed this).

I already updated Modelica.Mechanics.Rotational.Examples.CoupledClutches to utilize Clutch2.

Currently, only linear interpolation in the table is supported.

@MartinOtter Do you agree to also add the interpolation smoothness as new parameter? In that case we no longer should use Modelica.Math.Vectors.interpolate to determine mu0.

[MartinOtter]

However, it would be good to use the new friction elements at least in one example (maybe I missed this).

I already updated Modelica.Mechanics.Rotational.Examples.CoupledClutches to utilize Clutch2.

O.k.

Currently, only linear interpolation in the table is supported.

@MartinOtter Do you agree to also add the interpolation smoothness as new parameter? In that case we no longer should use Modelica.Math.Vectors.interpolate to determine mu0.

Yes, smoothness makes sense and mu0 should no longer be used with Vectors.interpolate.

[beutlich]

@MartinOtter Do you agree to also add the interpolation smoothness as new parameter? In that case we no longer should use Modelica.Math.Vectors.interpolate to determine mu0.

Yes, smoothness makes sense and mu0 should no longer be used with Vectors.interpolate.

Done for Clutch2 for first review. @tobolar If you agree we should migrate Brake2 and OneWayClutch2 from block CombiTable1D to external object ExternalCombiTable1D, too.

[beutlich]

Actually, this reimplementation shall have the same functionality as the original clutch model. So why is it necessary to double the it?

I agree that the revised models Clutch2/Brake2/OneWayClutch2 fall back to the same functionality as the legacy models Clutch/Brake/OneWayClutch. There is no particular reason (except that protected mu0 changed variability from variable to parameter) to not simply update the original models. @MartinOtter needs to decide here. If he agrees we can avoid the duplication of the models.

[christoff-buerger]

Note that I swapped the components of table_signs to match the columns of mu_pos.

That is good (= more intuitive).

May I suggest a name change? Calling things Foo2 is not helpful to the user unless the two signifies that there are two of something within the model (and as far as I understand it that's not the case here, it's an alternative implementation right?).

It is an slightly improved implementation. I only did so for the sake of consistency since we already have ElastoBacklash and ElastoBacklash2 in Rotational.Components (and even other component models like Diode and Diode2).

I agree with @otronarp. We should avoid doubling and names like Foo2.

Is there any rationale or argument why we need two clutch implementations; why we have to keep the old as well? Does the redesign have any drawbacks (functional or non-functional like useabelility or performance)?

This is particularly important because

We shall also reimplement Mechanics.Rotational.Components.Brake and Mechanics.Rotational.Components.OneWayClutch accordingly.

which would result in a plethora of Foo2 things.

Do we also double the test models/examples utilizing the redesign? Or would we just not use one of the two implementations?

Thus, I like to push on:

I agree that the revised models Clutch2/Brake2/OneWayClutch2 fall back to the same functionality as the legacy models Clutch/Brake/OneWayClutch. There is no particular reason (except that protected mu0 changed variability from variable to parameter) to not simply update the original models. @MartinOtter needs to decide here. If he agrees we can avoid the duplication of the models.

@MartinOtter what is your opinion?

[christoff-buerger]

@tobolar: The Brake2 still uses:

 final parameter Real mu0 = Modelica.Math.Vectors.interpolate(mu_pos[:,1], mu_pos[:,2], 0, 1)

i.e., always linear segments as smoothness; likewise the OneWayClutch2.

I propose to handle it like @beutlich improved for Clutch2, i.e., to support different table smoothness.

[christoff-buerger]

Above two things, (1) can we avoid doubling of clutch, brake etc models and (2) consistent support for full table smoothness, have to be addressed from my point of view before a positive review.

[beutlich]

Above two things, (1) can we avoid doubling of clutch, brake etc models and (2) consistent support for full table smoothness, have to be addressed from my point of view before a positive review.

Note, that I added Clutch2 and updated it with full table smoothnes just as an offer to discuss with the library officers to get their opinion which way to head. Once this is clarified we can modify/add Brake and OneWayClutch accordingly.

[tobolar]

Clutch2 looks fine to me now and the smoothness option is great.

Since the piece of code

    if     smoothness == Types.Smoothness.ConstantSegments then TabInternal.getTable1DValueNoDer(tableID, 1, 0)
    elseif smoothness == Types.Smoothness.LinearSegments   then TabInternal.getTable1DValueNoDer2(tableID, 1, 0)
    else                                                        TabInternal.getTable1DValue(tableID, 1, 0)

will be used also in other elements (and they are not only Rotational, see #3664), we shall extract it into a separate function. What could be the best location for such a function?

[beutlich]

What could be the best location for such a function?

I also thought about this. My proposal is to introduce inline function Modelica.Blocks.Tables.getTable1DValue (or Modelica.Blocks.Tables.Functions.getTable1DValue) (taking the smoothness as argument) and calling the functions from Internal. @tobolar Can you do it?

[tobolar]

Can you do it?

I will prepare a suggestion.

[beutlich]

What I still not understand is why mu0 gets the Evaluate=true annotation, but depends on mu_pos being default-evaluated. I didn't question it at first sight when @christoff-buerger suggested it, but would like to get rid of it again now that mu0 is of parameter variability anyway.

@HansOlsson @christoff-buerger Can you please confirm that the Evaluate annotation for mu0 should be removed again. Thanks.

[HansOlsson]

What I still not understand is why mu0 gets the Evaluate=true annotation, but depends on mu_pos being default-evaluated. I didn't question it at first sight when @christoff-buerger suggested it, but would like to get rid of it again now that mu0 is of parameter variability anyway.

@HansOlsson @christoff-buerger Can you please confirm that the Evaluate annotation for mu0 should be removed again. Thanks.

To me "Evaluate=true" should be removed as mu0 should not be evaluated during translation, since it depends on the external table (and evaluating that during translation requires quite specific solution and doesn't really help).

[HansOlsson]

As indicated remove the Evaluate=true for mu0.

[beutlich]

As indicated remove the Evaluate=true for mu0.

Done. Please update your review. Thank you again!

[HansOlsson]

Looks good now.

[beutlich]

OK, waiting for @christoff-buerger's final approval then.

[MartinOtter]

I agree that the revised models Clutch2/Brake2/OneWayClutch2 fall back to the same functionality as the legacy models Clutch/Brake/OneWayClutch. There is no particular reason (except that protected mu0 changed variability from variable to parameter) to not simply update the original models. @MartinOtter needs to decide here. If he agrees we can avoid the duplication of the models.

As long as the changes are backwards compatible (e.g. only new parameters added, but not old parameters removed or changed), it is of course better to have only one model and not two different models.

[christoff-buerger]

What I still not understand is why mu0 gets the Evaluate=true annotation, but depends on mu_pos being default-evaluated. I didn't question it at first sight when @christoff-buerger suggested it, but would like to get rid of it again now that mu0 is of parameter variability anyway.

@HansOlsson @christoff-buerger Can you please confirm that the Evaluate annotation for mu0 should be removed again. Thanks.

To me "Evaluate=true" should be removed as mu0 should not be evaluated during translation, since it depends on the external table (and evaluating that during translation requires quite specific solution and doesn't really help).

The "Evaluate=true" stems from the original redesign where mu0 was still defined by means of Modelica.Math.Vectors.interpolate and not a combi-table.

Since we decided since than that full table interpolation modes should be supported (not just linear), we also have to define mu0 via a combi-table now (because we need to support spline interpolation & co).

From a design point, mu0 is a constant. It is pity that we cannot use "Evaluate=true" with it because combi-tables as external objects are challenging for Modelica tools. To that end, I see your concern as a tool shortcoming/challenge.

It is an important point however -- that tables are not suited for "Evaluate=true" -- such that this annotation should be dropped.

[christoff-buerger]

Looks good. We have all relevant classes redesigned to use tables instead of Modelica.Math.Vectors.interpolate and support all kind of table smoothness/interpolation modes.

There are no class / functionality duplications like Clutch2 etc.

We also have no Evaluate=true on constants derived via table interpolation, since such is troublesome for external table objects (a common tooling shortcoming).

[christoff-buerger]

It can be merged from my perspective.