Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
AntoineGautier
left a comment
There was a problem hiding this comment.
This feedback is for commit 3f8c79f.
It seems that the remarks from #2299 (comment) have not been addressed.
This is problematic as several bugs were reported.
In addition, the main controller is still incompatible with a large subset of all possible plant configurations. But there is no assert statements in the code to check that the configuration settings are actually supported. So the user can only refer to the documentation of the main controller. This documentation enumerates all subcontrollers and provides for each of them a table showing which configurations are covered. So one must browse through all those tables to deduce which configurations the main controller is actually compatible with.
Furthermore, it appears that some subcontrollers are only valid for primary-secondary configurations (such as Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChillerPlant.Staging.SetPoints.Subsequences.CapacityRequirement--although this is not explicit in the documentation--which uses the secondary CHW return temperature and references §5.2.4.8 from RP1711) while others are only valid for primary-only configurations (such as Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChillerPlant.Pumps.ChilledWater.Controller).
I also have several additional inline remarks, see below.
| parameter Real fanSpeMin(unit="1")=0.1 | ||
| "Minimum tower fan speed" | ||
| annotation (Dialog(tab="Cooling Towers", group="Fan speed")); | ||
|
|
||
| parameter Real fanSpeMax(unit="1")=1 | ||
| "Maximum tower fan speed" | ||
| annotation (Dialog(tab="Cooling Towers", group="Fan speed")); |
There was a problem hiding this comment.
- First parameter only required for water-cooled plants.
- Second parameter never required as 100% AO from the BAS shall be mapped to maximum speed.
| Buildings.Controls.OBC.CDL.Interfaces.RealInput TConWatSup( | ||
| final unit="K", | ||
| displayUnit="degC", | ||
| final quantity="ThermodynamicTemperature") if not closeCoupledPlant | ||
| "Condenser water supply temperature (condenser entering)" | ||
| annotation(Placement(transformation(extent={{-940,-680},{-900,-640}}), | ||
| iconTransformation(extent={{-140,-340},{-100,-300}}))); |
There was a problem hiding this comment.
Missing condition for type of CT fan speed control.
In pseudo code this should be if have_CWRTControl and not closeCoupledPlant or have_CWSTControl.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
For reference, I am tracking here another suspected bug as discussed on 2/3/23. [EDIT on 1/20/25] On commit f1e3de2, the issue seems to persist with the staging logic for headered pumps. The validation model [EDIT on 9/29/25] @JayHuLBL There still seems to be an issue with the primary CHW pump staging logic in commit 55ea8c5. The validation model
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
AntoineGautier
left a comment
There was a problem hiding this comment.
Can you fix the remaining comments?
I will then test the controller with the CHW plant template and validate the functional aspects.
| annotation (Placement(transformation(extent={{920,70},{960,110}}), | ||
| iconTransformation(extent={{100,-110},{140,-70}}))); | ||
|
|
||
| Buildings.Controls.OBC.CDL.Interfaces.BooleanOutput y1ChiWatIsoVal[nChi] |
There was a problem hiding this comment.
This has been changed from real (in 55ea8c5) to Boolean: why?
G36 4.10.1:
A modulating valve is recommended on primary-only variable flow systems to allow for slow changes in flow during chiller staging.
Ideally, the type of valve should be specified by a configuration parameter.
If only a single choice is supported, it should be real (because the controller only supports primary-only plants) and it should be stated in the controller documentation.
There was a problem hiding this comment.
I added the position setpoint output yChiWatIsoVal.
There was a problem hiding this comment.
But these are mutually exclusive, you can't have both like now:
Buildings.Controls.OBC.CDL.Interfaces.BooleanOutput y1ChiWatIsoVal[nChi]
"Chilled water isolation valve position commanded on"
annotation (Placement(transformation(extent={{920,-50},{960,-10}}),
iconTransformation(extent={{100,-170},{140,-130}})));
Buildings.Controls.OBC.CDL.Interfaces.RealOutput yChiWatIsoVal[nChi](
final min=fill(0, nChi),
final max=fill(1, nChi),
final unit=fill("1", nChi))
"Chilled water isolation valve position setpoint"
annotation (Placement(transformation(extent={{920,-80},{960,-40}}),
iconTransformation(extent={{100,-190},{140,-150}})));Either
- introduce a configuration parameter to select between the two, defaulting to "modulating" for variable primary-only plants as recommended in G36, or
- keep only modulating as this is the recommended option.
| Buildings.Controls.OBC.CDL.Interfaces.RealInput uChiWatIsoVal[nChi]( | ||
| final min=fill(0, nChi), | ||
| final max=fill(1, nChi), | ||
| final unit=fill("1", nChi)) | ||
| "Chilled water isolation valve position" | ||
| annotation(Placement(transformation(extent={{-940,-188},{-900,-148}}), | ||
| iconTransformation(extent={{-140,-100},{-100,-60}}))); |
There was a problem hiding this comment.
This input only makes sense for modulating CHW isolation valves.
But now the controller only supports 2-position valves (see comment on y1ChiWatIsoVal).
And even for modulating valves, this input should be optional per G36 4.10.1:
Retain the following points for optional analog valve position feedback.
Reiterating my comment on 55ea8c5:
There was a problem hiding this comment.
The input is now removed and replaced by the position setpoint.
There was a problem hiding this comment.
If you keep the choice between modulating and two-position valves (see previous comment), you must also have the two options as inputs: AI or DI
- mutually exclusive depending on a configuration parameter "two-position or modulating valve", and
- both optional depending on another configuration parameter such as "true for valves with position feedback or open/close end switch status (whichever applies)"
…water isolation valve position setpoint output
|
@AntoineGautier The new comments have been addressed in the c74154b. Would you please review it? |
|
@JayHuLBL Thanks for the updates. See my comments above. I have a little request: can you modify the following parameter declarations with an unknown size array and rather check the size with an assert statement at initialization? With the current syntax and when these parameters are propagated as in the template, Dymola painfully attempts to check the size at compile time and fails, yielding a translation error. parameter Integer staMat[:, nChi] // change to parameter Integer staMat[:, :]
parameter Integer conWatPumStaMat[nPlaSta, nConWatPum] // change to parameter Integer conWatPumStaMat[:, :]In addition, simulating the plant template with the latest changes (commit 4d3e9d9) yields
|
|
@AntoineGautier |
AntoineGautier
left a comment
There was a problem hiding this comment.
@JayHuLBL Please see my comments.
In addition, with the latest changes in the controller, the template validation model Buildings.Templates.Plants.Chillers.Validation.WaterCooled in e4f86df exhibits some staging issues.
- When the plant transitions from stage 1 to 2, it takes 6 h for the lag chiller to be enabled.
- When the plant transitions back from stage 2 to 1, it takes 2 h for the lag chiller to be disabled.
There was a problem hiding this comment.
Both OCT and OMC fail to compile this model: has it been investigated?
Error in flattened model:
A when-guard is involved in an algebraic loop, consider breaking it using pre() expressions. Equations in block:
temp_00000024 = if chiPlaCon.staSetCon.cha.reaToInt.u - 0.5 <= pre(temp_00000024) - 1 or chiPlaCon.staSetCon.cha.reaToInt.u - 0.5 > pre(temp_00000024) or initial() then ceil(chiPlaCon.staSetCon.cha.reaToInt.u - 0.5) else pre(temp_00000024)
temp_00000025 = if temp_00000024 < pre(temp_00000025) or temp_00000024 >= pre(temp_00000025) + 1 or initial() then integer(temp_00000024) else pre(temp_00000025)
temp_00000026 = if chiPlaCon.staSetCon.cha.reaToInt.u + 0.5 < pre(temp_00000026) or chiPlaCon.staSetCon.cha.reaToInt.u + 0.5 >= pre(temp_00000026) + 1 or initial() then floor(chiPlaCon.staSetCon.cha.reaToInt.u + 0.5) else pre(temp_00000026)
temp_00000027 = if temp_00000026 < pre(temp_00000027) or temp_00000026 >= pre(temp_00000027) + 1 or initial() then integer(temp_00000026) else pre(temp_00000027)
chiPlaCon.staSetCon.cha.and4.u2 = not chiPlaCon.staSetCon.yDow
chiPlaCon.staSetCon.cha.and4.y = chiPlaCon.staSetCon.yUp and chiPlaCon.staSetCon.cha.and4.u2
chiPlaCon.staSetCon.cha.switch1.y = smooth(0, if chiPlaCon.staSetCon.cha.and4.y then chiPlaCon.staSetCon.cha.intToRea1.y else chiPlaCon.staSetCon.cha.intToRea.y)
chiPlaCon.staSetCon.cha.staChaHol2.not_u = not chiPlaCon.staSetCon.cha.or1.y
temp_00000135 = chiPlaCon.staSetCon.cha.staChaHol2.not_u and not pre(chiPlaCon.staSetCon.cha.staChaHol2.not_u)
temp_00000134 = chiPlaCon.staSetCon.cha.or1.y and not pre(chiPlaCon.staSetCon.cha.or1.y)
chiPlaCon.staSetCon.cha.staChaHol1.not_u = not chiPlaCon.staSetCon.cha.and1.y
The staging process takes long time is due to the check of the chilled water flow reaching the new setpoint as below: In the subsequence By adjusting the parameter |
Then there is an issue with the logic to check that "the new setpoint is achieved". parameter Real relFloDif=0.01
"Relative error to the setpoint for checking if it has achieved flow rate setpoint"
annotation (Dialog(tab="Advanced"));
Buildings.Controls.OBC.CDL.Reals.GreaterThreshold greThr(
final t=0.5*relFloDif,
final h=0.5*relFloDif)
"Check if chiller water flow rate achieves the minimum flow setpoint"
So the check is true if
Other minor issues found in that block:
|
|
@AntoineGautier Thanks! |
|
@JayHuLBL Great! That solves the chiller staging issue, although I would have implemented that simply by checking In 8296435, I've tested the controller for a plant with a WSE and modulating CHW bypass valve, using the model
|
|
Yes, exactly. This is the most generic and precise way.
(Thanks for the clarification about the absolute hysteresis when using |
|
@JayHuLBL
[EDIT] @JayHuLBL Looking in greater detail at the staging events, there seems to be some remaining "glitches" to fix (this is from
|













Closes #2293, #2086, #1977.
@JayHuLBL