-
Notifications
You must be signed in to change notification settings - Fork 161
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
Issue2885 fan coil unit controls #3627
base: master
Are you sure you want to change the base?
Conversation
Merge lbl master into pnnl master
…uildings:issue1913_g36_final
Issue2885 fan coil unit controls
…sts. Updated parameter group annotations. Simplified logic with hysteresis blocks
…ontrols Issue2885 fan coil unit controls
@mwetter It's ready for your review after the CI tests passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See inline comments. The main part to address is that there is no closed loop test for the new controller. We do want closed loop tests as experience shows that they are essential to check for mistakes in the control logic or unsuitable control I/O.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert this formatting only change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert this formatting only change which I think is unintentional.
or heaCoi==Buildings.Controls.OBC.ASHRAE.G36.Types.HeatingCoil.Electric)); | ||
|
||
parameter Real cooDea( | ||
unit="1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Units should be made final
as the model is only valid for these units.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This controller is not used in any closed loop test. We need to have a closed loop test before merging. Experience shows that often bugs or unsuitable I/O are discovered when a controller is connected to an equipment model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simulating simulateModel("Buildings.Controls.OBC.ASHRAE.G36.AHUs.MultiZone.VAV.Validation.Controller_UnspecifiedClimate", method="Cvode", stopTime=3600, tolerance=1e-06, resultFile="MultizoneController");
gives warnings about unspecified climate that need to be corrected.
"Time period for which hotWatResReqLim3 has to be exceeded before three hot water reset requests are sent" | ||
annotation(Dialog(tab="Request limits", group="Hot water reset requests", enable=have_hotWatCoi)); | ||
|
||
parameter Real Thys( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be THys
(Note that Thys
is used in many models).
This example validates | ||
<a href=\"modelica://Buildings.Controls.OBC.ASHRAE.G36.FanCoilUnit.Controller\"> | ||
Buildings.Controls.OBC.ASHRAE.G36.FanCoilUnit.Controller</a>. | ||
<br> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
</p>
<p>
to separate paragraphs rather than <br>
@karthikeyad-pnnl Would you please address Michael's inline comments? |
@karthikeyad-pnnl Let me know if you need help on addressing the comments. |
@JayHuLBL Thanks for checking in! We are currently working on the closed loop validation, so we can temporarily deprioritize this |
@karthikeyad-pnnl Is there any update on this PR? |
This closes #2885.