Skip to content
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

Improved Support for Multi-speed Fans #9746

Merged
merged 34 commits into from
Aug 16, 2023
Merged

Improved Support for Multi-speed Fans #9746

merged 34 commits into from
Aug 16, 2023

Conversation

lgu1234
Copy link
Contributor

@lgu1234 lgu1234 commented Nov 18, 2022

Pull request overview

Allow multiple supply air flow ratio to be added to the ZoneHVAC:WaterToAirHeatPump and ZoneHVAC:TerminalUnit:VariableRefrigerantFlow objects. In simulation, for each time step, the fan air flow shall cycle between adjacent speeds based on the cooling / heating capacity (or coordinate with the cooling / heating coil stage for variable speed water to air coils).

NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE

Pull Request Author

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

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
  • If any diffs are expected, author must demonstrate they are justified using plots and descriptions
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions
  • If any defect files are updated to a more recent version, upload new versions here or on DevSupport
  • If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
  • If structural output changes, add to output rules file and add OutputChange label
  • If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

@lgu1234 lgu1234 added NewFeature Includes code to add a new feature to EnergyPlus IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) labels Nov 18, 2022

<span style="color:red">

An alternative way is to use the existing object of UnitarySystemPerformance:Multispeed. Since this object is not used only for UnitarySystem, the name of the object may be changed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgu1234 I would support changing the existing object name rather than making a new object. Of course, that will require a transition rule for the existing field in AirloopHVAC:UnitarySystem.

\memo operating speed. This object is primarily used for ZoneHVAC:WaterToAirHeatPump and
\memo ZoneHVAC:TerminalUnit:VariableRefrigerantFlow objects to allow operation at
\memo different fan flow rates.
\extensible:2 - repeat last two fields, remembering to remove ; from "inner" fields.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rraustad If this object is truly extensible, then the fields for Number of Speeds for Heating/Cooling should not have \maximum declared in the IDD.

Comment on lines 661 to 664
\note Used only for Multi speed coils
\note Enter the next highest operating supply air flow ratio during heating
\note operation or specify autosize. This value is the ratio of air flow
\note at this speed to the maximum air flow rate.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These notes don't need to be repeated for every speed.

@lgu1234
Copy link
Contributor Author

lgu1234 commented Dec 1, 2022

@mjwitte @Myoldmopar @rraustad @EnergyArchmage

Based on discussion in the conference call today, I revised the NFP by adding a section to show differences between between UnitarySystemPerformance:Multispeed and Fan:SystemModel for review.

The Fan:SystemModel does not have No Load Supply Air Flow Rate Ratio, and different fan flow ratios between heating and cooling.

Justification
No Load Supply Air Flow Rate Ratio
As long as the No Load Supply Air Flow Rate is designed in the parent object correctly, there is no need to have this field.

Different fan flow ratios between heating and cooling
The real system has only a single supply fan.

Please let me know your comments via E-mail.

Thanks.

@nrel-bot-3
Copy link

@lgu1234 @Myoldmopar it has been 28 days since this pull request was last updated.

@mjwitte
Copy link
Contributor

mjwitte commented Jan 4, 2023

@lgu1234 Using the speeds in Fan:SystemModel to inform the parent object is desirable, because it avoids duplicate input. But it would introduce a new paradigm.

My understanding is that none of the current parent objects mine this information from the fan. The parent objects decide on a flow rate and tell the fan where it's operating. Then the fan calculates it's power use based on interpolating between the speeds. @rraustad is that correct?

Since ZoneHVAC:WaterToAirHeatPump is actually using the UnitarySystem code, the most expedient approach seems to be to use UnitarySystemPerformance:Multispeed for this one. It should require minimal new code.

If UnitarySystem does not currently throw any warnings for inconsistent flow fractions between UnitarySystemPerformance:Multispeed and the Fan:SystemModel object, then I would suggest adding these checks as part of this PR.

For ZoneHVAC:TerminalUnit:VariableRefrigerantFlow it would then make sense to keep the same approach and use UnitarySystemPerformance:Multispeed.

Another alternative is to support both methods: If UnitarySystemPerformance:Multispeed is specified, then use it. If not, check the fan inputs and use those.

@rraustad @EnergyArchmage ?

@lgu1234
Copy link
Contributor Author

lgu1234 commented Jan 9, 2023

@mjwitte Thanks for your comments. I updated NFP based on your comments and uploaded it a while ago. Since I have not received any more comments from @rraustad and @EnergyArchmage, I assume they agree with your comments. Here is a summary of the NFP update:

The object of UnitarySystemPerformance:Multispeed will be used in this new feature for both ZoneHVAC:WaterToAirHeatPump and ZoneHVAC:TerminalUnit:VariableRefrigerantFlow.

Warnings will be issued if UnitarySystem does not currently throw any warnings for inconsistent flow fractions between UnitarySystemPerformance:Multispeed and the Fan:SystemModel object, but speed fractions from UnitarySystemPerformance:Multispeed will be used.

The alternative method will be included if I have time left: If UnitarySystemPerformance:Multispeed is specified, then use it. If not, check the fan inputs and use those.

I am going to start design document.

@Myoldmopar
Copy link
Member

I am making my pass through the PR list, and I assume based on the status here that this is going in 23.2, so marking it as such. If it's intended for 23.1 let me know!

@Myoldmopar Myoldmopar added this to the EnergyPlus 23.2 IOFreeze milestone Feb 9, 2023
@lgu1234
Copy link
Contributor Author

lgu1234 commented Feb 13, 2023

@Myoldmopar I plan to complete it for the fall release due to family emergency.

@rraustad
Copy link
Contributor

My understanding is that none of the current parent objects mine this information from the fan. The parent objects decide on > a flow rate and tell the fan where it's operating. Then the fan calculates it's power use based on interpolating between the
speeds. @rraustad is that correct?

@mjwitte there is a fan flow variable in UnitarySystem but for the max flow and not related to speeds.

if (this->m_FanExists) {
    if (this->m_FanType_Num == DataHVACGlobals::FanType_SystemModelObject) {
        this->m_ActualFanVolFlowRate = state.dataHVACFan->fanObjs[this->m_FanIndex]->designAirVolFlowRate;
    } else {
        this->m_ActualFanVolFlowRate = Fans::GetFanDesignVolumeFlowRate(
            state, blankString, blankString, state.dataUnitarySystems->initUnitarySystemsErrorsFound, this->m_FanIndex);
    }
}

@nrel-bot-3
Copy link

@lgu1234 @Myoldmopar it has been 28 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@lgu1234 it has been 12 days since this pull request was last updated.

@nrel-bot-2
Copy link

@lgu1234 it has been 7 days since this pull request was last updated.

@@ -486,6 +486,7 @@ namespace UnitarySystems {
bool m_ZoneCompFlag = true;
std::string m_AvailManagerListName;
int m_EquipCompNum = 0; // 1-based index of this parent type for specific equipment type processing
bool reportVariablesAreSetup = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this new variable used? I can't find it anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rraustad No need to use this variable. Remove it.

this->m_HeatingAuxElecConsumption += this->m_AncillaryOffPower * (1.0 - this->m_PartLoadFrac) * ReportingConstant;
}
elecHeatingPower = state.dataHVACGlobal->DXElecHeatingPower;
} break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 cases look the same. Why were they split out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rraustad I am going to combine both.

((state.dataUnitarySystems->unitarySys[sysNum].m_HeatingCoilType_Num == DataHVACGlobals::Coil_HeatingWaterToAirHPSimple ||
state.dataUnitarySystems->unitarySys[sysNum].m_HeatingCoilType_Num ==
DataHVACGlobals::Coil_HeatingWaterToAirHPVSEquationFit) &&
state.dataUnitarySystems->unitarySys[sysNum].m_NumOfSpeedHeating > 1)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you simply use:

if (this->m_MultiSpeedCoolingCoil || this->m_MultiSpeedHeatingCoil)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rraustad It may not be simplified, because these output variables are used only for Coil_Cooling/heatingWaterToAirHPSimple and Coil_Cooling/heatingWaterToAirHPVSEquationFit object. m_MultiSpeedCoolingCoil and m_MultiSpeedHeatingCoil many cover other air source coils also.

Real64 FanSystem::AirMassFlowRateAtSpeed(int const SpeedNum)
{
return m_massFlowAtSpeed[SpeedNum];
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If m_numSpeeds and m_massFlowAtSpeed were public you wouldn't need these new functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any problems to make these two members as public. I am going to wait a couple of days.

@Myoldmopar
Copy link
Member

@lgu1234 it looks like the last time this got good results. Some diffs, but every file passing except the one debug case. That one was failing on all branches, and unrelated to your work. If you want a fully clean build, you can pull develop in one more time, but it's not necessary now. I'll look it over right now. It looks like the diffs have been scrutinized and are basically just order changes, not really significant diffs. And the design was discussed multiple times, so I don't think I need any other review input here. But of course if you want to also provide final thoughts, they would of course be welcome @mjwitte @rraustad.

Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgu1234 I made a few comments on here. Some are suggestions, some are a little more. Let me know what you think and we'll see about getting this wrapped up.

A22; \field Design Specification Multispeed Object Name
\type object-list
\object-list UnitarySystemPerformaceNames
\note Enter the name of the performance specification object used to describe the multispeed coil or fan.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two optional fields, no transition 👍

A21; \field Design Specification Multispeed Object Name
\type object-list
\object-list UnitarySystemPerformaceNames
\note Enter the name of the performance specification object used to describe the multispeed coil or fan.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two optional fields, no transition 👍

A22; \field Design Specification Multispeed Object Name
\type object-list
\object-list UnitarySystemPerformaceNames
\note Enter the name of the performance specification object used to describe the multispeed coil or fan.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wording here is a little funny, but maybe there's precedent:

\note Enter the name of the performance specification objec

The phrase "Enter the name of..." sounds like it's one of the many interactive text-based games I probably don't still play in 2023. If there is any other change to make, I'd prefer to at least take out the word "Enter". @mjwitte any thoughts on this one?

@@ -351,11 +351,11 @@ namespace HVACFan {

FanSystem::FanSystem(EnergyPlusData &state, std::string const &objectName)
: availSchedIndex(0), inletNodeNum(0), outletNodeNum(0), designAirVolFlowRate(0.0), speedControl(SpeedControlMethod::NotSet), deltaPress(0.0),
designElecPower(0.0), powerModFuncFlowFractionCurveIndex(0), AirLoopNum(0), AirPathFlag(false), fanIsSecondaryDriver(false),
designElecPower(0.0), powerModFuncFlowFractionCurveIndex(0), AirLoopNum(0), AirPathFlag(false), m_numSpeeds(0), fanIsSecondaryDriver(false),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, but I would request that any future time you are adding or modifying class member variables, just initialize them on the class declaration itself, and don't perpetuate these long initializer lists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Myoldmopar I can do it to initialize class member variables. In order to be consistent with existing structure, I prefer to make any changes later after the new feature is merged. Thanks.

@@ -140,6 +140,8 @@ namespace HVACFan {
int powerModFuncFlowFractionCurveIndex; // pointer to performance curve or table
int AirLoopNum; // AirLoop number
bool AirPathFlag; // Yes, this fan is a part of airpath
int m_numSpeeds; // input for how many speed levels for discrete fan
std::vector<Real64> m_massFlowAtSpeed;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A regular vector, always nice to see standard types. The size of this isn't known at compile time, is it? (If so, we should make it a fixed size std::array...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Myoldmopar Unfortunately, the size of vector is unknown at compile time. A fixed size of array may not be used.

@@ -3888,7 +3888,8 @@ namespace WaterToAirHeatPumpSimple {
state.dataWaterToAirHeatPumpSimple->GetCoilsInputFlag = false;
}

if (CoilType == "COIL:COOLING:WATERTOAIRHEATPUMP:EQUATIONFIT" || CoilType == "COIL:HEATING:WATERTOAIRHEATPUMP:EQUATIONFIT") {
if (UtilityRoutines::SameString(CoilType, "COIL:COOLING:WATERTOAIRHEATPUMP:EQUATIONFIT") ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm seems like a missed opportunity to refactor this function. I don't think we need to be passing a string in for the coil type. Unless I'm missing something, I think it should be one of the equipment type enum values. But this function is likely used in a bunch of spots, so I will just pause on this one, and we can clean it up later. I just ........ well, I need to check how frequently this function is used during the simulation. @mjwitte do you have an idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Myoldmopar OK. I am going to pause for the time being.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say leave this for now, but open a new refactoring task to clean up both ``WaterToAirHeatPumpSimple::GetCoilAirFlowRate and VariableSpeedCoils::GetCoilAirFlowRateVariableSpeed.

a. The coil type strings should be enums. The calling functions should already know the coil type enum by the time this is called.
b. The coil name can be replaced with coil index. Again, the calling functions should already know this.
c. I think the check for getinput can be removed from these and just test for a valid coil index and a valid type enum and throw an error.
@rraustad Would you agree?

Copy link
Contributor

@rraustad rraustad Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree. DataHVACGlobals has a CoilType enum and coilTypeNamesUC array that can be used for that. If the coil name is needed for something it can be looked up. Regarding getting rid of getInput in these mining functions, I'm not sure of that at this point. The calling function would need to use the index on this call and that might be a little more work to cover all parent objects. If the index is known the data could be read directly from the state struct and the mining functions are no longer needed, except for getIndex. We are definitily moving in this direction.

See this type of refactor where the mining functions were removed in this commit.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good, I don't even have to make these types of comments anymore! I can move on to better and more pedantic comments about the use of this->m_X.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjwitte @rraustad @amirroth I leave it as is for future refactory.

double(NumOfSpeed)));
ShowContinueError(state, format("... The number of heating coil speeds in the {} will be used.", MultispeedType));
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how nice this would look as a switch/case block instead of nested IFs.

@lgu1234
Copy link
Contributor Author

lgu1234 commented Jul 6, 2023

@Myoldmopar I made a switch/case block instead of nested IFs.

@lgu1234
Copy link
Contributor Author

lgu1234 commented Jul 6, 2023

@Myoldmopar I addressed the comments and uploaded the change in Github. It is ready for further review.

Thanks.

@@ -4471,6 +4472,94 @@ void GetVRFInputData(EnergyPlusData &state, bool &ErrorsFound)
}
}

if (!lAlphaFieldBlanks(20) && !lAlphaFieldBlanks(21)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto &vrftu would be helpful here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amirroth Done. Thanks.

@@ -3067,6 +3074,27 @@ namespace UnitarySystems {
this->m_DesignCoolingCapacity = DataSizing::AutoSize;
if (this->m_HeatingCoilType_Num == DataHVACGlobals::Coil_HeatingWaterToAirHP)
EqSizing.DesHeatingLoad = state.dataSize->DataConstantUsedForSizing;
// airflow sizing with multispeed fan
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to make a general comment here. The "scope Hungarian" notation m_ means "member variable". Prepending access with this-> is only necessary in cases where a member variable is shadowed (i.e., has the same name) as a local variable, e.g., this->name = name; where the first name is the member and the second one is the local. Otherwise it is not necessary because C++ assumes that if a variable inside a method is not local then it must be a member. [Note: global variables have lower scope precedence than member variables, but we don't really have these in EnergyPlus anymore except for the various xNamesUC arrays].

Anyways, if all member variables start with m_ and local variables do not, then there is no possibility of name shadowing and therefore no need for this->.

TL;DR this->m_X is redundant from both a compiler and readability standpoint. The m_ tells both the compiler and the reader the same thing, i.e., that this variable is a member of the class that defines the current method. I would eliminate one of them, preferably this-> because it is longer and thus more annoying. Also, it's a keyword so it gets colorized and makes the whole screen of that color.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amirroth I agree with you. The changes you suggested is beyond this new feature scope. It is better to make the changes in the future factory.

for (int i = 1; i <= this->m_NumOfSpeedCooling; ++i) {
this->m_CoolMassFlowRate[i] = 0.0;
this->m_CoolVolumeFlowRate[i] = 0.0;
this->m_MSCoolingSpeedRatio[i] = 1.0;
}
if (this->m_CoolingCoilType_Num == DataHVACGlobals::Coil_CoolingWaterToAirHPVSEquationFit) {
std::string MultispeedType = "UnitarySystemPerformance:Multispeed";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make a string_view comment here, but there are bigger problems with the fact that Coil_ is not an enumeration so I will restrain myself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amirroth I will leave it as is and wait for better solution.

@nrel-bot-2b
Copy link

@lgu1234 it has been 14 days since this pull request was last updated.

@nrel-bot-2
Copy link

@lgu1234 it has been 7 days since this pull request was last updated.

@Myoldmopar
Copy link
Member

@lgu1234 thanks for resolving conflicts here!! I was thinking I'd end up having to do that. I'll look it over and get it built/tested.

@Myoldmopar
Copy link
Member

Branch sanity checks look good, no conflicts now, formatting and custom-check both pass, along with doc/cppcheck builds. Local build going now.

Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of great stuff in here, and it's been scrutinized enough. I found one documentation warning snuck in, so I fixed that and am doing final testing. Assuming it's all good, I'll push that one tiny fix and get this merged. Thanks @lgu1234

A22; \field Design Specification Multispeed Object Name
\type object-list
\object-list UnitarySystemPerformaceNames
\note Enter the name of the performance specification object used to describe the multispeed coil or fan.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The note actually still appears to say "Enter the name..." But it's fine, and not worth any other commits.

case DataHVACGlobals::Coil_HeatingElectric_MultiStage:
case DataHVACGlobals::Coil_HeatingGas_MultiStage:
case DataHVACGlobals::Coil_HeatingWaterToAirHPSimple:
case DataHVACGlobals::Coil_HeatingWaterToAirHPVSEquationFit: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The switch is so much nicer.

@Myoldmopar
Copy link
Member

Everything looks great! Text diffs in RDD, etc., but nothing of significance. Table diffs in some files, but it's due to some rows being reordered. This is good to go! Thanks @lgu1234 and reviewers!

@Myoldmopar Myoldmopar merged commit 46fa50b into develop Aug 16, 2023
@Myoldmopar Myoldmopar deleted the MultispeedFans branch August 16, 2023 19:41
@Myoldmopar Myoldmopar mentioned this pull request Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) NewFeature Includes code to add a new feature to EnergyPlus
Projects
None yet
Development

Successfully merging this pull request may close these issues.