-
Notifications
You must be signed in to change notification settings - Fork 398
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
Correct crash with DOAS VS DX coils and fix system name in Coil Sizing tables #10334
Correct crash with DOAS VS DX coils and fix system name in Coil Sizing tables #10334
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.
Walk-thru
@@ -853,7 +853,6 @@ namespace AirLoopHVACDOAS { | |||
if (state.dataGlobal->BeginEnvrnFlag && this->MyEnvrnFlag) { | |||
bool ErrorsFound = false; | |||
Real64 rho; | |||
state.dataSize->CurSysNum = this->m_OASystemNum; |
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.
Underlying problem with the table reporting issues. This line reset CurSysNum to CurOASysNum which confused table reports. The component variables CurSysNum and CurOASysNum are set in SizingAirLoopDOAS as:
state.dataSize->CurSysNum = state.dataHVACGlobal->NumPrimaryAirSys + this->m_AirLoopDOASNum + 1;
state.dataSize->CurOASysNum = this->m_OASystemNum;
Whether these initializations should be moved to SimAirLoopHVACDOAS to emulate SimAirLoops is another question. I'm not sure these variables are used in anything other than sizing.
@@ -451,7 +451,7 @@ Real64 HeatingCapacitySizer::size(EnergyPlusData &state, Real64 _originalValue, | |||
|
|||
this->selectSizerOutput(state, errorsFound); | |||
|
|||
if (this->isCoilReportObject && this->curSysNum <= state.dataHVACGlobal->NumPrimaryAirSys) { | |||
if (this->isCoilReportObject) { |
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.
Many of these issues. The DOAS indexes are currently appended to the number of air loops (i.e., CurSysNum = 6 for a DOAS with 5 AirLoopHVAC objects). When code uses CurSysNum to access air loop arrays there is an access violation when DOAS components are sized. These subtle changes were made to get this defect file to run.
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.
Interesting. Should we actually use a separate counter for DOAS systems rather than keeping them magically in line with the other air loops? That's something for later, but just curious how you'd like to proceed in the future.
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.
We have CurZoneEqNum, CurSysNum, and CurOASysNum. I think we do need a CurDOASNum. While making this change I was also wondering what the long term fix was. Still not sure if if (this->isCoilReportObject && this->curDOASNum == 0)
is any better than what is here now.
c->airloopNum > 0 ? state.dataAirSystemsData->PrimaryAirSystems(c->airloopNum).Name : "N/A"); | ||
c->airloopNum > 0 && c->airloopNum <= state.dataHVACGlobal->NumPrimaryAirSys | ||
? state.dataAirSystemsData->PrimaryAirSystems(c->airloopNum).Name | ||
: "N/A"); |
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.
DOAS index violation. The thing I don't like about the if (airloopNum < NumPrimaryAirSys) is that DOAS information is not reported. This is of course fixable.
@@ -543,7 +548,7 @@ void ReportCoilSelection::doAirLoopSetup(EnergyPlusData &state, int const coilVe | |||
{ | |||
// this routine sets up some things for central air systems, needs to follow setting of an airloop num | |||
auto &c(coilSelectionDataObjs[coilVecIndex]); | |||
if (c->airloopNum > 0 && allocated(state.dataAirSystemsData->PrimaryAirSystems)) { | |||
if (c->airloopNum > 0 && c->airloopNum <= state.dataAirSystemsData->PrimaryAirSystems.size()) { |
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.
DOAS index violation within this block.
c->typeHVACname = "AirLoopHVAC:DedicatedOutdoorAirSystem"; | ||
int DOASSysNum = state.dataAirLoop->OutsideAirSys(c->oASysNum).AirLoopDOASNum; | ||
c->userNameforHVACsystem = state.dataAirLoopHVACDOAS->airloopDOAS[DOASSysNum].Name; | ||
} else if (c->airloopNum > 0 && c->zoneEqNum == 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.
Correct Coil Sizing tables to report DOAS type and name correctly.
@@ -1276,7 +1286,8 @@ void ReportCoilSelection::setCoilCoolingCapacity( | |||
// if ( c->zoneEqNum > 0 ) doZoneEqSetup( index ); | |||
c->oASysNum = curOASysNum; | |||
|
|||
if (curSysNum > 0 && c->zoneEqNum == 0 && allocated(state.dataSize->FinalSysSizing) && allocated(SysSizPeakDDNum)) { | |||
if (curSysNum > 0 && c->zoneEqNum == 0 && allocated(state.dataSize->FinalSysSizing) && allocated(SysSizPeakDDNum) && | |||
curSysNum <= state.dataHVACGlobal->NumPrimaryAirSys) { |
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.
DOAS index violation within this if block.
state.dataAirLoop->AirLoopControlInfo(state.dataSize->CurSysNum).UnitarySysSimulating = false; | ||
if (state.dataSize->CurSysNum <= state.dataHVACGlobal->NumPrimaryAirSys) { | ||
state.dataAirLoop->AirLoopControlInfo(state.dataSize->CurSysNum).UnitarySysSimulating = false; | ||
} |
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.
Protect access array violation when CoilSystem is used in DOAS systems.
RatedAirVolFlowRateDes = state.dataSize->FinalSysSizing(state.dataSize->CurSysNum).DesMainVolFlow; | ||
} else { | ||
RatedAirVolFlowRateDes = 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.
Add to VS DX coil sizing for coils in a DOAS. I did not update this code to use the sizing classes. Probably should have but was trying to get this file to run.
@@ -120,6 +120,7 @@ namespace DataSizing { | |||
// parameters for sizing | |||
constexpr int NonCoincident(1); | |||
constexpr int Coincident(2); | |||
constexpr int Combination(3); |
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.
If 1 airloop on a DOAS system sizes coincident and others size noncoincident then reporting for System Sizing Method Concurrence needs an alternate notation.
@@ -5014,6 +5043,8 @@ namespace VariableSpeedCoils { | |||
state.dataRptCoilSelection->coilSelectionReportObj->setCoilLvgAirTemp(state, varSpeedCoil.Name, varSpeedCoil.VarSpeedCoilType, SupTemp); | |||
state.dataRptCoilSelection->coilSelectionReportObj->setCoilLvgAirHumRat( | |||
state, varSpeedCoil.Name, varSpeedCoil.VarSpeedCoilType, SupHumRat); | |||
state.dataRptCoilSelection->coilSelectionReportObj->setCoilAirFlow( | |||
state, varSpeedCoil.Name, varSpeedCoil.VarSpeedCoilType, varSpeedCoil.RatedAirVolFlowRate, RatedAirFlowAutoSized); |
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.
Heating coils in OA systems were not reporting air flow in Coil Sizing tables. Or maybe it's just DOAS system, but there are times when air flow = -999 and this corrects that.
I have reviewed the code changes here and they look good. |
@rraustad @Myoldmopar it has been 28 days since this pull request was last updated. |
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.
These look good to me, I'll double check locally with other develop changes, but expect this to merge soon if clean.
Table diffs are quite expected and reasonable. Otherwise this is happy and ready to merge. Thanks @rraustad |
Pull request overview
Reporting corrections:
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.