Quick Fan Cleanup #10462

amirroth · 2024-04-10T12:53:17Z

I'm not going to press my luck here, so I will get this merged before I try something more drastic. The changes are:

Use enum class FanType rather that constexpr int FanType_* everywhere.
Unify five (!!!!!) separate "fan placement" enums plus the constexpr int versions into a single enum class FanPlace and use everywhere. If you think I am joking, look at the diffs.
Update Fan API to take FanName only for GetFanIndex and then take FanIndex everywhere else with the exception of Simulate (TODO).
Propagate getEnumValue and error shortcut patterns to places that were changed to use FanType and FanPlace.

Everything is green.

amirroth

Walk through.

amirroth · 2024-04-10T12:56:09Z

src/EnergyPlus/AirLoopHVACDOAS.cc

-                        thisOutsideAirSys.InletNodeNum(CompNum) = Fans::GetFanInletNode(state, typeNameUC, CompName, InletNodeErrFlag);
-                        thisOutsideAirSys.OutletNodeNum(CompNum) = Fans::GetFanOutletNode(state, typeNameUC, CompName, OutletNodeErrFlag);
+                        thisOutsideAirSys.InletNodeNum(CompNum) = Fans::GetFanInletNode(state, thisDOAS.m_FanIndex);
+                        thisOutsideAirSys.OutletNodeNum(CompNum) = Fans::GetFanOutletNode(state, thisDOAS.m_FanIndex);


All of these functions now take a FanIndex as an argument rather than FanType and FanName strings. This is probably temporary. These functions can be deleted entirely in favor of direct access and this may happen soon. But at least the access idiom is proper now.

amirroth · 2024-04-10T12:56:32Z

src/EnergyPlus/AirflowNetwork/include/AirflowNetwork/Elements.hpp

@@ -1085,7 +1086,7 @@ namespace AirflowNetwork {
        // Members
        Real64 FlowRate;           // Air volume flow rate
        Real64 Ctrl;               // Control ratio
-        int FanTypeNum;            // Fan type: Constant volume or ONOFF
+        DataHVACGlobals::FanType fanType;            // Fan type: Constant volume or ONOFF


Elevate all of these to enum class FanType.

amirroth · 2024-04-10T12:56:59Z

src/EnergyPlus/AirflowNetwork/src/Elements.cpp

@@ -717,7 +714,7 @@ namespace AirflowNetwork {

        int AirLoopNum = state.afn->AirflowNetworkLinkageData(i).AirLoopNum;

-        if (FanTypeNum == FanType_SimpleOnOff) {
+        if (fanType == DataHVACGlobals::FanType::OnOff) {


These are all enum class FanType now.

amirroth · 2024-04-10T12:58:00Z

src/EnergyPlus/AirflowNetwork/src/Solver.cpp

                int inletNode;
                int outletNode;

-                if (Util::SameString(Util::makeUPPER(fan_type), "FAN:SYSTEMMODEL")) {
+                DataHVACGlobals::FanType fanType =


Use getEnumValue since this is an enumeration now.

amirroth · 2024-04-10T12:59:28Z

src/EnergyPlus/Coils/CoilCoolingDX.cc

@@ -834,7 +834,7 @@ void CoilCoolingDX::simulate(EnergyPlusData &state,
                    state.dataRptCoilSelection->coilSelectionReportObj->setCoilSupplyFanInfo(state,
                                                                                             this->name,
                                                                                             state.dataCoilCooingDX->coilCoolingDXObjectName,
-                                                                                             state.dataFans->Fan(this->supplyFanIndex).FanName,
+                                                                                             state.dataFans->Fan(this->supplyFanIndex).Name,


The name field of basically every other object is called Name.

amirroth · 2024-04-10T13:04:39Z

src/EnergyPlus/DesiccantDehumidifiers.cc

@@ -378,6 +382,13 @@ namespace DesiccantDehumidifiers {

            desicDehum.RegenCoilType = Alphas(8);
            desicDehum.RegenCoilName = Alphas(9);
+


getEnumValue and error shortcut patterns.

amirroth · 2024-04-10T13:05:56Z

src/EnergyPlus/EvaporativeCoolers.cc

                state.dataHVACFan->fanObjs.emplace_back(new HVACFan::FanSystem(state, thisZoneEvapUnit.FanName)); // call constructor
-                thisZoneEvapUnit.FanIndex = HVACFan::getFanObjectVectorIndex(state, thisZoneEvapUnit.FanName);
+                thisZoneEvapUnit.FanIndex = state.dataHVACFan->fanObjs.size() - 1;


No need to search for this object, since you know it's going to be at the end (you just put it there).

amirroth · 2024-04-10T13:06:15Z

src/EnergyPlus/EvaporativeCoolers.hh

@@ -73,14 +74,6 @@ namespace EvaporativeCoolers {
        Num
    };

-    enum class FanPlacement


amirroth · 2024-04-10T13:08:27Z

src/EnergyPlus/Fans.hh


-    void GetFanVolFlow(EnergyPlusData &state, int FanIndex, Real64 &FanVolFlow);
+    Real64 GetFanVolFlow(EnergyPlusData &state, int FanIndex);


Return values via the return value.

amirroth · 2024-04-10T13:08:43Z

src/EnergyPlus/Fans.hh

-                    std::string_view ThisObjectType = {}, // parent object type (for error message)
-                    std::string_view ThisObjectName = {}  // parent object name (for error message)
-    );
+    DataHVACGlobals::FanType GetFanType(EnergyPlusData &state, int FanIndex); 


Return values via the return value.

rraustad · 2024-04-10T13:21:24Z

src/EnergyPlus/FanCoilUnits.cc

-            if (errFlag) {
-                ShowContinueError(state, format("specified in {} = \"{}\".", CurrentModuleObject, fanCoil.Name));
+            fanCoil.fanType = static_cast<DataHVACGlobals::FanType>(getEnumValue(DataHVACGlobals::fanTypeNamesUC, Alphas(9)));
+            if (fanCoil.fanType == DataHVACGlobals::FanType::Invalid) {


Fan type is a choice field and cannot be invalid at this point.

Ok, can change this to an assert. I guess I should change all of them.

Why? The assert will never fail.

assert(fanCoil.fantype != DataHVACGlobals::FanType::Invalid);

rraustad · 2024-04-10T13:40:52Z

src/EnergyPlus/FanCoilUnits.cc

+                    ShowSevereItemNotFound(state, eoh, cAlphaFields(10), fanCoil.FanName);
+                    ErrorsFound = true;    
+                } else {
+                    assert(fanCoil.fanType == Fans::GetFanType(state, fanCoil.FanIndex));


If a user selects the wrong fan object type in a fan coil unit there would be node connection warnings in release but no specific warning from the fan coil.

FanCoilAutoSize.idf:

ZoneHVAC:FourPipeFanCoil, Fan:OnOff, !- Supply Air Fan Object Type Zone1FanCoilFan, !- Supply Air Fan Name Fan:ConstantVolume, Zone1FanCoilFan, !- Name ** Warning ** Node Connection Error for object FAN:CONSTANTVOLUME=ZONE1FANCOILFAN ** ~~~ ** Air Nodes not on any Branch or Parent Object ** ~~~ ** Inlet Node : ZONE1FANCOILOAMIXEROUTLETNODE ** ~~~ ** Outlet Node: ZONE1FANCOILFANOUTLETNODE ** Warning ** Node Connection Error for object FAN:ONOFF=ZONE1FANCOILFAN ** ~~~ ** UNDEFINED not on any Branch or Parent Object ** ~~~ ** Inlet Node : ZONE1FANCOILOAMIXEROUTLETNODE ** ~~~ ** Outlet Node: UNDEFINED

In debug this assert will fail. I would suggest changing this to a severe warning. Using either this or a more detailed warning (probably the latter).

if (fanCoil.fanType != Fans::GetFanType(state, fanCoil.FanIndex)) { ShowSevereItemNotFound(state, eoh, cAlphaFields(9), fanCoil.FanName); ShowSevereItemNotFound(state, eoh, cAlphaFields(9), Alphas(9)); ErrorsFound = true; } ** Severe ** GetFanCoilUnits: ZoneHVAC:FourPipeFanCoil = ZONE1FANCOIL ** ~~~ ** Supply Air Fan Name = ZONE1FANCOILFAN, item not found. ** Severe ** GetFanCoilUnits: ZoneHVAC:FourPipeFanCoil = ZONE1FANCOIL ** ~~~ ** Supply Air Fan Object Type = FAN:ONOFF, item not found. ** Fatal ** GetFanCoilUnits: Errors found in input. Preceding condition(s) cause termination.

rraustad · 2024-04-10T13:53:39Z

src/EnergyPlus/IntegratedHeatPump.hh

@@ -228,7 +228,7 @@ namespace IntegratedHeatPump {
              WHtankType(DataPlant::PlantEquipmentType::Invalid), WHtankID(0), LoopNum(0), LoopSideNum(0), IsWHCallAvail(false), CheckWHCall(false),
              CurMode(IHPOperationMode::Idle), ControlledZoneTemp(0), WaterFlowAccumVol(0), SHDWHRunTime(0), CoolVolFlowScale(0), HeatVolFlowScale(0),
              MaxHeatAirMassFlow(0), MaxHeatAirVolFlow(0), MaxCoolAirMassFlow(0), MaxCoolAirVolFlow(0), IHPCoilsSized(false), IDFanID(0),
-              IDFanPlace(0), ODAirInletNodeNum(0),                                                                    // oudoor coil inlet Nod
+              fanPlace(DataHVACGlobals::FanPlace::Invalid), ODAirInletNodeNum(0),                                                                    // oudoor coil inlet Nod


Unused variable, only change is in the header.

rraustad · 2024-04-10T14:53:59Z

src/EnergyPlus/WindowAC.cc

+            state.dataWindowAC->WindAC(WindACNum).fanType =
+                static_cast<DataHVACGlobals::FanType>(getEnumValue(DataHVACGlobals::fanTypeNamesUC, Alphas(7)));
+            if (state.dataWindowAC->WindAC(WindACNum).fanType == DataHVACGlobals::FanType::Invalid) {
+                ShowSevereInvalidKey(state, eoh, cAlphaFields(7), Alphas(7));


The way this new function (#10066) is used, after converting a choice field to an enum, I don't see how it can ever be called because the choice field should always return a valid enum and therefore not be Invalid. The IP would flag any invalid inputs.

I'm changing all of these to asserts. Thx.

The assert would never execute. It would be dead code.

You're right. Why can't I remember this? FWIW they don't even compile in release code. So they are really a form of documentation more than anything else, but I will delete them.

Actually I'm going to leave them in. They make me feel better and they don't affect performance.

constexpr int getEnumValue(const gsl::span<const std::string_view> sList, const std::string_view s, const bool b) { for (unsigned int i = 0; i < sList.size(); ++i) { if (sList[i] == s) return i; } if (b) assert(false); return -1; }

Don't even need the b parameter.

I agree, if getEnumValue is only used for choice fields.

amirroth · 2024-04-10T21:21:17Z

@Myoldmopar, this is ready to merge.

Myoldmopar · 2024-04-11T20:44:41Z

I'll get clang format settled. It also looks like CppCheck is complaining intensely about some uninitialized variables. And there's an enum related custom check. I'll get them fixed up and get this merged!

rraustad · 2024-04-12T01:39:24Z

Gotta give credit, it's not used now:

Detected 1 enums in ONE source file:
 - DataHVACGlobals.hh : 187 :: UnitarySysType (1 usages) in DataHVACGlobals.hh
Custom Check Failed!

amirroth · 2024-04-12T13:08:58Z

Gotta give credit, it's not used now:

Detected 1 enums in ONE source file:
 - DataHVACGlobals.hh : 187 :: UnitarySysType (1 usages) in DataHVACGlobals.hh
Custom Check Failed!

I plan to use it in the next PR or the one after (I decided to get this part merged before I tried something more aggressive). I would let this go for now (just like the asserts).

Myoldmopar · 2024-04-12T13:12:37Z

I actually just commented it out locally and am doing final testing. If I leave it, then when I merge it in a minute, Custom Check will start failing for everyone's branches. It's still in there, just commented to hush the check. Is that OK?

Myoldmopar · 2024-04-12T13:30:42Z

And as expected CppCheck is happy now. No need to wait on the other CI, merging this one. Thanks @amirroth

amirroth added 5 commits April 8, 2024 09:35

Fan enumerations and data mining APIs.

7388b2b

Unify five FanPlace enums. Fix Furnace issue.

9c377a4

FanType enum everywhere

73fa4ab

Merge develop, fix AFN issue

f3f7cf9

Fix VRF error message

97ab13b

amirroth requested review from rraustad and Myoldmopar April 10, 2024 12:53

amirroth added DoNotPublish Includes changes that shouldn't be reported in the changelog Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring labels Apr 10, 2024

amirroth added this to the EnergyPlus 24.2 milestone Apr 10, 2024

amirroth commented Apr 10, 2024

View reviewed changes

amirroth marked this pull request as draft April 10, 2024 13:18

rraustad reviewed Apr 10, 2024

View reviewed changes

amirroth added 2 commits April 10, 2024 12:30

Address reviewer comments

09d2c2b

Fix error message in UnitarySystem

bb83759

fix CppCheck issues

0abd115

Custom check and clang format

9686d9b

Myoldmopar marked this pull request as ready for review April 12, 2024 13:30

Myoldmopar merged commit ec1780a into develop Apr 12, 2024
10 of 15 checks passed

Myoldmopar deleted the HVACGlobals2 branch April 12, 2024 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quick Fan Cleanup #10462

Quick Fan Cleanup #10462

amirroth commented Apr 10, 2024

amirroth left a comment

amirroth Apr 10, 2024

amirroth Apr 10, 2024

amirroth Apr 10, 2024

amirroth Apr 10, 2024

amirroth Apr 10, 2024

amirroth Apr 10, 2024

amirroth Apr 10, 2024

amirroth Apr 10, 2024

amirroth Apr 10, 2024

amirroth Apr 10, 2024

rraustad Apr 10, 2024

amirroth Apr 10, 2024 •

edited

Loading

rraustad Apr 10, 2024

rraustad Apr 10, 2024

rraustad Apr 10, 2024

rraustad Apr 10, 2024

amirroth Apr 10, 2024

rraustad Apr 10, 2024

amirroth Apr 10, 2024

amirroth Apr 10, 2024

rraustad Apr 10, 2024

amirroth Apr 10, 2024

rraustad Apr 10, 2024 •

edited

Loading

amirroth commented Apr 10, 2024

Myoldmopar commented Apr 11, 2024

rraustad commented Apr 12, 2024

amirroth commented Apr 12, 2024

Myoldmopar commented Apr 12, 2024

Myoldmopar commented Apr 12, 2024

		@@ -378,6 +382,13 @@ namespace DesiccantDehumidifiers {

		desicDehum.RegenCoilType = Alphas(8);
		desicDehum.RegenCoilName = Alphas(9);


		void GetFanVolFlow(EnergyPlusData &state, int FanIndex, Real64 &FanVolFlow);
		Real64 GetFanVolFlow(EnergyPlusData &state, int FanIndex);

Quick Fan Cleanup #10462

Quick Fan Cleanup #10462

Conversation

amirroth commented Apr 10, 2024

amirroth 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

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

amirroth Apr 10, 2024 • edited Loading

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

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 Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

amirroth commented Apr 10, 2024

Myoldmopar commented Apr 11, 2024

rraustad commented Apr 12, 2024

amirroth commented Apr 12, 2024

Myoldmopar commented Apr 12, 2024

Myoldmopar commented Apr 12, 2024

amirroth Apr 10, 2024 •

edited

Loading

rraustad Apr 10, 2024 •

edited

Loading