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

Light implementation to control lights using individual group addresses #508

Merged
merged 12 commits into from Dec 21, 2020
Merged

Light implementation to control lights using individual group addresses #508

merged 12 commits into from Dec 21, 2020

Conversation

Lennart99
Copy link
Contributor

@Lennart99 Lennart99 commented Nov 24, 2020

Light implementation to control lights using individual group addresses for red, green, blue and white

Description

This PR adds a light implementation that makes it possible to control RGB and RGBW lights that have separate addresses for red, green, blue and white. I have made this a new class to separate it from the existing code, because the addresses needed are different and I thought it might be better to create new class instead of extending the existing class.

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • I have performed a self-review of my own code
  • The documentation has been adjusted accordingly
  • The changes generate no new warnings
  • Tests have been added that prove the fix is effective or that the feature works
  • The changes are documented in the changelog
  • The Homeassistant plugin has been adjusted in case of new config options

example Homeassistant Configuration

xknx:
  light:
    - name: 'LED strip'
      individual_colors:
        white:
          address: '3/0/0'
          state_address: '3/3/0'
          brightness_address: '3/1/0'
          brightness_state_address: '3/3/1'
        green:
          address: '3/0/4'
          state_address: '3/3/4'
          brightness_address: '3/1/4'
          brightness_state_address: '3/3/5'
        blue:
          address: '3/0/6'
          state_address: '3/3/6'
          brightness_address: '3/1/6'
          brightness_state_address: '3/3/7'
        red:
          address: '3/0/2'
          state_address: '3/3/2'
          brightness_address: '3/1/2'
          brightness_state_address: '3/3/3'

it is also possible to switch all colors on/off with only one address

xknx:
  light:
    - name: 'LED strip'
      address: '3/0/0'
      individual_colors:
        white:
          state_address: '3/3/0'
          brightness_address: '3/1/0'
          brightness_state_address: '3/3/1'
        green:
          state_address: '3/3/4'
          brightness_address: '3/1/4'
          brightness_state_address: '3/3/5'
        blue:
          state_address: '3/3/6'
          brightness_address: '3/1/6'
          brightness_state_address: '3/3/7'
        red:
          state_address: '3/3/2'
          brightness_address: '3/1/2'
          brightness_state_address: '3/3/3'

@coveralls
Copy link

coveralls commented Nov 24, 2020

Coverage Status

Coverage increased (+0.05%) to 94.821% when pulling b287a60 on Lennart99:master into 30720c7 on XKNX:master.

@farmio
Copy link
Member

farmio commented Nov 25, 2020

Thanks, this would be a nice addition! 👍
I'll try to do a review later this week.

@Lennart99
Copy link
Contributor Author

I believe this PR is ready for review now

@Lennart99
Copy link
Contributor Author

I haved moved the functionality into the Light class and updated the config schemas.
I have tried to keep the schemas in the Homeassistant plugin and the library the same as much as possible.

docs/light.md Outdated
@@ -27,6 +27,24 @@ The Light object is either a representation of a binary or dimm actor, LED-contr
- `group_address_tunable_white_state` KNX group address for the current relative color temperature. *DPT 5.001*
- `group_address_color_temperature` KNX group address to set absolute color temperature. *DPT 7.600*
- `group_address_color_temperature_state` KNX group address for the current absolute color temperature. *DPT 7.600*

- `group_address_switch_red` KNX group address to switch the red component. *DPT 1.001*
- `group_address_switch_state_red` KNX group address for the state of the red component. *DPT 1.001*
Copy link
Member

Choose a reason for hiding this comment

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

we usually end with "_state" so it would be group_address_switch_red_state.

Copy link
Contributor Author

@Lennart99 Lennart99 Nov 30, 2020

Choose a reason for hiding this comment

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

Fixed in 56b88cc

docs/light.md Outdated
print(light.supports_tunable_white)
print(light.supports_color_temperature)

# Requesting current state via KNX GROUP WRITE
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Requesting current state via KNX GROUP WRITE
# Requesting current state via KNX GroupValueRead for all _state addresses

Copy link
Contributor Author

@Lennart99 Lennart99 Nov 30, 2020

Choose a reason for hiding this comment

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

Fixed in 56b88cc

Comment on lines 163 to 168
vol.Optional(RED): COLOR_SCHEMA,
vol.Optional(GREEN): COLOR_SCHEMA,
vol.Optional(BLUE): COLOR_SCHEMA,
vol.Optional(WHITE): COLOR_SCHEMA,
vol.Optional(CONF_COLOR_ADDRESS): cv.string,
vol.Optional(CONF_COLOR_STATE_ADDRESS): cv.string,
Copy link
Member

Choose a reason for hiding this comment

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

I think red green blue be in an inclusion group (optional white) and
these 3/4 in an exclusion group with color_address. https://alecthomas.github.io/voluptuous/docs/_build/html/voluptuous.html#module-voluptuous.schema_builder

Copy link
Contributor Author

@Lennart99 Lennart99 Nov 30, 2020

Choose a reason for hiding this comment

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

fixed in 56bd1e3
I have tried to put the color addresses in an exclude group, of course without changing existing behaviour. I couldn't add the exclude to both the color and color_state addresses because it would make it impossible to use them both at the same time, as should be possible.

Comment on lines 203 to 206
vol.Optional(CONF_RED): COLOR_SCHEMA,
vol.Optional(CONF_GREEN): COLOR_SCHEMA,
vol.Optional(CONF_BLUE): COLOR_SCHEMA,
vol.Optional(CONF_WHITE): COLOR_SCHEMA,
Copy link
Member

Choose a reason for hiding this comment

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

again I think this should share an inclusion group.

Copy link
Contributor Author

@Lennart99 Lennart99 Nov 30, 2020

Choose a reason for hiding this comment

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

fixed in 56bd1e3


@property
def is_on(self):
"""Return if light supports color."""
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"""Return if light supports color."""
"""Return if light is on."""

Copy link
Contributor Author

@Lennart99 Lennart99 Nov 30, 2020

Choose a reason for hiding this comment

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

thanks for your review, this must have been an error on my side
Fixed in 56b88cc

@Lennart99
Copy link
Contributor Author

I have updated the schemas to make it possible to omit the normal switch address when the switch addresses for the individual colors are used, it is also possible to use both at the same time. Some example configs are present in the test\config_tests\resources\light folder.

@farmio farmio linked an issue Dec 13, 2020 that may be closed by this pull request
2 tasks
@farmio farmio merged commit a25648e into XKNX:master Dec 21, 2020
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.

Controlling HDL DALI Gateway
3 participants