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

Issue472 fluid.fmi #598

Merged
merged 19 commits into from
Nov 30, 2016
Merged

Issue472 fluid.fmi #598

merged 19 commits into from
Nov 30, 2016

Conversation

mwetter
Copy link
Contributor

@mwetter mwetter commented Nov 21, 2016

This closes #472

Copy link
Contributor

@marcusfuchs marcusfuchs left a comment

Choose a reason for hiding this comment

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

The package seems well documented and working. I think I found one instance of misplaced annotations (see line comment) and I have one remark regarding one example:

  • In the graphical representation of example Annex60.Fluid.FMI.Adaptors.Examples.ThermalZoneHVACNoExhaust, the constant value for TOut is placed within the area of the thermal zone FMU, but also connected to the source in the HVAC FMU area. This may be confusing for users, as it may not be 100 % clear what would be in which FMU and whether there should be another connection in addition to the adaptor connection that is illustrated by this example.

In addition, I fixed some minor typos. After clarifying the 2 points this is ready to merge from my perspective.

needs to access the conditional connector, but conditional connectors
can only be used in <code>connect</code> statements.
</p>
</html>"));
Copy link
Contributor

Choose a reason for hiding this comment

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

@mwetter
It seems like part of the documentation has moved to the lines above, where it will not be processed as part of the Info section. Should this be removed or integrated to the rest of the documentation below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marcusfuchs
Thanks for your revision and above suggestion (regarding TOut). I added an additional source for TOut.

However, I am not sure what you mean by the change in Annex60/Fluid/FMI/Adaptors/ThermalZone.mo as this is the documentation of the protected block x_i_toX_w. Dymola does not show this documentation, but I think the documentation should still be there.

If you agree with my changes, then please merge it, or else add what else needs to be addressed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mwetter
I meant to suggest to have all the documentation for the top-level model and the internal blocks in one place at the top-level, as some users may overlook the internal block documentation when they read the documentation of the top-level model. But having a second look at it, I also see your point in putting the further information to the internal blocks, keeping the top-level documentation easier to read. I'll merge as is.

@marcusfuchs marcusfuchs merged commit b7e3e29 into master Nov 30, 2016
@marcusfuchs marcusfuchs deleted the issue472_fluid.fmi branch November 30, 2016 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inclusion of Buildings.Fluid.FMI
2 participants