From bc27fb0e18f7e5bf52e906801309053ab9bb76f9 Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Tue, 5 Mar 2019 14:22:54 +0100 Subject: [PATCH 1/4] Extend unit test for #3430: AirTerminalSingleDuctConstantVolumeFourPipeBeam::remove() --- ...gleDuctConstantVolumeFourPipeInduction.cpp | 4 +- ...leDuctConstantVolumeFourPipeBeam_GTest.cpp | 42 +++++++++++++++++-- 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/openstudiocore/src/model/AirTerminalSingleDuctConstantVolumeFourPipeInduction.cpp b/openstudiocore/src/model/AirTerminalSingleDuctConstantVolumeFourPipeInduction.cpp index 37acda766ce..1f60ce85b15 100644 --- a/openstudiocore/src/model/AirTerminalSingleDuctConstantVolumeFourPipeInduction.cpp +++ b/openstudiocore/src/model/AirTerminalSingleDuctConstantVolumeFourPipeInduction.cpp @@ -257,8 +257,8 @@ namespace detail { } } - model().disconnect(getObject(),inletPort()); - model().disconnect(getObject(),outletPort()); + model().disconnect(thisObject, inletPort()); + model().disconnect(thisObject, outletPort()); if( _coolingCoil && ( loop = _coolingCoil->plantLoop() ) ) { diff --git a/openstudiocore/src/model/test/AirTerminalSingleDuctConstantVolumeFourPipeBeam_GTest.cpp b/openstudiocore/src/model/test/AirTerminalSingleDuctConstantVolumeFourPipeBeam_GTest.cpp index 8d278cff6a8..325bcd681f3 100644 --- a/openstudiocore/src/model/test/AirTerminalSingleDuctConstantVolumeFourPipeBeam_GTest.cpp +++ b/openstudiocore/src/model/test/AirTerminalSingleDuctConstantVolumeFourPipeBeam_GTest.cpp @@ -40,6 +40,7 @@ #include "../AirLoopHVAC.hpp" #include "../ScheduleConstant.hpp" #include "../Node.hpp" +#include "../ThermalZone.hpp" // Needed for getConcreteModelObjects #include "../CoilCoolingFourPipeBeam_Impl.hpp" @@ -145,10 +146,27 @@ TEST_F(ModelFixture, AirTerminalSingleDuctConstantVolumeFourPipeBeam_CloneAndRem { Model m; + ThermalZone z(m); + AirLoopHVAC a(m); + PlantLoop hw_p(m); + PlantLoop chw_p(m); + CoilCoolingFourPipeBeam cc(m); CoilHeatingFourPipeBeam hc(m); AirTerminalSingleDuctConstantVolumeFourPipeBeam atu(m, cc, hc); + EXPECT_TRUE(a.addBranchForZone(z, atu)); + EXPECT_TRUE(chw_p.addDemandBranchForComponent(cc)); + EXPECT_TRUE( hw_p.addDemandBranchForComponent(hc)); + + EXPECT_EQ(1u, z.equipment().size()); + EXPECT_EQ(1u, z.equipmentInHeatingOrder().size()); // Would crash before remove() got implemented + // 7u = Node, Splitter, One branch with Node - Coil - Node, Mixer, Node + EXPECT_EQ(7u, chw_p.demandComponents().size()); + EXPECT_EQ(7u, hw_p.demandComponents().size()); + + + // Now clone AirTerminalSingleDuctConstantVolumeFourPipeBeam atuClone = atu.clone(m).cast(); // I expect the cooling/heating coils to have been cloned too @@ -156,16 +174,34 @@ TEST_F(ModelFixture, AirTerminalSingleDuctConstantVolumeFourPipeBeam_CloneAndRem EXPECT_EQ(2u, m.getConcreteModelObjects().size()); EXPECT_EQ(2u, m.getConcreteModelObjects().size()); + // And they should have been reassigned to the clone too - ASSERT_TRUE(atuClone.coolingCoil()); - ASSERT_TRUE(atuClone.heatingCoil()); + boost::optional _ccClone = atuClone.coolingCoil(); + boost::optional _hcClone = atuClone.heatingCoil(); + ASSERT_TRUE(_ccClone); + ASSERT_TRUE(_hcClone); + EXPECT_NE(_ccClone->handle(), cc.handle()); + EXPECT_NE(_hcClone->handle(), hc.handle()); + + + // Shouldn't have been added to the Zone though + EXPECT_EQ(1u, z.equipment().size()); + EXPECT_EQ(1u, z.equipmentInHeatingOrder().size()); // Would crash before remove() got implemented + + // And coils shouldn't have been connected to the PlantLoops either (to match other objects) + // 7u = Node, Splitter, One branch with Node - Coil - Node, Mixer, Node + EXPECT_EQ(7u, chw_p.demandComponents().size()); + EXPECT_EQ(7u, hw_p.demandComponents().size()); + // test remove method - atuClone.remove(); + atu.remove(); EXPECT_EQ(1u, m.getConcreteModelObjects().size()); EXPECT_EQ(1u, m.getConcreteModelObjects().size()); EXPECT_EQ(1u, m.getConcreteModelObjects().size()); + EXPECT_EQ(0u, z.equipment().size()); + EXPECT_EQ(0u, z.equipmentInHeatingOrder().size()); // Would crash before remove() got implemented } From 34c8db05deed8a0fa1ebf62d96306382181fe6ed Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Tue, 5 Mar 2019 16:01:37 +0100 Subject: [PATCH 2/4] Add test for addBranchForZone to ensure ATUFourPipeBeam coils are reconnected to same plantloop (OS App in particular) Typo --- ...leDuctConstantVolumeFourPipeBeam_GTest.cpp | 73 ++++++++++++++++++- 1 file changed, 71 insertions(+), 2 deletions(-) diff --git a/openstudiocore/src/model/test/AirTerminalSingleDuctConstantVolumeFourPipeBeam_GTest.cpp b/openstudiocore/src/model/test/AirTerminalSingleDuctConstantVolumeFourPipeBeam_GTest.cpp index 325bcd681f3..ca532d0a18b 100644 --- a/openstudiocore/src/model/test/AirTerminalSingleDuctConstantVolumeFourPipeBeam_GTest.cpp +++ b/openstudiocore/src/model/test/AirTerminalSingleDuctConstantVolumeFourPipeBeam_GTest.cpp @@ -189,7 +189,7 @@ TEST_F(ModelFixture, AirTerminalSingleDuctConstantVolumeFourPipeBeam_CloneAndRem EXPECT_EQ(1u, z.equipmentInHeatingOrder().size()); // Would crash before remove() got implemented // And coils shouldn't have been connected to the PlantLoops either (to match other objects) - // 7u = Node, Splitter, One branch with Node - Coil - Node, Mixer, Node + // It's the AirLoopHVAC::addBranchForZone that will clone the last terminal used, and reconnect it like so EXPECT_EQ(7u, chw_p.demandComponents().size()); EXPECT_EQ(7u, hw_p.demandComponents().size()); @@ -202,6 +202,75 @@ TEST_F(ModelFixture, AirTerminalSingleDuctConstantVolumeFourPipeBeam_CloneAndRem EXPECT_EQ(0u, z.equipment().size()); EXPECT_EQ(0u, z.equipmentInHeatingOrder().size()); // Would crash before remove() got implemented - + // 5u = node, splitter, branch with drop node, mixer, node + EXPECT_EQ(5u, chw_p.demandComponents().size()); + EXPECT_EQ(5u, hw_p.demandComponents().size()); } + +TEST_F(ModelFixture, AirTerminalSingleDuctConstantVolumeFourPipeBeam_addBranchForZone) +{ + + Model m; + ThermalZone z(m); + ThermalZone z2(m); + + AirLoopHVAC a(m); + PlantLoop hw_p(m); + PlantLoop chw_p(m); + + CoilCoolingFourPipeBeam cc(m); + CoilHeatingFourPipeBeam hc(m); + AirTerminalSingleDuctConstantVolumeFourPipeBeam atu(m, cc, hc); + + EXPECT_TRUE(a.addBranchForZone(z, atu)); + EXPECT_TRUE(chw_p.addDemandBranchForComponent(cc)); + EXPECT_TRUE( hw_p.addDemandBranchForComponent(hc)); + + ASSERT_TRUE(atu.chilledWaterPlantLoop()); + EXPECT_EQ(chw_p.handle(), atu.chilledWaterPlantLoop()->handle()); + ASSERT_TRUE(atu.hotWaterPlantLoop()); + EXPECT_EQ(hw_p.handle(), atu.hotWaterPlantLoop()->handle()); + + ASSERT_EQ(1u, z.equipment().size()); + ASSERT_EQ(0u, z2.equipment().size()); + EXPECT_EQ(1u, m.getConcreteModelObjects().size()); + EXPECT_EQ(1u, m.getConcreteModelObjects().size()); + EXPECT_EQ(1u, m.getConcreteModelObjects().size()); + + // addBranchForZone(ThermalZone&) is the one used in OS App, and will clone the last terminal found + // **AND** reconnect the coils like the original + + EXPECT_TRUE(a.addBranchForZone(z2)); + + ASSERT_EQ(1u, z.equipment().size()); + ASSERT_EQ(1u, z2.equipment().size()); + + // I expect the cooling/heating coils to have been cloned too + EXPECT_EQ(2u, m.getConcreteModelObjects().size()); + EXPECT_EQ(2u, m.getConcreteModelObjects().size()); + EXPECT_EQ(2u, m.getConcreteModelObjects().size()); + + boost::optional _atuClone = z2.equipment()[0].optionalCast(); + ASSERT_TRUE(_atuClone); + + // Coils should have been cloned and reassigned to the clone too + boost::optional _ccClone = _atuClone->coolingCoil(); + boost::optional _hcClone = _atuClone->heatingCoil(); + ASSERT_TRUE(_ccClone); + ASSERT_TRUE(_hcClone); + EXPECT_NE(_ccClone->handle(), cc.handle()); + EXPECT_NE(_hcClone->handle(), hc.handle()); + + // They should have been reconnected like original one + ASSERT_TRUE(_atuClone->chilledWaterPlantLoop()); + EXPECT_EQ(chw_p.handle(), _atuClone->chilledWaterPlantLoop()->handle()); + ASSERT_TRUE(_atuClone->hotWaterPlantLoop()); + EXPECT_EQ(hw_p.handle(), _atuClone->hotWaterPlantLoop()->handle()); + + // The original ones should be untouched too + ASSERT_TRUE(atu.chilledWaterPlantLoop()); + EXPECT_EQ(chw_p.handle(), atu.chilledWaterPlantLoop()->handle()); + ASSERT_TRUE(atu.hotWaterPlantLoop()); + EXPECT_EQ(hw_p.handle(), atu.hotWaterPlantLoop()->handle()); +} From 281a64895096c365bd4e5a8b867ca1bca5088923 Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Tue, 5 Mar 2019 14:31:42 +0100 Subject: [PATCH 3/4] Fix #3430: add AirTerminalSingleDuctConstantVolumeFourPipeBeam::remove() method --- ...alSingleDuctConstantVolumeFourPipeBeam.cpp | 32 ++++++++++++------- ...gleDuctConstantVolumeFourPipeBeam_Impl.hpp | 2 +- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/openstudiocore/src/model/AirTerminalSingleDuctConstantVolumeFourPipeBeam.cpp b/openstudiocore/src/model/AirTerminalSingleDuctConstantVolumeFourPipeBeam.cpp index b20d1e20d91..5ebff2850c4 100644 --- a/openstudiocore/src/model/AirTerminalSingleDuctConstantVolumeFourPipeBeam.cpp +++ b/openstudiocore/src/model/AirTerminalSingleDuctConstantVolumeFourPipeBeam.cpp @@ -139,14 +139,14 @@ namespace detail { return result; } - // TODO -/* std::vector AirTerminalSingleDuctConstantVolumeFourPipeBeam_Impl::remove() { Model _model = this->model(); ModelObject thisObject = this->getObject(); - HVACComponent _coolingCoil = coilCoolingCooledBeam(); + boost::optional loop; + boost::optional _coolingCoil = this->coolingCoil(); + boost::optional _heatingCoil = this->heatingCoil(); boost::optional sourceModelObject = this->inletModelObject(); boost::optional sourcePort = this->connectedObjectPort(this->inletPort()); @@ -154,6 +154,7 @@ namespace detail { boost::optional targetModelObject = this->outletModelObject(); boost::optional targetPort = this->connectedObjectPort(this->outletPort()); + // Remove from any ZoneHVACEquipmentList std::vector thermalZones = _model.getConcreteModelObjects(); for( auto & thermalZone : thermalZones ) { @@ -162,7 +163,6 @@ namespace detail { if( std::find(equipment.begin(),equipment.end(),thisObject) != equipment.end() ) { thermalZone.removeEquipment(thisObject); - break; } } @@ -184,9 +184,14 @@ namespace detail { inletNode->disconnect(); inletNode->remove(); - if( boost::optional loop = _coolingCoil.plantLoop() ) + if( _coolingCoil && ( loop = _coolingCoil->plantLoop() ) ) + { + loop->removeDemandBranchWithComponent(*_coolingCoil); + } + + if( _heatingCoil && ( loop = _heatingCoil->plantLoop() ) ) { - loop->removeDemandBranchWithComponent(_coolingCoil); + loop->removeDemandBranchWithComponent(*_heatingCoil); } return StraightComponent_Impl::remove(); @@ -195,16 +200,21 @@ namespace detail { } } - model().disconnect(getObject(),inletPort()); - model().disconnect(getObject(),outletPort()); + model().disconnect(thisObject, inletPort()); + model().disconnect(thisObject, outletPort()); - if( boost::optional loop = _coolingCoil.plantLoop() ) + if ( _coolingCoil && ( loop = _coolingCoil->plantLoop() ) ) { - loop->removeDemandBranchWithComponent(_coolingCoil); + loop->removeDemandBranchWithComponent(*_coolingCoil); + } + + if ( _heatingCoil && ( loop = _heatingCoil->plantLoop() ) ) + { + loop->removeDemandBranchWithComponent(*_heatingCoil); } return StraightComponent_Impl::remove(); - } */ + } bool AirTerminalSingleDuctConstantVolumeFourPipeBeam_Impl::isRemovable() const { diff --git a/openstudiocore/src/model/AirTerminalSingleDuctConstantVolumeFourPipeBeam_Impl.hpp b/openstudiocore/src/model/AirTerminalSingleDuctConstantVolumeFourPipeBeam_Impl.hpp index b08623b843a..320a7a012ed 100644 --- a/openstudiocore/src/model/AirTerminalSingleDuctConstantVolumeFourPipeBeam_Impl.hpp +++ b/openstudiocore/src/model/AirTerminalSingleDuctConstantVolumeFourPipeBeam_Impl.hpp @@ -84,7 +84,7 @@ namespace detail { virtual ModelObject clone(Model model) const override; - // TODO: virtual std::vector remove() override; + virtual std::vector remove() override; virtual bool isRemovable() const override; From 615de272fb7118579469e67c6936ee30fd7cdbdc Mon Sep 17 00:00:00 2001 From: Julien Marrec Date: Tue, 5 Mar 2019 16:39:07 +0100 Subject: [PATCH 4/4] Modify addBranchForZone(ThermalZone&) to reconnect coils --- openstudiocore/src/model/AirLoopHVAC.cpp | 36 +++++++++++++++++-- openstudiocore/src/model/AirLoopHVAC_Impl.hpp | 1 + 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/openstudiocore/src/model/AirLoopHVAC.cpp b/openstudiocore/src/model/AirLoopHVAC.cpp index 188cea7324c..35353d62ac7 100644 --- a/openstudiocore/src/model/AirLoopHVAC.cpp +++ b/openstudiocore/src/model/AirLoopHVAC.cpp @@ -74,6 +74,8 @@ #include "AirTerminalSingleDuctConstantVolumeCooledBeam_Impl.hpp" #include "AirTerminalSingleDuctConstantVolumeFourPipeInduction.hpp" #include "AirTerminalSingleDuctConstantVolumeFourPipeInduction_Impl.hpp" +#include "AirTerminalSingleDuctConstantVolumeFourPipeBeam.hpp" +#include "AirTerminalSingleDuctConstantVolumeFourPipeBeam_Impl.hpp" #include "AvailabilityManagerAssignmentList.hpp" #include "AvailabilityManagerAssignmentList_Impl.hpp" #include "AvailabilityManager.hpp" @@ -407,7 +409,7 @@ namespace detail { // Reconnect the cloned terminal to the plant loop(s) - // TODO: (Temporary?) Ugly hack for FourPipeInduction for now, which has both a cooling and heating plantLoop + // Special case for FourPipeInduction for now, which has both a cooling and heating plantLoop if ( lastAirTerminal->iddObjectType() == IddObjectType::OS_AirTerminal_SingleDuct_ConstantVolume_FourPipeInduction ) { // Safe to directly cast @@ -421,14 +423,41 @@ namespace detail { // If the original ATU has a cooling coil, if it's a CoilCoolingWater, and the cooling coil has a plantLoop, reconnect it here if (lastAtuFourPipe.coolingCoil()) { - if (boost::optional _lastCC = lastAtuFourPipe.coolingCoil()->cast() ) { + if (boost::optional _lastCC = lastAtuFourPipe.coolingCoil()->optionalCast() ) { if (boost::optional _coolingPl = _lastCC->plantLoop()) { _coolingPl->addDemandBranchForComponent(newAtuFourPipe.coolingCoil().get()); } } } - // TODO: Another ugly hack for CooledBeam, which isn't a HVAComponent but a StraightComponent + // Special case for FourPipeInduction, which has both optional cooling and heating coils, that can or cannot be linked to some PlantLoops + } else if ( lastAirTerminal->iddObjectType() == IddObjectType::OS_AirTerminal_SingleDuct_ConstantVolume_FourPipeBeam ) { + + // Safe to directly cast (checked iddObjectType already) + AirTerminalSingleDuctConstantVolumeFourPipeBeam lastAtuFourPipeBeam = lastAirTerminal->cast(); + AirTerminalSingleDuctConstantVolumeFourPipeBeam newAtuFourPipeBeam = airTerminal.cast(); + + boost::optional _loop; + + // If the original ATU has a cooling coil, and the cooling coil has a plantLoop, reconnect it here + boost::optional _hc = lastAtuFourPipeBeam.heatingCoil(); + if ( _hc && (_loop = _hc->plantLoop()) ) { + boost::optional _hcClone = newAtuFourPipeBeam.heatingCoil(); + // FourPipeBeam::clone should clone and re-set the coils, so it should have worked + OS_ASSERT(_hcClone); + _loop->addDemandBranchForComponent(*_hcClone); + } + + // If the original ATU has a cooling coil, and the cooling coil has a plantLoop, reconnect it here + boost::optional _cc = lastAtuFourPipeBeam.coolingCoil(); + if ( _cc && (_loop = _cc->plantLoop()) ) { + boost::optional _ccClone = newAtuFourPipeBeam.coolingCoil(); + // FourPipeBeam::clone should clone and re-set the coils, so it should have worked + OS_ASSERT(_ccClone); + _loop->addDemandBranchForComponent(*_ccClone); + } + + // Special case for CooledBeam, which isn't a HVAComponent but a StraightComponent } else if (lastAirTerminal->iddObjectType() == IddObjectType::OS_AirTerminal_SingleDuct_ConstantVolume_CooledBeam) { // Safe to directly cast @@ -992,6 +1021,7 @@ namespace detail { return result; } + // TODO: REMOVE! //bool AirLoopHVAC_Impl::addBranchForZoneImpl(ThermalZone & thermalZone, OptionalStraightComponent & airTerminal) //{ // boost::optional comp; diff --git a/openstudiocore/src/model/AirLoopHVAC_Impl.hpp b/openstudiocore/src/model/AirLoopHVAC_Impl.hpp index 6ce2adc6920..ee76a326a8d 100644 --- a/openstudiocore/src/model/AirLoopHVAC_Impl.hpp +++ b/openstudiocore/src/model/AirLoopHVAC_Impl.hpp @@ -181,6 +181,7 @@ class MODEL_API AirLoopHVAC_Impl : public Loop_Impl { Splitter & splitter, Mixer & mixer); + // TODO: remove? //bool addBranchForZoneImpl(openstudio::model::ThermalZone & thermalZone, // boost::optional & optAirTerminal);