-
Notifications
You must be signed in to change notification settings - Fork 162
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
Multizone AHU controller: Removed measured input, fixed singular issues #3531
Conversation
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.
We are getting very close!
I used this script to test all valid combinations of the controller parameters (this is based on the developments from #3528).
https://github.com/AntoineGautier/modelica-buildings/blob/tmp/issue3526_connectors/Buildings/Resources/Scripts/travis/templates/VAVMultiZoneController.py
It turns out that only 11 % of the tested combinations fail to simulate (with a high probability of a recurring pattern that could explain most of them). I am sending the log file to you by email so that you can inspect and debug the faulty ones.
In addition, I have the following comments.
Inside Buildings.Controls.OBC.ASHRAE.G36.AHUs.MultiZone.VAV.SetPoints.PlantRequests
the input uCooCoiSet
is not conditional. The block is still incompatible with the following options.
- No cooling coil
- DX coil
Could we introduce a parameter to specify the type of cooling coil, and adapt the block in a similar manner as with have_hotWatCoi
?
Also, since we want to cover 3 options for the heating coil (none, HW, electric) and for the cooling coil (none, CHW, DX) I would introduce enumerations to describe these options, rather than increase the number of Boolean parameters, such as have_hotWatCoi
and have_eleHeaCoi
. Note also that the implementation currently does not avoid have_hotWatCoi=true
and have_eleHeaCoi=true
, which an enumeration would avoid.
@AntoineGautier I added the new enumeration The new enumeration has been applied in MultiZone and single zone VAV controller, and also the terminal unit controllers. Due to the change, I believe the template models also need to update. |
@JayHuLBL Noted. I'll refactor the templates via a commit to the branch |
parameter Buildings.Controls.OBC.ASHRAE.G36.Types.Coil cooCoi=Buildings.Controls.OBC.ASHRAE.G36.Types.Coil.WaterBasedCooling | ||
"Cooling coil type" | ||
annotation (__cdl(ValueInReference=false), | ||
Dialog(group="System and building parameters")); | ||
parameter Boolean have_eleHeaCoi=false | ||
"True: the AHU has electric heating coil" | ||
parameter Buildings.Controls.OBC.ASHRAE.G36.Types.Coil heaCoi=Buildings.Controls.OBC.ASHRAE.G36.Types.Coil.WaterBasedHeating | ||
"Heating coil type" | ||
annotation (__cdl(ValueInReference=false), | ||
Dialog(group="System and building parameters")); |
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.
@JayHuLBL I think 2 distinct enumerations should be introduced: one for heating coils, another one for cooling coils.
The current implementation does not prevent from selecting cooCoi=Buildings.Controls.OBC.ASHRAE.G36.Types.Coil.ElectricHeating
for example (the simulation of Buildings.Controls.OBC.ASHRAE.G36.AHUs.MultiZone.VAV.Validation.Controller
actually succeeds with this class modification).
Let me know if this is possible. (I'll wait before refactoring the VAV template.)
@AntoineGautier I was thinking about having two distinct enumeration, but realized that in |
The use of the enumeration differs in the Templates package. The enumeration itself is not exposed to the user, but only used "in the background", as a means of type introspection, for example:
At the top-level, we use replaceable components with an explicit choices annotation, for example:
Having enumerations that differ between the packages Controls and Templates is not an issue. (Of course, if we can use the same, this is better, but this is not a requirement.) |
@AntoineGautier I just changed the one enumeration to two enumerations for heating and cooling coil. |
I updated the templates in my last commit 052bdb3. However, the model
The model |
@AntoineGautier The compiling error of the |
That makes sense. I updated the VAV box template with the parameter |
…ssue3526_connectors
@JayHuLBL This is ready to merge I believe. |
@mwetter Please let us know if it is ready to merge. |
@mwetter The tests have passed and it is ready for your review. |
This closes #3526.