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

Fix unit errors in ExternalCombiTimeTable constructor #4188

Merged

Conversation

henrikt-ma
Copy link
Contributor

The errors are in relation to how the constructor is called in Modelica.Blocks.Sources.CombiTimeTable:

startTime/timeScale,
columns,
smoothness,
extrapolation,
shiftTime/timeScale,

The errors are in relation to how the constructor is called in Modelica.Blocks.Sources.CombiTimeTable.
@henrikt-ma henrikt-ma added the L: Blocks Issue addresses Modelica.Blocks label Sep 1, 2023
Modelica/Blocks/Types.mo Outdated Show resolved Hide resolved
Modelica/Blocks/Types.mo Outdated Show resolved Hide resolved
@beutlich beutlich self-requested a review September 4, 2023 14:53
Copy link
Member

@beutlich beutlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, in #1473 (comment) I stated that the code is correct w.r.t. quantities and units.

@henrikt-ma
Copy link
Contributor Author

Hm, in #1473 (comment) I stated that the code is correct w.r.t. quantities and units.

I'm sure you had good intentions, even though a small mistake may have slipped by.

@beutlich
Copy link
Member

beutlich commented Sep 5, 2023

Changing the name of one input argument of the external object ctor is a backwards-compatibility breaking change. I guess a conversion script will be required in that case.

@henrikt-ma
Copy link
Contributor Author

Changing the name of one input argument of the external object ctor is a backwards-compatibility breaking change. I guess a conversion script will be required in that case.

Right, but right now I don't think it looks like the argument names should be changed, since it apparently isn't the intention that those arguments must be "scaled time".

Copy link
Contributor

@HansOlsson HansOlsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@beutlich beutlich merged commit 4898815 into modelica:master Sep 7, 2023
2 checks passed
@beutlich beutlich self-assigned this Sep 7, 2023
@beutlich beutlich added this to the MSL4.1.0 milestone Sep 7, 2023
@henrikt-ma henrikt-ma deleted the bugfix/externalcombitimetable-unit-error branch September 11, 2023 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
L: Blocks Issue addresses Modelica.Blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants