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

Merge OBC package from Buildings to IBPSA #1946

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Conversation

mwetter
Copy link
Contributor

@mwetter mwetter commented Dec 14, 2024

This closes #1944

It also moves the round function in the math package as it is used in various Elementary Blocks, and it moves some Composite Control blocks that are likely of general interest.

@mwetter mwetter marked this pull request as ready for review December 16, 2024 15:24
@mwetter mwetter requested a review from FWuellhorst December 16, 2024 16:33
Copy link
Contributor

@FWuellhorst FWuellhorst left a comment

Choose a reason for hiding this comment

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

@mwetter I will look into the CDL package in the upcoming days, just noticed a minor naming issue outside of that package.

Comment on lines +44 to +45
Buildings.Controls.OBC.CDL.Reals.Round</a>.
</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Buidlings. is still used in some places

@FWuellhorst
Copy link
Contributor

Aside from that comment, the documentation and naming looks ok.
However, you maybe should note in the wiki on naming that you chose a different naming approach for CDL package, as this is heavily oriented at the MSL package or maybe the standard.
Else, you have a lot of complete words and control specific terms in there, not listed as possible exceptions from your naming rules.

Otherwise, I only have two requests:

  • Add a central UsersGuide on what this package is and why one should use it. You can refer to existing doc elsewhere if you want.
  • Maybe state in that UsersGuide or model doc what the difference to the MSL blocks are, and if changing them is as easy as replacing the MSL block, e.g. and with the one in CDL. Looking at the models, I sometimes do not get why you needed to duplicate a lot of stuff from MSL. Avoid inheritance is one reason, but why add e.g. RealInput again? If switching is easy, do you have a script to automatically switch, if possible? Also, are all blocks exactly the same in terms of functionality, e.g. IBPSA.Controls.OBC.CDL.Logical.Timer? I directly see you added the passed option, but don't get why you did not just compose a new block out of the standard timer and a greater threshold.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge Buildings.Controls.OBC.CDL
2 participants