VariableRefrigerantFlow Terminal Unit on air loop does not test node connections #8723

rraustad · 2021-04-20T18:14:24Z

Pull request overview

Fixes VRF TU on AirLoopHVAC supply branch will force loop off if terminal OA mixer not set and OA inputs not autosized #8718
When the ZoneHVAC:TerminalUnit:VariableRefrigerantFlow object is place in AirloopHVAC the node connections are not tested for incorrect node names. This can lead to incorrect operation or no operation at all.

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

Reviewer

This will not be exhaustively relevant to every PR.

rraustad · 2021-04-20T18:16:09Z

New warning for defect file.

** Severe  ** ZoneHVAC:TerminalUnit:VariableRefrigerantFlow "TU5INAIRLOOP"
**   ~~~   ** ... For draw thru fan when no OA mixer is specified and a cooling coil is present the terminal unit inlet node name must match the cooling coil inlet node name.
**   ~~~   ** ... Terminal unit inlet node name = AIRLOOP MIXED AIR OUTLET.
**   ~~~   ** ... Cooling coil inlet node name = TU5 VRF DX CCOIL INLET NODE.
**  Fatal  ** GetVRFInput: Errors found in getting AirConditioner:VariableRefrigerantFlow system input. Preceding condition(s) causes termination.

rraustad · 2021-04-20T18:19:48Z

tst/EnergyPlus/unit/HVACVariableRefrigerantFlow.unit.cc

@@ -422,7 +424,7 @@ class AirLoopFixture : public EnergyPlusFixture
        int VRFTUOutletNodeNum = 31;
        int VRFTUOAMixerOANodeNum = 32;
        int VRFTUOAMixerRelNodeNum = 33;
-        int VRFTUOAMixerRetNodeNum = 34;
+        int VRFTUOAMixerRetNodeNum = VRFTUInletNodeNum;


Existing unit test was not configured correctly. When testing new function the failure was due to the OA mixer return node not connected to the TU inlet node. Correcting this changed the setup needed for the unit test.

rraustad · 2021-04-20T18:20:08Z

tst/EnergyPlus/unit/HVACVariableRefrigerantFlow.unit.cc

@@ -545,7 +548,7 @@ TEST_F(AirLoopFixture, VRF_SysModel_inAirloop)
    state->dataScheduleMgr->Schedule(state->dataHVACVarRefFlow->VRF(curSysNum).SchedPtr).CurrentValue = 1.0; // enable the VRF condenser
    state->dataScheduleMgr->Schedule(thisTU.SchedPtr).CurrentValue = 1.0;                                    // enable the terminal unit
    state->dataScheduleMgr->Schedule(thisTU.FanAvailSchedPtr).CurrentValue = 1.0;                            // turn on fan
-    state->dataScheduleMgr->Schedule(thisTU.FanOpModeSchedPtr).CurrentValue = 0.0;                           // set cycling fan operating mode
+    state->dataScheduleMgr->Schedule(thisTU.FanOpModeSchedPtr).CurrentValue = 1.0;                           // set constant fan operating mode


Changed to constant fan for set point control.

rraustad · 2021-04-20T18:20:33Z

tst/EnergyPlus/unit/HVACVariableRefrigerantFlow.unit.cc

@@ -583,7 +587,8 @@ TEST_F(AirLoopFixture, VRF_SysModel_inAirloop)
    state->dataLoopNodes->Node(thisTU.VRFTUOutletNodeNum).TempSetPoint = 20.0; // select 20 C as TU outlet set point temperature

    InitVRF(*state, curTUNum, curZoneNum, FirstHVACIteration, OnOffAirFlowRatio, QZnReq); // Initialize all VRFTU related parameters
-
+    thisTU.NoCoolHeatOutAirMassFlow = 0.0;
+    // the above needs correcting for air loop equipment. OA flow should be set to 0 if autosized as it is here.


This will require a follow up issue.

rraustad · 2021-04-20T18:21:42Z

tst/EnergyPlus/unit/HVACVariableRefrigerantFlow.unit.cc

+    thisTU.heatCoilAirInNode += 1;  // revert previous change
+    thisTU.heatCoilAirOutNode -= 1; // change index of comp node (watch for array bounds)
+    CheckVRFTUNodeConnections(*state, curTUNum, ErrorsFound);
+    EXPECT_TRUE(ErrorsFound); // nodes are not connected correctly


New tests to exercise node connection function. This actually found a few missed cases in function.

rraustad · 2021-04-20T18:22:12Z

tst/EnergyPlus/unit/HVACVariableRefrigerantFlow.unit.cc

+    thisTU.heatCoilAirInNode += 1;  // revert previous change
+    thisTU.heatCoilAirOutNode -= 1; // change index of comp node (watch for array bounds)
+    CheckVRFTUNodeConnections(*state, VRFTUNum, ErrorsFound);
+    EXPECT_TRUE(ErrorsFound); // nodes are not connected correctly


Same functions tests for a TU as zone equipment.

…8-VRF-TU-on-AirLoopHVAC-Node-Testing

mjwitte

@rraustad I've tested this with the defect file (and with a corrected defect file) and everything works as it should here now. Unit tests all run fine. A few comments below.

src/EnergyPlus/HVACVariableRefrigerantFlow.cc

mjwitte · 2021-04-29T15:42:36Z

src/EnergyPlus/HVACVariableRefrigerantFlow.cc

+void CheckVRFTUNodeConnections(EnergyPlusData &state, int const VRFTUNum, bool &ErrorsFound)
+{
+
+    std::string const cTerminalUnitType("ZoneHVAC:TerminalUnit:VariableRefrigerantFlow");


Checking with @Myoldmopar about our current best practice for declaring string constants. This is nit-picking for a one-time function, but I'd like to make sure new code doesn't perpetuate old patterns.

@mjwitte I found where I had seen @lefticus document the best practice. The message is:

For any future update of const static std::string, I recommend a move to constexpr static std::string_view and updating all downstream code to avoid as many dynamic allocations as possible.

For a fuller discussion, check the main comment body of #8663. I now have a better feel for how to utilize string views; this is a really exciting change with lots of potential in our code.

So constexpr static std::string_view cTerminalUnitType("string"); should be used here?

src/EnergyPlus/HVACVariableRefrigerantFlow.cc

Myoldmopar · 2021-04-29T15:55:17Z

src/EnergyPlus/HVACVariableRefrigerantFlow.cc

+void CheckVRFTUNodeConnections(EnergyPlusData &state, int const VRFTUNum, bool &ErrorsFound)
+{
+
+    std::string const cTerminalUnitType("ZoneHVAC:TerminalUnit:VariableRefrigerantFlow");


@lefticus could you document in a GitHub comment here the best practice for local variable strings? We have discussed them on calls and maybe chats, but it would be good to lock it down here. IIRC it's going to be a constexpr string_view, but I can't recall all the implications.

@lefticus, use constexpr static std::string_view cTerminalUnitType("string"); here?

@Myoldmopar @rraustad I wrote up a full explanation of what is best and what the implications may or may not be on this other PR:

#8663

…8-VRF-TU-on-AirLoopHVAC-Node-Testing

rraustad · 2021-04-30T17:33:51Z

Defect warning with revised warning format.

** Severe  ** ZoneHVAC:TerminalUnit:VariableRefrigerantFlow="TU5INAIRLOOP",
**   ~~~   ** ... For draw thru or no fan when no OA mixer is specified and a cooling coil is present the terminal unit inlet node name must match the cooling coil inlet node name.
**   ~~~   ** ... Terminal unit inlet node name = AIRLOOP MIXED AIR OUTLET.
**   ~~~   ** ... Cooling coil inlet node name = TU5 VRF DX CCOIL INLET NODE.
**  Fatal  ** GetVRFInput: Errors found in getting AirConditioner:VariableRefrigerantFlow system input. Preceding condition(s) causes termination.

rraustad · 2021-04-30T18:05:59Z

@Myoldmopar @mjwitte this should be ready now.

mjwitte

@rraustad Looks good. I'll let CI make more green and the merge this.
And thanks for cleaning up the stray unit test formatting.

rraustad added 3 commits April 19, 2021 17:27

Revise VRF node testing logic

4812a28

More node connection logic and unit test

af8706c

More unit testing

c818338

rraustad added the Defect Includes code to repair a defect in EnergyPlus label Apr 20, 2021

rraustad self-assigned this Apr 20, 2021

rraustad added this to the EnergyPlus Future milestone Apr 20, 2021

rraustad commented Apr 20, 2021

View reviewed changes

rraustad requested a review from mjwitte April 28, 2021 18:38

rraustad added 2 commits April 28, 2021 18:32

Fix non=zero TU OA flow

edc8ea5

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into 871…

e95b1e7

…8-VRF-TU-on-AirLoopHVAC-Node-Testing

mjwitte reviewed Apr 29, 2021

View reviewed changes

Myoldmopar reviewed Apr 29, 2021

View reviewed changes

rraustad added 3 commits April 30, 2021 09:54

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into 871…

b20364d

…8-VRF-TU-on-AirLoopHVAC-Node-Testing

Add additional node check when blow thru fan and OA mixer is used

8173707

Merge branch 'develop' of https://github.com/NREL/EnergyPlus into 871…

72f6b34

…8-VRF-TU-on-AirLoopHVAC-Node-Testing

rraustad added 2 commits April 30, 2021 13:35

Clang format unit test

9341138

Clang format unrelated unit test

18b9df0

mjwitte approved these changes Apr 30, 2021

View reviewed changes

mjwitte merged commit 2d2d7fa into develop May 1, 2021

mjwitte deleted the 8718-VRF-TU-on-AirLoopHVAC-Node-Testing branch May 1, 2021 01:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VariableRefrigerantFlow Terminal Unit on air loop does not test node connections #8723

VariableRefrigerantFlow Terminal Unit on air loop does not test node connections #8723

rraustad commented Apr 20, 2021 •

edited by mjwitte

Loading

rraustad commented Apr 20, 2021

rraustad Apr 20, 2021

rraustad Apr 20, 2021

rraustad Apr 20, 2021

rraustad Apr 20, 2021

rraustad Apr 20, 2021

mjwitte left a comment

mjwitte Apr 29, 2021

Myoldmopar Apr 30, 2021

rraustad Apr 30, 2021

Myoldmopar Apr 29, 2021

rraustad Apr 30, 2021

lefticus Apr 30, 2021

rraustad commented Apr 30, 2021

rraustad commented Apr 30, 2021

mjwitte left a comment

VariableRefrigerantFlow Terminal Unit on air loop does not test node connections #8723

VariableRefrigerantFlow Terminal Unit on air loop does not test node connections #8723

Conversation

rraustad commented Apr 20, 2021 • edited by mjwitte Loading

Pull request overview

Pull Request Author

Reviewer

rraustad commented Apr 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjwitte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rraustad commented Apr 30, 2021

rraustad commented Apr 30, 2021

mjwitte left a comment

Choose a reason for hiding this comment

rraustad commented Apr 20, 2021 •

edited by mjwitte

Loading