Chilled beam sequences

[karthikeyad-pnnl]

Pull request for adding chilled beam sequences to repo

[karthikeyad-pnnl]

@mwetter : Hi Michael, is there a staging branch to which I should submit the pull request?

Thanks,
Karthik

[mwetter]

@karthikeyad-pnnl : Can you please make the PR against issue2614_ChilledBeamSequences

[karthikeyad-pnnl]

@mwetter: Thanks!

[mwetter]

@karthikeyad-pnnl : Thanks for the PR #2626 which I merged and made some minor changes on the branch issue2614_ChilledBeamSequences.

Can you please also address the following points (after merging issue2614_ChilledBeamSequences):

• Add a closed loop example that shows that the controller works closed loop for the control of an HVAC system. This should be outside of the OBC package.
• Could you also update the comments of the blocks so it is clearer what their functionality is. See for example fe47db9
• Make sure all packages have an info section, for example Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem and Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem.System have no info section. Here it would be good to state what the source of the sequences are.
• Rename Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem.Types.OperationModeTypes to Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem.Types.OperationModes
• In particular for more complex examples such as Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem.System.Validation.SystemController please add one or two sentences that describe the experiment, e.g., what is the model testing. (It is clear that Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem.System.Validation.SystemController validates Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem.System.SystemController, but what aspect of it is it validating? Think about a user who wants to understand how to use the block, or a developer who may make changes and needs to interpret whether the validation is still correct.)
• In Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem.SecondaryPumps.Controller there is a vector output yChiWatPum but yPumSpe is only scalar. Wouldn't it be clearer and easier to use if yPumSpe is also a vector so that each pump can get its speed control signal? -- Revision: Use a scalar signal
• The controller Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem.SecondaryPumps.Subsequences.Speed_remoteDp uses a PID controller, and does not expose whether it is P, PI, or PID. By default, it uses PID. Please change to PI as default, derivative action is only very seldom needed, and also expose the parameter that allows switching from PI to P etc.
• The controller Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem.SecondaryPumps.Controller has a unit delay with a sample time of 1 second. This will cause the solver to stop every 1 second of simulation, which is prohibitive expensive (you can try it with putting such a delay for example in the VAVReheat model to see the effect). This will need to be changed.
• Please expand the description of Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem.SecondaryPumps.Controller so it is clear to a user what the inputs and outputs are. For example, uPumLeaLag is not clear. You may need to give an example in the info section. For example, are these unique (strictly) positive values, or how is this order array defined?
• If dpChiWatSet is set to zero, for example if the system is off, then div[] causes a division by zero. This needs to be guarded against.

[karthikeyad-pnnl]

@mwetter : Hi Michael,

I have addressed these comments with changes on PR #2783. I have addressed all of them except for the first comment about adding a closed loop validation model. That will require the clean-up of the DOAS control sequences, which I have already discussed with you. Please review the changes.

Thanks,
Karthik

[mwetter]

@karthikeyad-pnnl : The changes look good. I have a couple more questions/suggestions (besides the needed addition of the closed loop example).

• In BypassValvePosition, there is a guard against division by zero. It add a small negative number. But in Validation.BypassValvePosition, the input to which this small negative number is added is positive. This is incorrect. Please correct it, add a clear description whether Buildings.Controls.OBC.CDL.Interfaces.RealInput dpChiWatLoo needs to be positive (or negative), and set the min or max attribute in the connector.
• The top-level file states "The sequence of operations were compiled after a literature review of the best practices in the industry. " -- Is there a citation you can add? Also, if the citation is not from ASHRAE, then this package needs to be moved from OBC.ASHRAE.PrimarySystems.ChilledBeams to OBC.PrimarySystems.ChilledBeams (or even drop the "PrimarySystems").
• In the top-level file, can you add a schematics that shows for what hydraulic system these controllers are applicable. This would help users understand the applicability, and maybe you can use the same figure for the closed loop example.

I also refactored the models to remove the AddParameter block as this may be deprecated based on the ASHRAE 231P discussions, and I renamed packages and models to shorten the name, using

import buildingspy.development.refactor as r

r.move_class("Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem.System.SystemController",
             "Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem.System.Controller")
r.move_class("Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem.System.Validation.SystemController",
             "Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem.System.Validation.Controller")
r.move_class("Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem.Terminal.Validation.TerminalController",
             "Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem.Terminal.Validation.Controller")
r.move_class("Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem.Terminal.TerminalController",
             "Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem.Terminal.Controller")
r.move_class("Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeamSystem",
             "Buildings.Controls.OBC.ASHRAE.PrimarySystem.ChilledBeams")

Note that to run the unit tests, you need to update BuildingsPy to include the changes of lbl-srg/BuildingsPy#446. This is currently in the CI tests at lbl-srg/BuildingsPy#447

[mwetter]

@karthikeyad-pnnl : Thanks for the changes. The implementation improved considerably. Besides the closed loop model, there are a few more items that need to address, please see below. Please update your branch as I committed quite a lot of changes.

• In ./Resources/Images/Controls/OBC/ChilledBeams/ChilledBeamsSchematic.png which is great to give context, I suggest to (i) removed the "isolation valve" as this is for repairs only, (ii) show what are the controller inputs, (iii) move the "request to primary loop" to the right (so signals enter the controller from the left and leave on the right). Maybe you can even delete (or grey out) the "primary loop" as this is outside the scope.
• In the validation cases, please don't just write "This example validates ...". Everyone knows that. Better, explain what you test, such as that first the pumps ramp up and then down because of xxx. Then, users and developers will have a better chance understanding what you test with the model.
• Make a pull request for ./Resources/Images/Controls/OBC/ChilledBeams/ChilledBeamsSchematic.svg so that it can be edited.
• In Buildings.Controls.OBC.ChilledBeams.SetPoints.BypassValvePosition you normalize the control input twice, first with the division block, and then by the parameter conPID.r. Remove the division block.
• Many blocks set the parameters nPum = 2 and nVal = 3. These must all be removed as the number depends on the actual installation. Rather, set the min attribute to 1. I did this in a few but not all blocks. (We want to force the user to set parameters for which no generally good default value can be given.)
• As above, remove the default values for chiWatStaPreMax and chiWatStaPreMin as these are design values. Otherwise, systems that are differently designed (which smaller systems will) would control erratically.
• Make sure parameters that have no default values are all on the main tab, only parameters with good default values should be on other tabs. I corrected some in SetPoints.ZoneRegulation.
• SetPoints.ZoneRegulation uses the room temperature set points as a parameter rather than an input. How would you do demand response with such a controller, or shift the cooling set point as a function of outdoor air temperature (which is in some energy codes). This should be changed to have the heating and cooling set point as an input to the controller so it can be adjusted in a BAS.
• Terminal.Controller has a parameter schTab. This should also be an input as it should come from a higher level schedule.
• In general, it would be easier to use the blocks if the input and outputs where labelled, and, in particular the higher level blocks, had an icon that visualizes what the block is doing. This way, users should understand faster what the control diagram means.
• The controller System.Controller has a sampling time of 30 seconds (for the trim and response logic). Do you need such fast sampling? If we could use 120 seconds as the minimum sampling time, that would be much better in terms of computing speed. I noted that this value is used for the chilled water static pressure reset as a function of the valve positions. As valve open/close in about 120 seconds, even 30 seconds is probably slow. Why not replacing this with a PI controller (as a trim & respond controller with equal trim and respond magnitudes is a special case of a time-discretized PI controller)
• Also, some other models sample every 60 seconds. Again, for computing speed, it would be great if this can be made at least 120 seconds (as we already use in VAV sequences).

[karthikeyad-pnnl]

Thanks for the feedback, Michael! Will address the comments over the next week or so

[mwetter]

@karthikeyad-pnnl : I merged your PR and update to the current version of the master (CDL.Continuous is now renamed). This is not yet ready for review. I need more updates to the code from you before reviewing this code.

• I still can't find the closed loop example, see Chilled beam sequences #2614 (comment)
  This is important to understand the full context.

• Also, in https://github.com/lbl-srg/modelica-buildings/blob/issue2614_ChilledBeamSequences/Buildings/Resources/Images/Controls/OBC/ChilledBeams/ChilledBeamsSchematic.png
  shouldn't the differential pressure sensor be upstream of the "chilled water control valve" (if the control intend is to provide sufficient head). Also, in this figure, I assume there is one bypass valve, but many chilled water control valves, e.g., one for each chilled beam manifold. The figure needs to make this clear.

• Also, explain all the abbreviations as non-native speakers won't necessarily know them.

• I assume "cav" is constant air volume, but then what is a "constant air volume damper", or do these dampers indeed maintain a constant air volume flow rate? For example, in ChilledBeams/Terminal/Controller, it says that yDam is used to regulate the zone temperature, so this makes me think the air flow rate is not constant as it cannot be contant while at the same time regulating the temperature.

[karthikeyad-pnnl]

@mwetter Thank you for the comments! I was planning to include the closed loop model after cleaning up and merging the DOAS control sequences, because the closed loop model relies on a DOAS system for ventilation. We will prioritize the sequence cleanup, and then incorporate the closed loop model into this PR. We will let you know once this is ready for review again after that