Skip to content
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

Fix VRF ThermostatOffsetPriority control method #5537

Conversation

Projects
None yet
6 participants
@rraustad
Copy link
Contributor

commented Mar 9, 2016

Pull request overview

Addresses #5536
Fixed AirConditioner:VariableRefrigerantFlow to prevent crash with ThermostatOffsetPriority control method.

Work Checklist

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

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • At least one of the following appropriate labels must be added to this PR to be consumed into the changelog:
    • Defect: This pull request repairs a github defect issue. The github issue should be referenced in the PR description
    • Refactoring: This pull request includes code changes that don't change the functionality of the program, just perform refactoring
    • NewFeature: This pull request includes code to add a new feature to EnergyPlus
    • Performance: This pull request includes code changes that are directed at improving the runtime performance of EnergyPlus
    • DoNoPublish: This pull request includes changes that shouldn't be included in the changelog

Review Checklist

This will not be exhaustively relevant to every PR.

  • Code style (parentheses padding, variable names)
  • Functional code review (it has to work!)
  • If defect, results of running current develop vs this branch should exhibit the fix
  • CI status: all green or justified
  • Performance: CI Linux results include performance check -- verify this
  • Unit Test(s)
  • C++ checks:
    • Argument types
    • If any virtual classes, ensure virtual destructor included, other things
  • IDD changes: N/A
@nrel-bot-2

This comment has been minimized.

Copy link

commented on 9cfd651 Mar 9, 2016

#5536-AirConditionerVariableRefrigerantFlow-with-ThermostatOffsetPriority-crashes-with-blank-Zone-Name-for-Master-Thermostat-Location (rraustad) - x86_64-Linux-Ubuntu-14.04-cppcheck-1.61: OK (0 of 0 tests passed, 0 test warnings)

Build Badge

This comment has been minimized.

Copy link

replied Mar 9, 2016

#5536-AirConditionerVariableRefrigerantFlow-with-ThermostatOffsetPriority-crashes-with-blank-Zone-Name-for-Master-Thermostat-Location (rraustad) - x86_64-Linux-Ubuntu-14.04-gcc-4.8: OK (2167 of 2167 tests passed, 0 test warnings)

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 9, 2016

#5536-AirConditionerVariableRefrigerantFlow-with-ThermostatOffsetPriority-crashes-with-blank-Zone-Name-for-Master-Thermostat-Location (rraustad) - x86_64-Linux-Ubuntu-14.04-custom_check: OK (0 of 0 tests passed, 0 test warnings)

Build Badge

This comment has been minimized.

Copy link

replied Mar 9, 2016

#5536-AirConditionerVariableRefrigerantFlow-with-ThermostatOffsetPriority-crashes-with-blank-Zone-Name-for-Master-Thermostat-Location (rraustad) - x86_64-MacOS-10.9-clang: OK (2129 of 2129 tests passed, 0 test warnings)

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 9, 2016

#5536-AirConditionerVariableRefrigerantFlow-with-ThermostatOffsetPriority-crashes-with-blank-Zone-Name-for-Master-Thermostat-Location (rraustad) - x86_64-Linux-Ubuntu-14.04-gcc-4.8-UnitTestsCoverage-Debug: OK (1021 of 1021 tests passed, 0 test warnings)

Build Badge Test Badge Coverage Badge

This comment has been minimized.

Copy link

replied Mar 9, 2016

#5536-AirConditionerVariableRefrigerantFlow-with-ThermostatOffsetPriority-crashes-with-blank-Zone-Name-for-Master-Thermostat-Location (rraustad) - x86_64-Linux-Ubuntu-14.04-gcc-4.8-IntegrationCoverage-Debug: OK (1579 of 1579 tests passed, 0 test warnings)

Build Badge Test Badge Coverage Badge

This comment has been minimized.

Copy link

replied Mar 9, 2016

#5536-AirConditionerVariableRefrigerantFlow-with-ThermostatOffsetPriority-crashes-with-blank-Zone-Name-for-Master-Thermostat-Location (rraustad) - i386-Windows-7-VisualStudio-12: OK (2135 of 2135 tests passed, 0 test warnings)

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 9, 2016

#5536-AirConditionerVariableRefrigerantFlow-with-ThermostatOffsetPriority-crashes-with-blank-Zone-Name-for-Master-Thermostat-Location (rraustad) - Win64-Windows-7-VisualStudio-12: OK (2135 of 2135 tests passed, 0 test warnings)

Build Badge Test Badge

} else if ( SELECT_CASE_var == SingleCoolingSetPoint ) {
// if cooling load, ZoneDeltaT will be positive
ZoneDeltaT = max( 0.0, ZT( ThisZoneNum ) - SPTempHi );
MaxDeltaT( VRFCond ) = max( MaxDeltaT( VRFCond ), ZoneDeltaT );

This comment has been minimized.

Copy link
@rraustad

rraustad Mar 9, 2016

Author Contributor

These were backwards. If you look below at line 6478 and 6481, you see that MaxDeltaT > 0 represent cooling and MinDeltaT < 0 represent heating. I fixed the math and used the correct variables.

CoolingLoad( VRFCond ) = true;
} else if ( FanOpMode == ContFanCycCoil && LastModeHeating( VRFCond ) ) {
HeatingLoad( VRFCond ) = true;
CoolingLoad( VRFCond ) = false;

This comment has been minimized.

Copy link
@rraustad

rraustad Mar 9, 2016

Author Contributor

OK, now here. I can see the intent here with this code, albeit wrong as implemented. These ELSE IFs looked to the fan operating mode schedule to enable cooling/heating if MaxDeltaT/MinDeltaT were 0. For example, if the previous time step met the load, then it is possible that the zone temp = SP. If VRF TUs use constant fan mode, that would pose a load on the zone and this code tried to set operating mode based on the last operating mode. The problem here was that it looked to the master thermostat location to make that determination. That was incorrect in 1) no master thermostat is needed for this control, and 2) the load due to constant fan would be zone specific. IF this were to be done, it would need to be done up at line 6320-6330 ish, where we loop through all the terminal units. It's just that I am not sure what that logic would look like. For example, if a zone was at the SP, and used constant fan, and last mode was cooling, what would I do? Add max(MaxDeltaT(), 1) to the cooling var MaxDeltaT. That was the only thing I could think of doing last night IF I were to implement something here. Bottom line is that ThermostatOffsetPriority is meant to set the cooling/heating operating mode based on the worst case zone deviation from SP. I thought that just reverting back to that methodology was sufficient for now.

@mjwitte mjwitte added the Defect label Mar 11, 2016

@mjwitte

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2016

@rraustad There's a build warning.
src/EnergyPlus/HVACVariableRefrigerantFlow.cc:6274:7 unused variable 'FanOpMode' [-Wunused-variable]

@mjwitte

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2016

Tested defect files and they run now. Also made a variation to force the system into a different operating mode (cooling in January, or heating in July) for a day using OtherEquipment in one zone. Works as expected.

@rraustad

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2016

Corrected the build warning for FanOpMode

@mjwitte mjwitte modified the milestone: EnergyPlus 8.5.0 Mar 11, 2016

@nrel-bot-3

This comment has been minimized.

Copy link

commented on 3065c13 Mar 11, 2016

#5536-AirConditionerVariableRefrigerantFlow-with-ThermostatOffsetPriority-crashes-with-blank-Zone-Name-for-Master-Thermostat-Location (rraustad) - x86_64-MacOS-10.9-clang: OK (2129 of 2129 tests passed, 0 test warnings)

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 12, 2016

#5536-AirConditionerVariableRefrigerantFlow-with-ThermostatOffsetPriority-crashes-with-blank-Zone-Name-for-Master-Thermostat-Location (rraustad) - x86_64-Linux-Ubuntu-14.04-cppcheck-1.61: OK (0 of 0 tests passed, 0 test warnings)

Build Badge

This comment has been minimized.

Copy link

replied Mar 12, 2016

#5536-AirConditionerVariableRefrigerantFlow-with-ThermostatOffsetPriority-crashes-with-blank-Zone-Name-for-Master-Thermostat-Location (rraustad) - x86_64-Linux-Ubuntu-14.04-gcc-4.8: OK (2167 of 2167 tests passed, 0 test warnings)

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 12, 2016

#5536-AirConditionerVariableRefrigerantFlow-with-ThermostatOffsetPriority-crashes-with-blank-Zone-Name-for-Master-Thermostat-Location (rraustad) - x86_64-Linux-Ubuntu-14.04-gcc-4.8-UnitTestsCoverage-Debug: OK (1021 of 1021 tests passed, 0 test warnings)

Build Badge Test Badge Coverage Badge

This comment has been minimized.

Copy link

replied Mar 12, 2016

#5536-AirConditionerVariableRefrigerantFlow-with-ThermostatOffsetPriority-crashes-with-blank-Zone-Name-for-Master-Thermostat-Location (rraustad) - x86_64-Linux-Ubuntu-14.04-custom_check: OK (0 of 0 tests passed, 0 test warnings)

Build Badge

This comment has been minimized.

Copy link

replied Mar 12, 2016

#5536-AirConditionerVariableRefrigerantFlow-with-ThermostatOffsetPriority-crashes-with-blank-Zone-Name-for-Master-Thermostat-Location (rraustad) - x86_64-Linux-Ubuntu-14.04-gcc-4.8-IntegrationCoverage-Debug: OK (1579 of 1579 tests passed, 0 test warnings)

Build Badge Test Badge Coverage Badge

This comment has been minimized.

Copy link

replied Mar 12, 2016

#5536-AirConditionerVariableRefrigerantFlow-with-ThermostatOffsetPriority-crashes-with-blank-Zone-Name-for-Master-Thermostat-Location (rraustad) - i386-Windows-7-VisualStudio-12: OK (2135 of 2135 tests passed, 0 test warnings)

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 12, 2016

#5536-AirConditionerVariableRefrigerantFlow-with-ThermostatOffsetPriority-crashes-with-blank-Zone-Name-for-Master-Thermostat-Location (rraustad) - Win64-Windows-7-VisualStudio-12: OK (2135 of 2135 tests passed, 0 test warnings)

Build Badge Test Badge

mjwitte added a commit that referenced this pull request Mar 12, 2016

Merge pull request #5537 from NREL/#5536-AirConditionerVariableRefrig…
…erantFlow-with-ThermostatOffsetPriority-crashes-with-blank-Zone-Name-for-Master-Thermostat-Location

Fix VRF ThermostatOffsetPriority control method

@mjwitte mjwitte merged commit 424cbc1 into develop Mar 12, 2016

8 checks passed

Win64-Windows-7-VisualStudio-12 OK (2135 of 2135 tests passed, 0 test warnings)
Details
i386-Windows-7-VisualStudio-12 OK (2135 of 2135 tests passed, 0 test warnings)
Details
x86_64-Linux-Ubuntu-14.04-cppcheck-1.61 OK (0 of 0 tests passed, 0 test warnings)
Details
x86_64-Linux-Ubuntu-14.04-custom_check OK (0 of 0 tests passed, 0 test warnings)
Details
x86_64-Linux-Ubuntu-14.04-gcc-4.8 OK (2167 of 2167 tests passed, 0 test warnings)
Details
x86_64-Linux-Ubuntu-14.04-gcc-4.8-IntegrationCoverage-Debug OK (1579 of 1579 tests passed, 0 test warnings)
Details
x86_64-Linux-Ubuntu-14.04-gcc-4.8-UnitTestsCoverage-Debug OK (1021 of 1021 tests passed, 0 test warnings)
Details
x86_64-MacOS-10.9-clang OK (2129 of 2129 tests passed, 0 test warnings)
Details

@mjwitte mjwitte deleted the #5536-AirConditionerVariableRefrigerantFlow-with-ThermostatOffsetPriority-crashes-with-blank-Zone-Name-for-Master-Thermostat-Location branch Mar 12, 2016

@Myoldmopar

This comment has been minimized.

Copy link
Member

commented on 1131b78 Mar 16, 2016

These changes have been ported to the LaTeX in preparation for merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.