search

lbl-srg / modelica-buildings

Modelica Buildings library
247 stars 155 forks source link

add additional control blocks for CDL #610

Closed mwetter closed 7 years ago

mwetter commented 7 years ago

The following blocks need to be added to the package Experimental.OpenBuildingControl.CDL

For development, make a new branch based on issue609_cdl

milicag commented 7 years ago

@mwetter Some of the above listed blocks (e.g. both SetPoint blocks, SignalRanker) extend Interfaces (SISO, MIMO) and basic block icon. Would you like me to copy those over to the CLD package as well, or should I rather bundle them into the blocks that I'm creating?

mwetter commented 7 years ago

Please instantiate the inputs and outputs of these base classes (SISO, MIMO) in the new blocks so that we don't have extends statements in the CDL package.

milicag commented 7 years ago

I noticed that all the currently available blocks in ..CDL.Psychrometrics contain extends Modelica.Blocks.Icons.Block

mwetter commented 7 years ago

These extends statements should be removed and instead the graphical elements be copied.

milicag commented 7 years ago

Sounds good, thanks.

milicag commented 7 years ago

What should we do about Modelica.SIunits.*?

mwetter commented 7 years ago

Please use Modelica.SIunits.*

milicag commented 7 years ago

I have an issue with the DayType. It needs a DayTypeOutput interface, which uses a Day type. If I try to avoid defining Day type in a separate file and simply define it in DayType, I can't place the DayTypeOutput connector under interfaces. Should I define both the Day type under a new Types folder and then a DayTypeOutput interface, or is there a more preferred way to deal with this?

milicag commented 7 years ago

I'm erasing my original comment. It looks like the time block could use a revision at some point, but it should be good for now.

mwetter commented 7 years ago

Could you please correct this one your branch. We can then merge the fix to the development branch.

On Tue, Jan 24, 2017 at 2:01 PM, milicag notifications@github.com wrote:

Regarding the time block, there is an unnecessary extend statement in the Buildings.Utilities.Time.CalendarTime, which simply adds an icon on top of an existing icon. It looks like this block could use a revision at some point.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lbl-srg/modelica-buildings/issues/610#issuecomment-274953135, or mute the thread https://github.com/notifications/unsubscribe-auth/ABRHuGHrywUyPykMXGBpU1ZsEBHtv-Hvks5rVnSwgaJpZM4La_Ih .

milicag commented 7 years ago

This was an oversight on my side. Sorry about that, the time block can stay as it is.

On Tue, Jan 24, 2017 at 4:47 PM, Michael Wetter notifications@github.com wrote:

Could you please correct this one your branch. We can then merge the fix to the development branch.

On Tue, Jan 24, 2017 at 2:01 PM, milicag notifications@github.com wrote:

Regarding the time block, there is an unnecessary extend statement in the Buildings.Utilities.Time.CalendarTime, which simply adds an icon on top of an existing icon. It looks like this block could use a revision at some point.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lbl-srg/modelica-buildings/issues/610# issuecomment-274953135, or mute the thread https://github.com/notifications/unsubscribe-auth/ ABRHuGHrywUyPykMXGBpU1ZsEBHtv-Hvks5rVnSwgaJpZM4La_Ih .

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/lbl-srg/modelica-buildings/issues/610#issuecomment-274986659, or mute the thread https://github.com/notifications/unsubscribe-auth/AFE4v6PMj3Cywz-srpw2MTgzK5bvgGuIks5rVpuwgaJpZM4La_Ih .

-- Milica Grahovac, PhD Energy Efficiency Standards Group Lawrence Berkeley National Lab (510) 486-7227

JayHuLBL commented 7 years ago

I created a new branch based on issue610_cdl_additional_blocks, with name of issue610_cdl_extra_basic_blocks. Following classes have been added so far:

More blocks will add to the package CDL.Logical:

The extends stataments were not used. Beside using the Modelica.SIunits.*, I also used Modelica.Math.*. Should we use Modelica.Math.* in CDL?

milicag commented 7 years ago

This comment lists issues with the basic blocks noticed while modeling the sequences:

mwetter commented 7 years ago

@milicag You could add a new block CDL.Conversions.BooleanToReal. I would prefer such a block over conditional output connectors for simplicity. If you add a block, please also add a validation test.

You could add blocks Or3 and And3 similar to what we have for Add3.

milicag commented 7 years ago

@mwetter Sounds good, I'll open an issue to add these 3 basic blocks.

milicag commented 7 years ago

@JayHuLBL sorry for closing and reopening the, I was just realizing the comment I was typing was irrelevant.

mwetter commented 7 years ago

@milicag The above blocks are all on the issue609_cdl branch. Can this issue be closed? Also, the issue610_cdl_additional_blocks has no unmerged code: https://github.com/lbl-srg/modelica-buildings/compare/issue609_cdl...issue610_cdl_additional_blocks?expand=1 I will therefore delete it.

milicag commented 7 years ago

This issue is resolved and relevant blocks added to issue609_cdl branch.