-
Notifications
You must be signed in to change notification settings - Fork 389
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
Enhancement to crankcase heater #9949
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.
@yujiex A few comments below. Also, I don't see any doc changes yet.
@@ -51383,6 +51383,10 @@ Coil:Cooling:DX:CurveFit:Performance, | |||
\note is used as Reheat mode. | |||
\type object-list | |||
\object-list DXCoolingOperatingModeNames | |||
A8; \field Outdoor Temperature Dependent Crankcase Heater Capacity Curve Name |
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.
To be consistent with other curve names this should be
"Crankcase Heater Capacity Function of Temperature Curve Name"
ref.
Coil:Cooling:DX:SingleSpeed, Total Cooling Capacity Function of Temperature Curve Name
Coil:Cooling:DX:CurveFit:Speed, Total Cooling Capacity Modifier Function of Temperature Curve Name
Also this should be tagged with
\type object-list
\object-list UnivariateFunctions
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.
Right, I'll change the name to "Crankcase Heater Capacity Function of Temperature Curve Name" to match the existing patterns and add the object-list tag.
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.
Right, I'll change the name to "Crankcase Heater Capacity Function of Temperature Curve Name" to match the existing patterns and add the object-list tag.
These changes are still needed. Also, for each affected object, \min-fields should be increased by one (unless the new field is already beyond the current min-fields value).
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.
right right, I'll change min-fields too for ones too for those with the new curve field not beyond the current min-field.
idd/Energy+.idd.in
Outdated
\type object-list | ||
\object-list UnivariateFunctions | ||
\note quadratic curve = a + b*ffa + c*ffa**2 | ||
\note cubic curve = a + b*ffa + c*ffa**2 + d*ffa**3 | ||
\note ffa = Fraction of the full load Air Flow | ||
A50; \field Outdoor Temperature Dependent Crankcase Heater Capacity Curve Name |
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.
Hmm, the group of speed fields is currently not extensible, so technically, this is ok to add at the end of the object, but it just seems lost here. @Myoldmopar do we care?
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.
Yeah, I'm not certain whether I should put it at the end or near the other crankcase heater fields.
@@ -117,6 +118,7 @@ struct CoilCoolingDXCurveFitPerformance | |||
std::string name; | |||
Real64 crankcaseHeaterCap = 0.0; | |||
Real64 crankcaseHeaterPower = 0.0; | |||
Real64 crankcaseHeaterCapacityCurveIndex = 0.0; |
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.
Integer
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.
It was fixed in another commit. I'll push it soon. I was trying to figure out some issues with the variable speed coils curve application.
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.
It was fixed in another commit.
This is still not an integer. Did you by chance forget to push a commit?
EXPECT_EQ("COIL COOLING DX 1", thisCoil.name); | ||
EXPECT_EQ("COIL COOLING DX CURVE FIT PERFORMANCE 1", thisCoil.performance.name); | ||
EXPECT_EQ("HEATERCAPCURVE", Curve::GetCurveName(*state, thisCoil.performance.crankcaseHeaterCapacityCurveIndex)); | ||
} |
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.
Add a return to the last line.
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.
an explicit return like this return;
? @mjwitte
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.
Sorry, no, a return. Look at the display in the PR and you'll see a red circle at the end, because the last line of the file is not terminated fully.
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.
Oh! I understand now. I'll add a new line in the end.
tst/EnergyPlus/unit/DXCoils.unit.cc
Outdated
",", | ||
",", | ||
",", | ||
",", | ||
",", | ||
",", | ||
",", | ||
",", | ||
"heaterCapCurve5; !- Outdoor Temperature Dependent Crankcase Heater Capacity Curve Name", |
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.
And this shows why the new field should be placed before the speed groups for variable speed and multi-speed coils, even if it forces a transition rule. @Myoldmopar ?
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.
yeah, this is kind of tedious.
if (!input_data.outdoor_temperature_dependent_crankcase_heater_capacity_curve_name.empty()) { | ||
this->crankcaseHeaterCapacityCurveIndex = | ||
Curve::GetCurveIndex(state, input_data.outdoor_temperature_dependent_crankcase_heater_capacity_curve_name); | ||
} |
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.
The input processing needs more steps here (and everywhere) to check if a valid curve was found and to check the curve dimensions. For example, see here. For this application, any uni-variate curve or table is acceptable.
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.
good point. I'll add in the curve validations.
@yujiex it has been 7 days since this pull request was last updated. |
3 similar comments
@yujiex it has been 7 days since this pull request was last updated. |
@yujiex it has been 7 days since this pull request was last updated. |
@yujiex it has been 7 days since this pull request was last updated. |
} else { | ||
ShowSevereError(state, format("{}{}=\"{}\", invalid", RoutineName, CurrentModuleObject, thisDXCoil.Name)); | ||
ShowContinueError(state, format("...not found {}=\"{}\".", cAlphaFields(12 + (I - 1) * 6), Alphas(15 + (I - 1) * 6))); | ||
ShowContinueError(state, format("...not found {}=\"{}\".", cAlphaFields(13 + (I - 1) * 6), Alphas(13 + (I - 1) * 6))); |
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.
Was this a mistake in code and was corrected? I see an alpha index of 12 and 15 before, and now I see 13 and 13. If this fixes a mistake, which it looks like it does, then that should be added to the PR description.
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.
It should probably be 13 as the error message was stating what the current value of this field is, so the cAlphaFields and Alphas should have the same index. The 12th (one before) and 14th (one after) fields also have similar formats. So I corrected it.
@@ -9319,6 +9401,9 @@ void CalcDoe2DXCoil(EnergyPlusData &state, | |||
// If used in a heat pump, the value of MaxOAT in the heating coil overrides that in the cooling coil (in GetInput) | |||
if (CompAmbTemp < thisDXCoil.MaxOATCrankcaseHeater) { | |||
CrankcaseHeatingPower = thisDXCoil.CrankcaseHeaterCapacity; | |||
if (thisDXCoil.CrankcaseHeaterCapacityCurveIndex > 0) { | |||
CrankcaseHeatingPower *= Curve::CurveValue(state, thisDXCoil.CrankcaseHeaterCapacityCurveIndex, CompAmbTemp); | |||
} |
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.
This code could have probably been added to ReportDXCoil, instead of each calc funtion. It's OK as is, just noting an alternate way to account for the curve value.
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.
yeah, I do see a lot of places with this type of calculation.
Real64 CrankcaseHeaterCapacity; // total crankcase heater capacity [W] | ||
Real64 CrankcaseHeaterPower; // report variable for average crankcase heater power [W] | ||
Real64 MaxOATCrankcaseHeater; // maximum OAT for crankcase heater operation [C] | ||
int CrankcaseHeaterCapacityCurveIndex; // Crankcase heater power-temperature curve or table index |
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.
I am not suggesting a change here, but when adding new struct varaiables, declare inline (i.e., = 0) instead of adding to the constructor. Better yet, delete the constructor and decalre everything inline.
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.
make sense. Should probably gradually phase these out and use inline.
src/EnergyPlus/VariableSpeedCoils.cc
Outdated
if (state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).BasinHeaterSchedulePtr == 0) { | ||
ShowWarningError( | ||
state, | ||
format("{}{}=\"{}\", invalid", RoutineName, CurrentModuleObject, state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).Name)); | ||
ShowContinueError(state, format("...not found {}=\"{}\".", cAlphaFields(14), AlphArray(9))); | ||
ShowContinueError(state, format("...not found {}=\"{}\".", cAlphaFields(14), AlphArray(10))); |
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.
Why does this cAlphaFields index point to a different input than AlphArray?
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.
I need to fix this
src/EnergyPlus/VariableSpeedCoils.cc
Outdated
} | ||
ErrorsFound = true; | ||
} else { | ||
CurveVal = CurveValue(state, state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).PLFFPLR, 1.0); | ||
if (CurveVal > 1.10 || CurveVal < 0.90) { | ||
if (CurveVal > 1.11 || CurveVal < 0.90) { |
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.
Oops! This should be 1.10 (i.e., 10% tolerance).
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.
oh no I probably did some global replace and didn't see this ... I'll fix it.
src/EnergyPlus/VariableSpeedCoils.cc
Outdated
ShowWarningError(state, | ||
format("{}{}=\"{}\", curve values", | ||
RoutineName, | ||
CurrentModuleObject, | ||
state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).Name)); | ||
ShowContinueError(state, format("...{} output is not equal to 1.0 (+ or - 10%) at rated conditions.", cAlphaFields(10))); | ||
ShowContinueError(state, format("...{} output is not equal to 1.0 (+ or - 11%) at rated conditions.", cAlphaFields(11))); |
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.
and same here, 10%
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.
Wow, that's a lot of changes to add a curve object. There are 2 comments that should be addressed.
@@ -1182,7 +1182,7 @@ namespace VariableSpeedCoils { | |||
ShowWarningError( | |||
state, | |||
format("{}{}=\"{}\", invalid", RoutineName, CurrentModuleObject, state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).Name)); | |||
ShowContinueError(state, format("...not found {}=\"{}\".", cAlphaFields(14), AlphArray(10))); | |||
ShowContinueError(state, format("...not found {}=\"{}\".", cAlphaFields(10), AlphArray(10))); |
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.
This looks correct now.
@@ -1717,7 +1717,7 @@ namespace VariableSpeedCoils { | |||
RoutineName, | |||
CurrentModuleObject, | |||
state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).Name)); | |||
ShowContinueError(state, format("...not found {}=\"{}\".", cAlphaFields(AlfaFieldIncre), AlphArray(14 + (I - 1) * 6))); | |||
ShowContinueError(state, format("...not found {}=\"{}\".", cAlphaFields(AlfaFieldIncre), AlphArray(AlfaFieldIncre))); |
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.
This corrects a mistake...
@@ -1810,7 +1810,7 @@ namespace VariableSpeedCoils { | |||
RoutineName, | |||
CurrentModuleObject, | |||
state.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).Name)); | |||
ShowContinueError(state, format("...not found {}=\"{}\".", cAlphaFields(AlfaFieldIncre), AlphArray(16 + (I - 1) * 6))); | |||
ShowContinueError(state, format("...not found {}=\"{}\".", cAlphaFields(AlfaFieldIncre), AlphArray(AlfaFieldIncre))); |
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.
...and this corrects a mistake. These corrections should be described in the PR description. To be explicit, an issue would be created that this PR corrects.
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.
Let me create an issue and link it to the PR
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.
Created it: #10137
Hi @rraustad and @Myoldmopar, I addressed the comments and merged in develop. Would you mind taking another look? Thanks |
@rraustad looks like you are satisfied based on the comments addressed above. Could you mark it as approved if so? I'll look over things and build/test now. |
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.
Comments have been addressed. This is ready to merge.
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.
I need to work on the transition stuff before I call it ready to go, but it certainly is close.
@@ -1358,6 +1363,7 @@ | |||
DXCoilAirInletNode, !- Air Inlet Node Name | |||
DXCoilAirOutletNode, !- Air Outlet Node Name | |||
, !- Crankcase Heater Capacity {W} | |||
, !- Crankcase Heater Capacity Function of Temperature Curve Name |
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.
OK, datasets updated manually with new field which will require transition.
@@ -117,6 +118,7 @@ struct CoilCoolingDXCurveFitPerformance | |||
std::string name; | |||
Real64 crankcaseHeaterCap = 0.0; | |||
Real64 crankcaseHeaterPower = 0.0; | |||
Real64 crankcaseHeaterCapacityCurveIndex = 0.0; |
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.
It was fixed in another commit.
This is still not an integer. Did you by chance forget to push a commit?
@@ -493,6 +493,30 @@ SUBROUTINE CreateNewIDFUsingRules(EndOfFile,DiffOnly,InLfn,AskForInput,InputFile | |||
OutArgs(23:CurArgs+4)=InArgs(19:CurArgs) | |||
CurArgs = CurArgs + 4 | |||
|
|||
CASE('COIL:COOLING:DX:CURVEFIT:PERFORMANCE') |
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.
I thought I was going to be merging this right now but this is another big set of transitions for the coils. I need to look this over deeper and ... probably? ... do some manual merging of these rules.
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.
This is still not an integer. Did you by chance forget to push a commit?
Let me see. I changed it in other places in the header but missed this line somehow.
Thanks @rraustad, it's definitely close, but I need to spend some time with the transition code before merging. |
OK, I spent a little time in there this evening. As I did with the other coil transition branch, I ran transition on the full set of 23.1 example files, which passed fine. I then ran EnergyPlus on the manually-converted example files in this branch, along with the set of auto-converted example files I had just made. I compared the full set of ERR outputs and found no meaningful difference, meaning once again that the transition code appears to be working happily on an extensive array of configurations. While I was looking at the ERR file diff, I noticed some files were still 23.1 version, so I cleaned those up. I also integrated the transition rule markdown file...the variable speed DX coil now has changes related to the 90.1 metrics, the latent/curve stuff, and now this crankcase change. FYI @jmarrec !!! As long as there isn't another coil transition PR in the queue, we should be good. I am uninterested in doing this deep of a transition test again this week. So I think everything is happy now. I'll let CI run and check in the morning, but if it's all clean I plan to merge it in. |
And local regression testing is showing only 2 ERR diffs, which are introduced by me. They are simply cleaning up the bad version number in a couple files. Hopefully Decent agrees with me on that diff result. |
OK, so the failures showing up are also showing up on other branches. Apparently a previous coil started producing duplicate output columns and mathdiff is fumbling. Other than that, this looks clean though, so I'm merging it and I'll investigate that other issue. Thanks @yujiex |
Thanks @Myoldmopar =) |
@Myoldmopar Any specific action items for me to tackle here since you tagged me? |
Pull request overview
Crankcase heaters use a simple electrical circuit to create heat within the crankcase when the compressor of the chiller or heat pump is not in use. The heat keeps the refrigerant from condensing and prevents the refrigerant from migrating through the seals into the oil. Poor control of the crankcase heater could result in excessive energy consumption. Some of the existing products start to provide variable heater power based on different outdoor temperature conditions. Currently, EnergyPlus can only model a constant power crankcase heater with a temperature cutoff. This feature proposes to enhance the crankcase heater in EnergyPlus to allow for variable heater power, which enables to model realistic performance of heat pumps.
testing with a testfile
Running HeatPump_variableCrankcaseHeaterCapacity.idf, with a crankcase heater capacity curve of y = 60 - 2x. The following plot shows the relationship between outdoor temperature and heating coil crankcase heater electricity rate, limited to the time period with runtime fraction = 0.
The following is the simulation result of HeatPumpVSAS_variableCrankcaseHeaterCapacity.idf with variable speed coils using the same crankcase heater capacity curve. Column C computes the crankcase heater power before adjusting for the runtime fraction
Comparing output of a HeatPumpVSAS.idf and HeatPumpVSAS_variableCrankcaseHeaterCapacity.idf with constant curve
As anticipated, the crankcase heater power are the same for the two cases
Comparing a curve with a constant capacity of 200 and a curve with a constant capacity of 100
Summary of some identified issues
1. The crankcase heater for the variable speed heat pump water heater might be off all the time
In the file VariableSpeedCoils.cc, the following chunk of code (in the function CalcVarSpeedHPWH) here modifies the temporary variable
CrankcaseHeatingPower
which is then used in the calculation ofstate.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).CrankcaseHeaterPower
later on. It seems that the check of whether the temperature is appropriate for the crankcase heater to operate is only executed when EvapInletNode is 0. But this variable gets value fromstate.dataVariableSpeedCoils->VarSpeedCoil(DXCoilNum).AirInletNodeNum
which is from a required field. So it can’t be 0 in normal cases. This might mean the crankcase heater power will always be 02. Some inconsistency in what temperature should be compared to the maximum temperature threshold below which the crankcase heater should operate
See this issue: #10052
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.
Reviewer
This will not be exhaustively relevant to every PR.