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

Wrap AirConditioner:VariableRefrigerantFlow:FluidTemperatureControl and AirConditioner:VariableRefrigerantFlow:FluidTemperatureControl:HR #4778

Merged
merged 100 commits into from Feb 14, 2023

Conversation

joseph-robertson
Copy link
Collaborator

@joseph-robertson joseph-robertson commented Dec 21, 2022

Pull request overview

Pull Request Author

Labels:

  • If change to an IDD file, add the label IDDChange
  • If breaking existing API, add the label APIChange
  • If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • Code Style, strip trailing whitespace, etc.
  • All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
  • Labeling is ok
  • 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

@joseph-robertson joseph-robertson added Enhancement Request component - HVAC component - Model Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels Dec 21, 2022
@joseph-robertson joseph-robertson self-assigned this Dec 21, 2022
@jmarrec
Copy link
Collaborator

jmarrec commented Feb 6, 2023

@joseph-robertson reported the new AirConditionerVRF objects do not have a name()/setName() method, and I realized they also don't serialize in ruby... something's wrong with the SWIG stuff

$ Products/openstudio -e 'm = OpenStudio::Model::Model.new; vrf = OpenStudio::Model::AirConditionerVariableRefrigerantFlowFluidTemperatureControl.new(m); puts vrf'
#<OpenStudio::Model::AirConditionerVariableRefrigerantFlowFluidTemperatureControl:0x000056251ac3e8b8>

Edit: fixed in ff3124f , the order matters

@jmarrec
Copy link
Collaborator

jmarrec commented Feb 10, 2023

A few things are missing here I think:

  • A ZoneHVACTerminalUnitVariableRefrigerantFlow should enforce using the same coil types (FluidTemp Control Or not) for both HC and CC
  • The AirCondVRFs types should only accept the right types of ZoneHVACTUs
  • The Fan type should match as well

I will make the changes (I have them partially done, but failing tests)

…ls, and AirCOnd and ZoneHVACTU. + Add a Ctor for convenience for ZOneHVACTU that can create a FluidTempCtrl one
Copy link
Collaborator

@jmarrec jmarrec left a comment

Choose a reason for hiding this comment

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

@joseph-robertson can you review my changes quickly please?

A2 , \field Name
\required-field
\type alpha
\reference ConnectionObject
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it's a ConnectionObject since it doesn't actually have any nodes. In which case it's actually a ParentObject and not an HVACComponent... That feels kinda weird though, I need to think this through a bit more...

@@ -54,11 +56,16 @@ namespace model {
{

public:
explicit ZoneHVACTerminalUnitVariableRefrigerantFlow(const Model& model);
explicit ZoneHVACTerminalUnitVariableRefrigerantFlow(const Model& model, bool isFluidTemperatureControl = false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

New convenience ctor. When isFLuidTemperature Control, it uses CoilHeating/CoolingDXVariableRefrigerantFluidTemperatureControl coils + a FanSystemModel

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wait, we want this to be "+ a FanVariableVolume", no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I checked the idd, and you're right. It can be FanSystemModel or FanVariableVolume.

Comment on lines +163 to +164
// Returns true if the Cooling and Heating Coils are of the FluidTemperatureControl type
bool isFluidTemperatureControl() const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

New

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea.

Comment on lines +423 to +440
auto fanType = component.iddObjectType();
if (isFluidTemperatureControl()) {
if ((fanType != IddObjectType::OS_Fan_SystemModel) && (fanType != IddObjectType::OS_Fan_VariableVolume)) {
LOG(Warn, "For " << briefDescription()
<< ", since it is a FluidTemperatureControl unit, fan type must be FanSystemModel or FanVariableVolume, not "
<< component.briefDescription());
return false;
}
} else {
if ((fanType != IddObjectType::OS_Fan_SystemModel) && (fanType != IddObjectType::OS_Fan_OnOff)
&& (fanType != IddObjectType::OS_Fan_ConstantVolume)) {
LOG(Warn,
"For " << briefDescription()
<< ", since it is a non-FluidTemperatureControl unit, fan type must be FanSystemModel, FanOnOff, or FanConstantVolume, not "
<< component.briefDescription());
return false;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

supplyAirFan type is also enforced.

Comment on lines +848 to +872
if (isFluidTemperatureControl) {
CoilCoolingDXVariableRefrigerantFlowFluidTemperatureControl coolingCoil(model);
coolingCoil.setName(name().get() + " Cooling Coil");
getImpl<detail::ZoneHVACTerminalUnitVariableRefrigerantFlow_Impl>()->setCoolingCoil(coolingCoil);

CoilHeatingDXVariableRefrigerantFlowFluidTemperatureControl heatingCoil(model);
heatingCoil.setName(name().get() + " Heating Coil");
getImpl<detail::ZoneHVACTerminalUnitVariableRefrigerantFlow_Impl>()->setHeatingCoil(heatingCoil);

CoilHeatingDXVariableRefrigerantFlow heatingCoil(model);
heatingCoil.setName(name().get() + " Heating Coil");
getImpl<detail::ZoneHVACTerminalUnitVariableRefrigerantFlow_Impl>()->setHeatingCoil(heatingCoil);
FanSystemModel fan(model);
fan.setName(name().get() + " Fan");
getImpl<detail::ZoneHVACTerminalUnitVariableRefrigerantFlow_Impl>()->setSupplyAirFan(fan);
} else {
CoilCoolingDXVariableRefrigerantFlow coolingCoil(model);
coolingCoil.setName(name().get() + " Cooling Coil");
getImpl<detail::ZoneHVACTerminalUnitVariableRefrigerantFlow_Impl>()->setCoolingCoil(coolingCoil);

FanOnOff fan(model, alwaysOnSchedule);
fan.setName(name().get() + " Fan");
getImpl<detail::ZoneHVACTerminalUnitVariableRefrigerantFlow_Impl>()->setSupplyAirFan(fan);
CoilHeatingDXVariableRefrigerantFlow heatingCoil(model);
heatingCoil.setName(name().get() + " Heating Coil");
getImpl<detail::ZoneHVACTerminalUnitVariableRefrigerantFlow_Impl>()->setHeatingCoil(heatingCoil);

FanOnOff fan(model, alwaysOnSchedule);
fan.setName(name().get() + " Fan");
getImpl<detail::ZoneHVACTerminalUnitVariableRefrigerantFlow_Impl>()->setSupplyAirFan(fan);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Convenience ctor logic.

Comment on lines +1482 to +1487
bool AirConditionerVariableRefrigerantFlow_Impl::addTerminal(ZoneHVACTerminalUnitVariableRefrigerantFlow& vrf) {
if (vrf.isFluidTemperatureControl()) {
LOG(Warn, "For " << briefDescription() << ", cannot add a terminal that uses FluidTemperatureControl coils: " << vrf.briefDescription());
return false;
}
return vrfModelObjectList().addModelObject(vrf);
Copy link
Collaborator

Choose a reason for hiding this comment

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

AirConditionerVariableRefrigerantFlow only accepts Non-FluidCtrl terminals

Comment on lines +886 to +892
bool AirConditionerVariableRefrigerantFlowFluidTemperatureControl_Impl::addTerminal(ZoneHVACTerminalUnitVariableRefrigerantFlow& vrf) {
if (!vrf.isFluidTemperatureControl()) {
LOG(Warn, "For " << briefDescription() << ", cannot add a terminal that uses non-FluidTemperatureControl coils: " << vrf.briefDescription());
return false;
}
return vrfModelObjectList().addModelObject(vrf);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

AirConditionerVariableRefrigerantFlowFluidTemperatureControl only accepts FluidCtrl terminals

Comment on lines +1156 to +1162
bool AirConditionerVariableRefrigerantFlowFluidTemperatureControlHR_Impl::addTerminal(ZoneHVACTerminalUnitVariableRefrigerantFlow& vrf) {
if (!vrf.isFluidTemperatureControl()) {
LOG(Warn, "For " << briefDescription() << ", cannot add a terminal that uses non-FluidTemperatureControl coils: " << vrf.briefDescription());
return false;
}
return vrfModelObjectList().addModelObject(vrf);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

AirConditionerVariableRefrigerantFlowFluidTemperatureControlHR only accepts FluidCtrl terminals

@@ -477,7 +477,7 @@ namespace model {

bool setHeatRecoveryHeatingEnergyTimeConstant(double heatRecoveryHeatingEnergyTimeConstant);

void addTerminal(ZoneHVACTerminalUnitVariableRefrigerantFlow& vrf);
bool addTerminal(ZoneHVACTerminalUnitVariableRefrigerantFlow& vrf);
Copy link
Collaborator

Choose a reason for hiding this comment

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

addTerminal returns a bool now. True for all three types

@jmarrec jmarrec merged commit 9f2a92a into develop Feb 14, 2023
@jmarrec jmarrec deleted the vrf-temp-ctrl branch February 14, 2023 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APIChange A motivated non-backward compatible change that breaks the existing API and needs to be communicated component - HVAC component - Model Enhancement Request Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge.
Projects
None yet
3 participants