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.

Sign up for GitHub

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

Jump to bottom

Thermal wheels #3554

Open
wants to merge 144 commits into
base: master
Choose a base branch
from
Open

Thermal wheels #3554

wants to merge 144 commits into from

Conversation

JayHuLBL
Copy link
Contributor

@JayHuLBL JayHuLBL commented Oct 9, 2023

This closes #3538.

@mwetter
Copy link
Member

mwetter commented Oct 25, 2023

@JayHuLBL : What are the next steps, and who needs to act on them?

@JayHuLBL
Copy link
Contributor Author

@mwetter The PR will be ready for your review when CI test passed.

One questions however is about the base class Buildings.Fluid.HeatExchangers.AirToAirHeatRecovery.BaseClasses.HeatExchagerWithInputEffectiveness. The class is similar as Buildings.Fluid.MassExchangers.ConstantEffectiveness, with the difference that the effectiveness espS and espL are inputs rather than parameter. Is it a good choice to duplicate the class and change the effectiveness to be inputs (as implemented here.)?
Or making the changes in Buildings.Fluid.MassExchangers.ConstantEffectiveness so that the effectiveness could be conditional inputs. But in this case, the class name ConstantEffectiveness will need to be changed.

@mwetter
Copy link
Member

mwetter commented Oct 26, 2023

I think HeatExchagerWithInputEffectiveness is fine as I don't see an elegant way to change the effectiveness to be a conditional input, and remove the parameter if the input is enabled.

mwetter
mwetter requested changes Oct 26, 2023
View reviewed changes
Copy link
Member

@mwetter mwetter left a comment

Choose a reason for hiding this comment

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

I did a quick review and it is clear that this model is not yet ready for a final review. Please see comments and review first the code; afterwards I am happy to do a last review.

Buildings/Fluid/HeatExchangers/AirToAirHeatRecovery/BaseClasses/Effectiveness.mo Outdated Show resolved Hide resolved
Buildings/Fluid/HeatExchangers/AirToAirHeatRecovery/BaseClasses/Effectiveness.mo Outdated Show resolved Hide resolved
Buildings/Fluid/HeatExchangers/AirToAirHeatRecovery/BaseClasses/Effectiveness.mo Outdated Show resolved Hide resolved
Buildings/Fluid/HeatExchangers/AirToAirHeatRecovery/BaseClasses/Effectiveness.mo Outdated
vRat = v_ave/v_sup_nom,
</pre>
<p>
where <code>v_ave</code> is the average volumetric air flow rate,
Copy link
Member

Choose a reason for hiding this comment

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

See how we otherwise refer to volume or mass flow rates. (Moreover, if you were to use mass flow rate, then the sensor would be simpler, and the result would in my opinion not change much if at all).

Buildings/Fluid/HeatExchangers/AirToAirHeatRecovery/BaseClasses/Effectiveness.mo Outdated Show resolved Hide resolved
...Exchangers/AirToAirHeatRecovery/BaseClasses/Validation/HeatExchagerWithInputEffectiveness.mo Outdated Show resolved Hide resolved
Buildings/Fluid/HeatExchangers/AirToAirHeatRecovery/Examples/BypassDamper.mo Outdated
Comment on lines 70 to 71
m1_flow(start=5),
m2_flow(start=5),
Copy link
Member

Choose a reason for hiding this comment

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

Why are there start attribute set?

Buildings/Fluid/HeatExchangers/AirToAirHeatRecovery/Examples/BypassDamper.mo Outdated
Comment on lines 74 to 75
dp1_nominal=100,
dp2_nominal=100,
Copy link
Member

Choose a reason for hiding this comment

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

Is that realistic for a wheel? I think more common are around 300 Pa.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SenHuang19 In class Examples.WheelWithBypassDamper and Examples.WheelWithVariableSpeed, the dp1_nominal and dp2_nominal are still set to be 100. Is it typical?

Buildings/Fluid/HeatExchangers/AirToAirHeatRecovery/Examples/BypassDamper.mo Outdated
epsL_heat_nominal=0.7,
epsL_heat_partload=0.6) "Wheel"
annotation (Placement(transformation(extent={{6,-4},{26,16}})));
Modelica.Blocks.Sources.Ramp DamPos(
Copy link
Member

Choose a reason for hiding this comment

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

Naming convention.

Buildings/Fluid/HeatExchangers/AirToAirHeatRecovery/Examples/BypassDamper.mo Outdated
Comment on lines 89 to 90
inner Modelica.Fluid.System system
"Ambient environment"
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

@SenHuang19 SenHuang19 mentioned this pull request Jun 22, 2024
Merged
9 tasks
@SenHuang19
Copy link

Updates are made in #3909, which is ready for Jianjun's review.

@SenHuang19 SenHuang19 mentioned this pull request Sep 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SenHuang19 SenHuang19 SenHuang19 left review comments

@mwetter mwetter mwetter requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Air-to-Air Heat recovery
3 participants
@JayHuLBL @mwetter @SenHuang19