-
Notifications
You must be signed in to change notification settings - Fork 169
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
Comments
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).
I see several options:
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. |
Are you sure? With the default setup the behaviour of the block should not have changed. |
I think a (maybe otherwise empty) conversion script with detailed info would be better than just mentioning the change in the release notes. Something like this would make sure that anyone who is using
|
It's possible. |
Nice presentation of the difference - I agree that the greaterEqual-block does the same in both models. |
Hm, not sure. I was only looking at the types: In v4.0.0
I'd like to follow @tobolar's comment to unprotect |
Good point, I did not note that the type has changed. I guess it should be |
@m-kessler if |
Sure, that's why I suggested |
So I would prefer to define |
@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 On the other hand, I understand that
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 |
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? |
@casella: Indeed, the ticket is a bit confusing, as I started to mix in the 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:
The change could be communicated to all users which are actually using |
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:
This creates
conversion_report.html
in the Dymola working directory with following content:Inconsistent conversion script auto, for version 4.0.0.
The component
greaterEqual
in the reported class has been renamed togreaterEqual_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
inMechanics.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.The text was updated successfully, but these errors were encountered: