-
Notifications
You must be signed in to change notification settings - Fork 460
New Phase Change Material (PCM) Thermal Storage #11113
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
…ransfer properties
Updated IDD File to reflect the added PCM Material Object Input
|
@Myoldmopar @rohitj0204 @Myoldmopar it has been 28 days since this pull request was last updated. |
| \item | ||
| Thermal Energy Storage Heat Loss Rate~~~~~~~~~~ !- HVAC Average [W] | ||
| \item | ||
| Thermal Energy Storage Energy Stored~~~~~~~~~~~~ !- HVAC Average [J] |
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 would also show !- HVAC Sum [J]
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.
Same response as above. The energy can go up and down, so average would be a right method
|
|
||
| // Stop here — WH minimal tests end after input parsing & sanity asserts. | ||
| // (No Init/Calculate: that would require plant context and is outside this simple parse test.) | ||
| } |
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 a new example file. Could a new example file be added please? Include the example file name and weather file in \testfiles\CMakeLists.txt
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.
Added the example file and pushing it to the branch now
| EnergyPlus::PCMStorage::RegisterPCMStorageOutputVariables(state); | ||
| this->MyPlantScanFlag = false; | ||
| } | ||
| Real64 rho = 998.2; |
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.
Shouldn't this come from the PCM material properties?
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 the density value of water. PCM material properties are mostly obtained from the PCM object connected
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.
But you should be getting the properties of plant loop fluid itself. It could be something other than water.
src/EnergyPlus/PCMThermalStorage.cc
Outdated
| if (mUseReq > 0.0) { | ||
| useOutlet.Temp = useOutletTemp; | ||
| } else if (mPlantReq > 0.0) { | ||
| plantOutlet.Temp = std::min(plantOutletTemp, state.dataPlnt->PlantLoop[this->sourcePlantLoc.loopNum].MaxTemp); |
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 the plant outlet temp gets clipped here then the calculation for plantheatTransfer_req, netPowerW and EnergyStored above are inaccurate. I suggest moving those calculations below this if/else if.
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.
Thanks for the suggestion. Made the changes.
… modified unit test to resolve build errors
| } | ||
| if (soc >= cutOutSOC) { | ||
| chargingMode = 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.
If there is ever more than 1 storage tank then the chargingMode variable should be a member variable to control a specific tank.
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.
Thanks Richard for your feedback. I can work on this and maybe have it as another update for future release.
|
@mitchute I'm a little less critical of new models since they need time for user testing and feedback. This looks pretty sound without that feedback. |
rraustad
left a comment
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.
Looks like a good new model.
|
@rohitj0204 if you want to make some last minute changes then now is the time. |
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.
Some nitpicks, but this is generally OK. Unless there's justification, I think we ought to clean up the fluid prop issues noted.
|
|
||
| // PCM Thermal Storage Module - PCMThermalStorage.cc | ||
|
|
||
| #include "PCMThermalStorage.hh" |
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 import syntax is inconsistent with the rest of E+.
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.
Updated and will push it through a new branch
| EnergyPlus::PCMStorage::RegisterPCMStorageOutputVariables(state); | ||
| this->MyPlantScanFlag = false; | ||
| } | ||
| Real64 rho = 998.2; |
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.
But you should be getting the properties of plant loop fluid itself. It could be something other than water.
| // If the tank capacity has been set to zero or negative (autosize request), estimate a capacity. | ||
| // The estimate here is based on storing one hour of flow on the larger of the use side or plant side. | ||
| // This provides a reasonable latent mass storage size without relying on external sizing objects. | ||
| if (this->TankCapacity <= 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.
Fine to leave, but I don't see how this would be hit.
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.
Updated Plant Loop Fluid Properties
| Real64 PCM_TankTemp = 0.0; // C, estimated from enthalpy | ||
| Real64 EnergyStored = 0.0; // J | ||
| Real64 PercentCapacity = 0.0; // % | ||
| Real64 HeatLossRate_W = 0.0; // W | ||
| Real64 useheatTransfer = 0.0; // W | ||
| Real64 plantheatTransfer = 0.0; // W | ||
| Real64 DesignMassFlowRate = 0.0; // kg/s | ||
| Real64 UseSideDesignFlowRate = 0.0; | ||
| Real64 PlantSideDesignFlowRate = 0.0; | ||
| Real64 UseSideMassFlowRate = 0.0; | ||
| Real64 PlantSideMassFlowRate = 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.
Variable naming convention isn't consistent. Not a big deal, just noting in case any other quick fixes are made.
| #include <EnergyPlus/ScheduleManager.hh> | ||
| #include <EnergyPlus/UtilityRoutines.hh> | ||
|
|
||
| #include <algorithm> // for std::max used in autosizing |
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.
Convention in E+ is to group C++ imports first, then any Objexx imports, then the E+ imports.
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.
When I use Visual Studio's Format Document option it automatically is rearranging everything. I hope this is not a major issue
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.
When I use Visual Studio's Format Document option it automatically is rearranging everything. I hope this is not a major issue
If you follow this pattern (e.g. PlantManager.cc), it will stay at the top even with VS formatting.
license text....
// POSSIBILITY OF SUCH DAMAGE.
// C++ Headers
#include <algorithm>
#include <cassert>
// ObjexxFCL Headers
| Real64 FreezingTemp = 0.0; // C | ||
| Real64 LatentHeat = 0.0; // J/kg (typically from PCMmat) | ||
| Real64 SpecificHeat = 0.0; // J/kg-K (backup if needed) | ||
| Real64 Effectiveness = 0.9; // HX effectiveness (optional input) |
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.
Should make this constexpr since you are not exposing it to the user (contrary to the comment).
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.
Done
| #include <EnergyPlus/PlantComponent.hh> | ||
| #include <string> |
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.
| // Hysteresis thresholds (tune as needed) | ||
| constexpr Real64 cutInSOC = 0.40; // start charging when SOC <= 40% | ||
| constexpr Real64 cutOutSOC = 0.95; // stop charging when SOC >= 95% |
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.
Maybe expose these to the users at some point? Are these described in the docs?
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.
Will do so as a future update. However, the current limits seem to be a good starting point and current fit for the industry works
| // Real64 avail = this->AvailabilitySchedule->getCurrentVal(); | ||
| Real64 dt_seconds = state.dataHVACGlobal->TimeStepSys * 3600.0; | ||
|
|
||
| Real64 CpWater = 4180.0; // J/kg-C |
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.
Another water property that should be pulled from the actual plant loop fluid props.
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.
Done
|
|
||
| // Real64 useheatTransfer = massFlowUse * CpWater * deltaTUse; // Heat to Water Heater | ||
| // Real64 plantheatTransfer = massFlowPlant * CpWater * deltaTPlant; // Heat to PCM Tank | ||
| HeatLossRate_W = HeatLossRate; |
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 two variables? There's no difference between how they are used.
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.
Thanks for pointing it out. Fixed this issue
|
As I stated on the call, if you all are good with the current state of the inputs, we can merge this and handle any lingering cleanups after IO Freeze. |
@mitchute Code cleanup should be ready by Friday. If you want to merge this now, then those can be done in a follow-up PR. It's up to you. |
|
This is ready, so I'll just merge now and you can follow up when ready. |
Pull request overview
Description of the purpose of this PR
This PR adds a new Phase Change Material (PCM) Thermal Storage model in EnergyPlus. The PCM storage object allows simulation of latent heat storage using encapsulated or bulk PCM, which can smooth thermal loads, reduce peak demand, and integrate with CHP or renewable systems.
Pull Request Author
Reviewer