Skip to content

Conversation

@rohitj0204
Copy link
Contributor

@rohitj0204 rohitj0204 commented Jul 2, 2025

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

  • 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

  • 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

@mjwitte mjwitte mentioned this pull request Jul 2, 2025
20 tasks
@mjwitte mjwitte 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 Jul 2, 2025
@mjwitte mjwitte marked this pull request as draft July 2, 2025 20:22
@mjwitte mjwitte added this to the EnergyPlus 25.2 IO Freeze milestone Aug 20, 2025
@Myoldmopar Myoldmopar self-assigned this Aug 27, 2025
@nrel-bot-2
Copy link

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

@mitchute mitchute self-requested a review September 24, 2025 18:19
@mjwitte mjwitte changed the title Thermal Storage PCM V3 New Phase Change Material (PCM) Thermal Storage Sep 24, 2025
\item
Thermal Energy Storage Heat Loss Rate~~~~~~~~~~ !- HVAC Average [W]
\item
Thermal Energy Storage Energy Stored~~~~~~~~~~~~ !- HVAC Average [J]
Copy link
Contributor

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]

Copy link
Contributor Author

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.)
}
Copy link
Contributor

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

Copy link
Contributor Author

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;
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Collaborator

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 (mUseReq > 0.0) {
useOutlet.Temp = useOutletTemp;
} else if (mPlantReq > 0.0) {
plantOutlet.Temp = std::min(plantOutletTemp, state.dataPlnt->PlantLoop[this->sourcePlantLoc.loopNum].MaxTemp);
Copy link
Contributor

@rraustad rraustad Oct 1, 2025

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.

Copy link
Contributor Author

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.

}
if (soc >= cutOutSOC) {
chargingMode = false;
}
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@rraustad
Copy link
Contributor

rraustad commented Oct 7, 2025

@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.

Copy link
Contributor

@rraustad rraustad left a 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.

@rraustad
Copy link
Contributor

rraustad commented Oct 7, 2025

@rohitj0204 if you want to make some last minute changes then now is the time.

Copy link
Collaborator

@mitchute mitchute left a 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"
Copy link
Collaborator

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+.

Copy link
Contributor Author

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;
Copy link
Collaborator

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) {
Copy link
Collaborator

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.

Copy link
Contributor Author

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

Comment on lines +94 to +104
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;
Copy link
Collaborator

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
Copy link
Collaborator

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.

Copy link
Contributor Author

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

Copy link
Contributor

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)
Copy link
Collaborator

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines +56 to +57
#include <EnergyPlus/PlantComponent.hh>
#include <string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment on lines +302 to +304
// 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%
Copy link
Collaborator

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?

Copy link
Contributor Author

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
Copy link
Collaborator

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.

Copy link
Contributor Author

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;
Copy link
Collaborator

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.

Copy link
Contributor Author

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

@mitchute
Copy link
Collaborator

mitchute commented Oct 8, 2025

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.

@mjwitte
Copy link
Contributor

mjwitte commented Oct 8, 2025

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.

@mitchute
Copy link
Collaborator

mitchute commented Oct 9, 2025

This is ready, so I'll just merge now and you can follow up when ready.

@mitchute mitchute merged commit eb45bfa into NREL:develop Oct 9, 2025
11 checks passed
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.

7 participants