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

Issue1926 actuator travel #1933

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

Issue1926 actuator travel #1933

wants to merge 33 commits into from

Conversation

mwetter
Copy link
Contributor

@mwetter mwetter commented Sep 15, 2024

This closes #1926

@mwetter mwetter marked this pull request as ready for review September 18, 2024 14:54
@mwetter
Copy link
Contributor Author

mwetter commented Sep 18, 2024

This is ready for review and merge.
@jelgerjansen : Can you please review.
@nytschgeusen @FWuellhorst : Can you please give your OK within 2 weeks as this is a non-backward compatible change. The conversion script has been updated.

Copy link
Contributor

@jelgerjansen jelgerjansen left a comment

Choose a reason for hiding this comment

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

@mwetter thank you for your nice work! Most of my feedback are minor comments. Two issues that might require some more time:

  • I noticed that the renaming (conversion script) also changed variable names in the revision history, which is unintended. I'm not sure if this can be avoided somehow?
  • The value of Td (in the SlewRateLimiter) is defined as 10/strokeTime, but I would have expected strokeTime in the numerator (both in terms of units and physical meaning). Could you explain how you choose the expression for Td?

IBPSA/Electrical/Transmission/BaseClasses/BaseCable.mo Outdated Show resolved Hide resolved
IBPSA/Fluid/Actuators/BaseClasses/ActuatorSignal.mo Outdated Show resolved Hide resolved
IBPSA/Fluid/Actuators/BaseClasses/ActuatorSignal.mo Outdated Show resolved Hide resolved
IBPSA/Fluid/Actuators/BaseClasses/ActuatorSignal.mo Outdated Show resolved Hide resolved
IBPSA/Fluid/Actuators/BaseClasses/PartialThreeWayValve.mo Outdated Show resolved Hide resolved
IBPSA/Fluid/Movers/Validation/ControlledFlowMachine.mo Outdated Show resolved Hide resolved
IBPSA/Fluid/Movers/Validation/FlowControlled_dp.mo Outdated Show resolved Hide resolved
IBPSA/Fluid/Movers/Validation/FlowControlled_m_flow.mo Outdated Show resolved Hide resolved
IBPSA/Fluid/Movers/Validation/Pump_stratos.mo Outdated Show resolved Hide resolved
IBPSA/Fluid/Movers/Validation/SpeedControlled_y.mo Outdated Show resolved Hide resolved
@mwetter
Copy link
Contributor Author

mwetter commented Sep 20, 2024

@FWuellhorst : Thanks for the review. All changes have been addressed.

@FWuellhorst
Copy link
Contributor

@mwetter, @jelgerjansen did the review ;)
The changes look good for AixLib.

@jelgerjansen
Copy link
Contributor

@mwetter I think you indeed mixed up the names of @FWuellhorst and myself ;)

Thanks for fixing the time constant of the SlewRateLimiter block. Did this lead to significant changes in simulation time and/or results?

The updated revision notes contained an abundant white space, which I fixed myself through commit 27c8c85.

Furthermore, I noticed that some of my remarks were not yet addressed. Did you read over these comments or do you prefer the implementation as is?

@mwetter
Copy link
Contributor Author

mwetter commented Sep 23, 2024

@jelgerjansen : Sorry for missing some comments, I thought I addressed them through the text replace.

The simulation time stays about the same, some models become faster and others slower, see the link in the issue. In Buildings we have some models that don't work and we are looking into this, the reason being pump working against a closed valve and some chattering in control logic. I think it is not fundamental to this change but rather a change in numerics that now needs retuning or reformulation of some models that we got to work for the old filter model. I would expect a similar change being needed if models would have been developed for the new SlewRateLimiter and we were to change to the 2nd order filter.

@jelgerjansen
Copy link
Contributor

@mwetter thanks for addressing the other comments as well. I think this is now ready to be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor actuator travel time
3 participants