-
Notifications
You must be signed in to change notification settings - Fork 448
Correct Handling of Improper Design Objects in Radiant/Convective Systems #11124
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
Conversation
For inputs where there was a :Design object that contained additional data, entering nothing or a bad name for the field of the base object pointing to the :Design object led to a crash of EnergyPlus. This was a problem for four models: Low Temp Radiant Constant Flow, Low Temp Radiant Variable Flow, Water Rad/Conv Baseboard, and Steam Rad/Conv Baseboard. This commit fixes the issue for all four, has corrections to the IDD, and includes some improvements/standardization to the IO Reference.
The fix for a missing design object was modified to work with existing error handling and a unit test was added.
For some reason, my local clang format did not catch this. Nor did a previous commit? Strange. Hopefully this makes ci happy...
\required-field | ||
\type alpha | ||
\reference BaseboardDesignObject | ||
\reference RadiantDesignObject |
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 means that if there are water and steam baseboards, the when entering the design object in either of the baseboard types, a list of both water and steam design objects would be provided and it would be hard to know which to select. That's not really a bad thing because I noticed that the 2 BB design objects were identical when all that is really needed is one ZoneHVAC:Baseboard:RadiantConvective:Design
object.
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. I didn't do the original work that split this input objects into a base and design component. I'm not sure what the rationale was if they are identical. All I know is that in the code the lists are kept separately. Not sure why in a single building there would be both hot water baseboards and steam baseboards, but in any case, I think this is probably fine as is.
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 don't see any real issues with this change and could merge as-is.
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.
Doing final testing locally now, but this looks good to go.
ErrorsFound); | ||
if (ErrorsFound) { | ||
break; | ||
} |
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'm really glad you made a reusable function and applied it in multiple places. Good stuff.
ShowContinueError(state, " The Design Object Name was not found or was left blank. This is not allowed."); | ||
ShowContinueError(state, format(" A valid Design Object Name must be provided for any {} object.", itemType)); | ||
} | ||
} |
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 a pretty general purpose function, so UtilityRoutines makes good enough sense. And good job not resetting errorFound to false inside here 👍
expectedName = "Second Name"; | ||
expectedPtr = 2; | ||
objectType = {"ZoneHVAC:LowTemperatureRadiant:VariableFlow"}; | ||
objectName = {"MyVarFlowRadSys"}; |
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.
Not sure why these are inside braces... but it's fine I guess.
" ** ~~~ ** A valid Design Object Name must be provided for any ZoneHVAC:Baseboard:RadiantConvective:Steam object.", | ||
}); | ||
EXPECT_TRUE(compare_err_stream(error_stringTest3, true)); | ||
EXPECT_TRUE(gotErrors); |
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 good stuff.
All happy, merging. |
Pull request overview
Description of the purpose of this PR
Four radiant/convective systems (variable flow low temperature radiant system, constant flow low temperature radiant system, hot water radiant/convective baseboard, and steam radiant/convective baseboard) use two input objects, a base object and a "Design" object, to define the system. The base object refers to the design object by name, but when the name referenced in the base object is incorrect or blank, this was leading to a hard crash of EnergyPlus. This has now been fixed for all four systems. All four utilize the same utility routine to determine whether or not the name referenced is valid. When it is not, EnergyPlus will produce appropriate error messages highlighting the problem and severe/fatal out. This input is now set as required in the IDD. In addition to a unit test to show that the new utility routine is working properly, modifications were made to the Input Output Reference to standardize both the hyperlinks and the organization in these four affected systems. No differences in the iteration tests are expected as this only affects files where there is a problem of this nature.
Pull Request Author
Reviewer