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

MSL v4.1.0-beta.1 feedback: Missing conversion and convertMessage would help #4330

Open
m-kessler opened this issue Feb 22, 2024 · 13 comments
Open
Labels
discussion Discussion issue that it not necessarily related to a concrete bug or feature L: Electrical.PowerConverters Issue addresses Modelica.Electrical.PowerConverters L: Mechanics.Rotational Issue addresses Modelica.Mechanics.Rotational L: Mechanics.Translational Issue addresses Modelica.Mechanics.Translational P: high High priority issue V: 4.1.0-dev Issue originates in MSL v4.1.0-dev (and is not present in earlier releases)
Milestone

Comments

@m-kessler
Copy link
Collaborator

Dymola has a conversion script checker integrated, which reports that there is one missing conversion when coming from 4.0.0.

Run the command below with MSL 4.1.0 beta 1 loaded in Dymola 2024x:

DymolaCommands.SimulatorAPI.checkConversion(
  library="Modelica",
  from="4.0.0",
  oldVersions={"C:\Program Files\Dymola 2024x\Modelica\Library\Modelica 4.0.0\package.mo"})

This creates conversion_report.html in the Dymola working directory with following content:


Inconsistent conversion script auto, for version 4.0.0.
MessageClassComponent
Old component is missing, without conversionModelica.Electrical.PowerConverters.DCDC.Control.SignalPWMgreaterEqual

The component greaterEqual in the reported class has been renamed to greaterEqual_p.
I am not sure if I would create a conversion script just for this single component. I simple workaround would be to rename the component back.

If you decide to create a conversion script, I would also include messages for the now protected mu0 in Mechanics.Rotational.Components.Brake and others . One of the models in the ElectrifiedPowertrains library currently uses this variable and a conversions message would have helped to quickly identify the problem.

@beutlich beutlich added bug Critical/severe issue L: Electrical.PowerConverters Issue addresses Modelica.Electrical.PowerConverters L: Mechanics.Rotational Issue addresses Modelica.Mechanics.Rotational L: Mechanics.Translational Issue addresses Modelica.Mechanics.Translational P: high High priority issue V: 4.1.0-dev Issue originates in MSL v4.1.0-dev (and is not present in earlier releases) labels Feb 22, 2024
@beutlich
Copy link
Member

Thanks for the in-depth testing and reporting. Originally we did not aim for a conversion script for a minor release such as v4.1.0 (because following semantic versioning as promised by v4.0.0 we must not break things in minor versions).

  1. About Modelica.Electrical.PowerConverters.DCDC.Control.SignalPWM: This was changed in PowerConverters.DCDC.HBridge #3897 (where backwards-compatibilty was claimed in PowerConverters.DCDC.HBridge #3897 (comment)). I believe it's not simply done with renaming this component back.

  2. About Mechanics.Rotational.Components.Brake (and other mechanical components): We have been aware of the compatibility breakage (and thus the broken promise on semantic versioning) of Enhance frictional models based on ExternalCombiTable1D #3662 and Enhance frictional translational models based on ExternalCombiTable1D #3690 as we at least documented it already in the release notes in

    <p><br>
    The following <font color=\"blue\"><strong>existing components</strong></font> have been <font color=\"blue\"><strong>changed</strong></font> in a <font color=\"blue\"><strong>non-backward compatible</strong></font> way:
    </p>
    <table border=\"1\" cellspacing=\"0\" cellpadding=\"2\" style=\"border-collapse:collapse;\">
    <tr><td colspan=\"2\"><strong>Mechanics.MultiBody</strong></td></tr>
    <tr><td>World</td>
    <td>The protected parameters <code>ndim</code>, <code>ndim2</code> and
    <code>ndim_pointGravity</code> have been removed.</td></tr>
    <tr><td colspan=\"2\"><strong>Mechanics.Rotational.Components</strong></td></tr>
    <tr><td>Brake<br>Clutch<br>OneWayClutch</td>
    <td>The table interpolation in <code>mu_pos</code> utilizes the interpolation based on <a href=\"modelica://Modelica.Blocks.Types.ExternalCombiTable1D\">ExternalCombiTable1D</a>.<br>The public variable <code>mu0</code> was changed to a protected final parameter.</td></tr>
    <tr><td colspan=\"2\"><strong>Mechanics.Translational.Components</strong></td></tr>
    <tr><td>Brake</td>
    <td>The table interpolation in <code>mu_pos</code> utilizes the interpolation based on <a href=\"modelica://Modelica.Blocks.Types.ExternalCombiTable1D\">ExternalCombiTable1D</a>.<br>The public variable <code>mu0</code> was changed to a protected final parameter.</td></tr>
    </table>

I see several options:

  1. Revert the breaking changes for v4.1.0.
  2. Revert the breaking changes and also introduce the new models under new names, SignalPWM2 or Brake2.
  3. Keep the breaking changes, mitigate them as much as possible, document them (where missing, i.e. SignalPWM) and also document the violation of previously given semantic versioning promise.
  4. Keep the breaking changes, add a conversion script and still name it v4.1.0.
  5. Keep the breaking changes, add a conversion script and also add the old models to ObsoleteModelicaX and add a test suite similar to ModelicaTestConversion4. Name it v5.0.0.

Note, that none of the options comes for free. Option 3 seems to be the most pragmatic one, but still might be a wrong decision if broken compatibility is accepted in a minor release. I am aware that semantic versioning might be controversial (especially in the Modelica eco system), but for sure, we must not claim compatibility and break it deliberately.

@beutlich beutlich added this to the MSL4.1.0 milestone Feb 22, 2024
@beutlich beutlich added discussion Discussion issue that it not necessarily related to a concrete bug or feature and removed bug Critical/severe issue labels Feb 22, 2024
@m-kessler
Copy link
Collaborator Author

m-kessler commented Feb 23, 2024

#3897 (comment)). I believe it's not simply done with renaming this component back.

Are you sure? With the default setup the behaviour of the block should not have changed.
The new parameters refType and commonComparison have default values which make SignalPWM backward compatible.

Click for graphical comparison of SignalPWM

SignalPWM

@m-kessler
Copy link
Collaborator Author

About Mechanics.Rotational.Components.Brake (and other mechanical components): We have been aware of the compatibility breakage ... as we at least documented it already in the release notes in

I think a (maybe otherwise empty) conversion script with detailed info would be better than just mentioning the change in the release notes.
When I read the release notes, I did not remember that one of the thousands of classes we have in our libraries is using mu0.

Something like this would make sure that anyone who is using mu0 is informed:

convertMessage(
  "Mechanics.Rotational.Components.Brake",
  "Variable is now a protected parameter and should not be accessed any more form outside.",
  "mu0")

@HansOlsson
Copy link
Contributor

About Mechanics.Rotational.Components.Brake (and other mechanical components): We have been aware of the compatibility breakage ... as we at least documented it already in the release notes in

I think a (maybe otherwise empty) conversion script with detailed info would be better than just mentioning the change in the release notes. When I read the release notes, I did not remember that one of the thousands of classes we have in our libraries is using mu0.

It's possible.
However, since it still exists (even if protected) one can argue that it is not that problematic; as one will detect it afterwards.
If it had been deleted or renamed the need would have been clearer.

@HansOlsson
Copy link
Contributor

#3897 (comment)). I believe it's not simply done with renaming this component back.

Are you sure? With the default setup the behaviour of the block should not have changed.

Nice presentation of the difference - I agree that the greaterEqual-block does the same in both models.

@beutlich
Copy link
Member

beutlich commented Feb 23, 2024

Are you sure?

Hm, not sure. I was only looking at the types: In v4.0.0 greaterEqual is of type Modelica.Blocks.Logical.Less whereas in v4.1.0-beta.1 greaterEqual_p is of type Modelica.Blocks.Logical.Greater (and not Modelica.Blocks.Logical.GreaterEqual as the name would suggest, which seems odd anyway). Yes, I noticed that signal inputs are switched between zeroOrderHold and sawtooth. Even if the name might be identical, the type still differs, which can also be considered breaking compatibility.

When I read the release notes, I did not remember that one of the thousands of classes we have in our libraries is using mu0.

I'd like to follow @tobolar's comment to unprotect mu0 again for a minor release: #3662 (comment)

@m-kessler
Copy link
Collaborator Author

I was only looking at the types: In v4.0.0 greaterEqual is of type Modelica.Blocks.Logical.Less whereas in v4.1.0-beta.1 greaterEqual_p is of type Modelica.Blocks.Logical.Greater (and not Modelica.Blocks.Logical.GreaterEqual as the name would suggest, which seems odd anyway).

Good point, I did not note that the type has changed. I guess it should be GreaterEqual then.

@tobolar
Copy link
Contributor

tobolar commented Feb 26, 2024

One of the models in the ElectrifiedPowertrains library currently uses this variable and a conversions message would have helped to quickly identify the problem.

@m-kessler if mu0 is really used, IMO a conversion script would not really help as there is no public variable comparable to mu0.

@m-kessler
Copy link
Collaborator Author

Sure, that's why I suggested convertMessage.

@tobolar
Copy link
Contributor

tobolar commented Feb 26, 2024

So I would prefer to define mu0 public again.
A convert message can be added in future when upgrading MSL and defining mu0 protected again.

@casella
Copy link
Contributor

casella commented Jun 18, 2024

@m-kessler I am a bit confused by this ticket. Originally, it claimed that there is a non-backwards compatible component name change in Modelica.Electrical.PowerConverters.DCDC.Control.SignalPWM. Later comments by @beutlich also mention Mechanics.Rotational.Components.Brake (and other mechanical components)(which ones, exactly?) for which there was a non-backwards compatible change which has nothing to do with conversion and convertMessage, and was already dealt with in the release notes.

Regarding the original issue, to me this is really not a big deal. As I understand, @AHaumer and @christiankral made the converter model more general by having two blocks greaterEqual_p and greaterEqual_n that can be driven independently, instead of relying on just one block greaterEqual. So, the point is not that a component was renamed, but rather that the whole implementation was changed, to make it more general. So, I guess just renaming that component back won't solve the problem, it could actually make it even more confusing.

On the other hand, I understand that

  • the default behaviour of the component and its interface (parameters, connectors) is the same, hence backwards compatible
  • the greaterEqual blocks (before and after the change) have no parameters, so there's no potential problems with nested parameter modifiers in models that use this component
  • it is true that those models are not protected, so technically speaking the public interface is no longer the same; however, the change is only relevant if one wants to directly access some internal variables of those blocks from the outside with dot notation - this is of course possible but highly unlikely, except for debugging purposes. This occurrence can for sure be handled manually.

To me this is the typical situation that can be handled by a line in the release notes, so I'm definitely in favour of option 3. in @beutlich's comment. Introducing conversion scripts for such a marginal change seems to me completely unnecessary overkill. MSL 4.1.0 should have no conversion scripts, to me this is a strong requirement. This is a big nuisance in terms of project management, considering that everybody uses the MSL, so it should be used as sparingly as possible. Preferably not more than once every 10 years.

Regarding the mu0 issue, I understand it was already deemed a minor issue only worth of a line in the release notes. @tobolar if you think this is not an optimal solution, please open a separate ticket and explain how you would like this issue to be handled, and exactly on which components of the library. Thanks!

@casella
Copy link
Contributor

casella commented Jun 18, 2024

Although I understand that for this kind of PWM generation models using Greater or GreaterEqual would give exactly the same results (the ZC functions change sign exactly at the same point in time), I also find it slighly odd that the two blocks are named greaterEqual_p and greaterEqual_n, but then they are of type Greater. No big deal, but a bit odd.

I would then suggest either to change their type, or to change their name, to avoid confusion. @AHaumer was there any reason in favour of Greater vs. GreaterEqual for the new implementation?

@m-kessler
Copy link
Collaborator Author

@casella: Indeed, the ticket is a bit confusing, as I started to mix in the Brake / mu0 issue in the original post. Sorry for that.

Nevertheless, the whole issue is not a big deal and I did not expect to create such a big discussion. Just go for solution 3, its a reasonable choice.

Regarding:

Later comments by beutlich also mention Mechanics.Rotational.Components.Brake ... for which there was a non-backwards compatible change which has nothing to do with conversion and convertMessage, and was already dealt with in the release notes.

The change could be communicated to all users which are actually using m0 with convertMessage (additional to mentioning it in the release notes), so its not completely unrelated to conversion. But still, no big deal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussion issue that it not necessarily related to a concrete bug or feature L: Electrical.PowerConverters Issue addresses Modelica.Electrical.PowerConverters L: Mechanics.Rotational Issue addresses Modelica.Mechanics.Rotational L: Mechanics.Translational Issue addresses Modelica.Mechanics.Translational P: high High priority issue V: 4.1.0-dev Issue originates in MSL v4.1.0-dev (and is not present in earlier releases)
Projects
None yet
Development

No branches or pull requests

5 participants