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.

Sign up for GitHub

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

Jump to bottom

Add units to real I/O signals #3267

Merged
merged 3 commits into from
Jan 2, 2020
Merged

Add units to real I/O signals #3267

merged 3 commits into from
Jan 2, 2020

Conversation

beutlich
Copy link
Member

@beutlich beutlich commented Dec 15, 2019 •
edited

Though decided in #2492 to utilize final unit="X", I discared the final. For me, the unit of real I/O signals currently seems arbitrary and uncontrolled in MSL. Some use, unit="X", some final unit="X", some with displayUnit and some even with quantity. There could be anther issue to harmonize these kind of unit modifications.

@christiankral @HansOlsson @AHaumer Please check carefully when reviewing. Thanks.

Closes #2488, closes #2492.

@beutlich beutlich added the enhancement New feature or enhancement label Dec 15, 2019
@beutlich beutlich added this to the MSL4.0.0 milestone Dec 15, 2019
@beutlich beutlich self-assigned this Dec 15, 2019
@beutlich beutlich added the task General work that is not related to a bug or feature label Dec 16, 2019
HansOlsson
HansOlsson approved these changes Dec 17, 2019
View reviewed changes
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. There were some descriptions remaining with "... (rad)" (for variable having unit="rad") - but I think we can keep them for now.

@beutlich
Copy link
Member Author

Looks good. There were some descriptions remaining with "... (rad)" (for variable having unit="rad") - but I think we can keep them for now.

Yes, I also noticed when adding the unit attribute and decided to keep as is. Maybe @christiankral wants to have them removed.

@christiankral
Change wording of w
7a6b286 
I suppose the intention is that w represents the angular frequency w = 2 * pi * f
christiankral
christiankral previously requested changes Dec 22, 2019
View reviewed changes
Copy link
Contributor

@christiankral christiankral left a comment

Choose a reason for hiding this comment

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

@AHaumer Please double check 7a6b286 before merging

Other than that I am fine with the changes. I am just requesting a change to not accidentally merge without having @AHaumer's review

@christiankral
Copy link
Contributor

Looks good. There were some descriptions remaining with "... (rad)" (for variable having unit="rad") - but I think we can keep them for now.

Yes, I also noticed when adding the unit attribute and decided to keep as is. Maybe @christiankral wants to have them removed.

Yes, I removed the (rad) from the doc string in 2070494

@christiankral
Copy link
Contributor

@beutlich Thanks for applying the changes

@beutlich beutlich removed the request for review from christiankral January 2, 2020 18:21
@beutlich beutlich merged commit 83ef895 into modelica:master Jan 2, 2020
@beutlich beutlich deleted the add-units-to-signals branch January 2, 2020 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AHaumer AHaumer AHaumer approved these changes

@HansOlsson HansOlsson HansOlsson approved these changes

@christiankral christiankral christiankral left review comments

Assignees

@beutlich beutlich

Labels
enhancement New feature or enhancement task General work that is not related to a bug or feature
Projects
None yet
Milestone
MSL4.0.0
4 participants
@beutlich @christiankral @AHaumer @HansOlsson