SetpointManager:ReturnTemperature:Chilled/HotWater never reads PlantLoop fluid glycol (E+ Bugfix)

PMP-Rsch commented 5 months ago

Issue overview

This is based on my post on UnmetHours: https://unmethours.com/question/99091/setpointmanagerreturntemperaturechilledhotwater-never-reads-plantloop-fluid-glycol-e-bugfix/

I work with E+9.4 but this bug is present on the most recent version. However I don't know how to modify the SC there so I can only explain it in E+ 9.4 which I believe is more than enough in this case, the issue is relatively simple.

The short version is that the SetpointManager:ReturnTemperature:Chilled/HotWater object has a bug that makes it not read the fluid of the plant that it is in and always uses water. Therefore if using any glycol the heat balance is wrong in the function that regulates the impulsion temperature.

The only affected file is Setpointmanager.cc. I will use the lines for the subroutine of the chilledwater for the explanation but the hotwater has the exact same code and bug. The subroutine that doesn't work properly is "DefineReturnWaterChWSetPointManager" and its hotwater counterpart, for chillwater that is lines 7606 to 7624:

// we need to know the plant to get the fluid ID in case it is glycol
   // but we have to wait in case plant isn't initialized yet
   // if plant isn't initialized, assume index=1 (water)
   int fluidIndex = 1;
   **if (this->plantLoopIndex == 0) {** **This here is the problem, this only happens once, at first iteration**
       for (int plantIndex = 1; plantIndex <= DataPlant::TotNumLoops; plantIndex++) {
           if (this->supplyNodeIndex == DataPlant::PlantLoop(plantIndex).LoopSide(2).NodeNumOut) {
               this->plantLoopIndex = plantIndex;
               this->plantSetpointNodeIndex = DataPlant::PlantLoop(plantIndex).TempSetPointNodeNum;
               fluidIndex = DataPlant::PlantLoop(plantIndex).FluidIndex; **So this is correctly assigned once**
               // now that we've found the plant populated, let's verify that the nodes match
               if (!PlantUtilities::verifyTwoNodeNumsOnSamePlantLoop(this->supplyNodeIndex, this->returnNodeIndex)) {
                   ShowSevereError("Node problem for SetpointManager:ReturnTemperature:ChilledWater.");
                   ShowContinueError("Return and Supply nodes were not found on the same plant loop.  Verify node names.");
                   ShowFatalError("Simulation aborts due to setpoint node problem");
               }
           }
       }
   }

The important thing is that "fluidIndex" is the variable that tells the setpointmanager what the fluid is. It always starts as water and then inside that for loop it is changed to the fluid on the plant. But that loop activates only once, on first iteration. Because after the plandLoopIndex is set to the plantIndex that condition is never entered.

I checked that by puting some "ShowSevereError" inside that loop and it only enters the condition once and only on first iteration, therefore every single call to this subroutine, which occurs every timestep, sets the fluid as water but keeps the plantLoopIndex from the first iteration and it doesn't assignagain the fluidindex that was changed to water.

A simple fix is adding this afterwards: (I did it on my source code for 9.4 and I believe it works fine. The setpoint manager works the same way with water and with glycol it stops giving warnings about the fluid being water and its properties not calculable below 0C, btw that is the warning that led me to this issue, the explanation in the unmethours link):

 if (this->plantLoopIndex != 0) {
     fluidIndex = DataPlant::PlantLoop(this->plantLoopIndex).FluidIndex;
 }

With that it will initialise the plant, then change the fluid (which is redundant) and then if the plant is initialised in every subsequent timestep it will still change the fluidIndex to the correct fluid of the plant. Another possible solution is setting this->plantLoopIndex=0 at the start of the subroutine but that just makes the calculations longer. Maybe using and if condition when assigning to water as a default is even better in computation speed.

Details

Some additional details for this issue (if relevant):

Platform (Operating system, version): Win10
Version of EnergyPlus: 9.4
Unmethours link or helpdesk ticket number: https://unmethours.com/question/99091/setpointmanagerreturntemperaturechilledhotwater-never-reads-plantloop-fluid-glycol-e-bugfix/

Checklist

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

[x] Defect file added (Setpointmanager.cc)
[ ] Ticket added to Pivotal for defect (development team task)
[ ] Pull request created (the pull request will have additional tasks related to reviewing changes that fix this defect)

jmarrec commented 5 months ago

Could you provide an IDF to reproduce please @PMP-Rsch? If you can't that's not too big of an issue. Just trying to demonstrate that the future fix actually works.

I've looked into the current source code, and thanks to your detailed explanation, I do confirm this is still present.

PMP-Rsch commented 5 months ago

Yes I can. I just tested this from scratch right now by using design builder to make a .idf with a watertowater heatpump working with etylenglycol at 25% mass fraction (It was done hastily so it has tons of warnings and some errors, but unrelated to the bug). Then I changed the SetpointManager:Scheduled to SetpointManager:ReturnTemperature:ChilledWater. And it can be seen in the error file that only after changing to the return temperature setpoint this warning appears in the .err file:

Warning GetSpecificHeatGlycol: Temperature is out of range (too low) for fluid [WATER] specific heat supplied values ~~~ ..Called From:ReturnWaterChWSetPointManager::calculate,Temperature=[-4.75], supplied data range=[0.00,125.00] ~~~ ** Environment=UNTITLED (01-06:30-06), at Simulation time=06/02 09:00 - 09:30

This is really revealing as the other warnings related to temperature address the fluid as "CIRCUITO DE AGUA FRíA GLYCOL CONCENTRATION", making it even clearer that only the return temp. setpoint thinks its water.

So use this IDFs as testing if possible: https://drive.google.com/drive/folders/1pjmz0HYNYiaFHgdTXno7KeQ37dH9lazh?usp=sharing

jmarrec commented 5 months ago

@PMP-Rsch I will fix the condition that leads to the warnings.

But I realized while doing so that you can just plainly ignore them. That cp value which is calculated is never actually used and will not affect the results in any shape or form.

We calculate Qdemand = mdot * cp * deltaT

We check if Qdemand is < 0, but we can do that only from deltaT.

Then we use it as Qdemand / (mdot * cp). so that's the same as just using deltaT only

cf https://github.com/NREL/EnergyPlus/commit/cbfb14138d8ffef612594cf9d77fbd3e1fc09c7f

NREL / EnergyPlus