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

Added cover group platform #12303

Closed
wants to merge 43 commits into from
Closed

Added cover group platform #12303

wants to merge 43 commits into from

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Feb 11, 2018

Description:

This component allows for easy setup of a cover group (control serveral covers with one entity). To achieve the same with current components, it is necessary to use a combination of cover.template, scripts and a sensor.template as seen here https://home-assistant.io/components/cover.template/#multiple-covers

Branch is rebased onto #12592.

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4638

Example entry for configuration.yaml (if applicable):

cover:
  - platform: group
    name: My Cover Group
    entities:
      - cover.hall_window
      - cover.living_room_window

Checklist:

  • The code change is tested and works locally.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass

@OttoWinter
Copy link
Member

Related: #12229

@cdce8p cdce8p changed the title New component - Multicover WIP: New component - Multicover Feb 16, 2018
@cdce8p cdce8p changed the title WIP: New component - Multicover New component - Multicover Feb 16, 2018
@cdce8p cdce8p requested a review from a team as a code owner February 23, 2018 11:37
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

I haven't dug into all the logic yet. Just some minor comments so far.

async_add_devices(
[GroupCover(hass, config.get(CONF_NAME),
config.get(CONF_ENTITIES))])
return True
Copy link
Member

Choose a reason for hiding this comment

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

Remove this. Nothing is checking this return value.

class GroupCover(CoverDevice):
"""Representation of a GroupCover."""

def __init__(self, hass, name, entities):
Copy link
Member

Choose a reason for hiding this comment

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

Don't pass in hass. It will be set on the entity when it has been added to home assistant.


def __init__(self, hass, name, entities):
"""Initialize a GroupCover entity."""
self._hass = hass
Copy link
Member

Choose a reason for hiding this comment

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

Remove this.

self.async_schedule_update_ha_state(True)

for entity_id in self.entities:
new_state = self._hass.states.get(entity_id)
Copy link
Member

Choose a reason for hiding this comment

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

Use self.hass instead.

value.discard(entity_id)
for value in self.tilts.values():
value.discard(entity_id)
if value != set():
Copy link
Member

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but this seems a bit inefficient (creating a new object just to check emptiness).

if value:
    ...

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. tilt = True if value else False is even better.


if old_state is None:
features = new_state.attributes[ATTR_SUPPORTED_FEATURES]
if features & 1 and features & 2:
Copy link
Member

Choose a reason for hiding this comment

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

I don't really like having these hard-coded values in here. 1) they might theoretically change at some point 2)

if features & (SUPPORT_OPEN | SUPPORT_CLOSE):
    ...

Seems more descriptive to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Missed these when transitioning to final commit. Thanks

@property
def is_closed(self):
"""Return if all covers in group are closed."""
if self.covers[KEY_OPEN_CLOSE] == set():
Copy link
Member

@OttoWinter OttoWinter Feb 23, 2018

Choose a reason for hiding this comment

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

Same here: if self.covers[KEY_OPEN_CLOSE]: should (I think) also work.

Copy link
Member Author

Choose a reason for hiding this comment

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

if not since I want to catch an empty set, but you are right.

_LOGGER.debug("Stop covers called")
self._hass.add_job(self._hass.services.async_call(
DOMAIN, SERVICE_STOP_COVER,
{'entity_id': self.covers[KEY_STOP]}))
Copy link
Member

@OttoWinter OttoWinter Feb 23, 2018

Choose a reason for hiding this comment

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

This code pretty much is the same as cover#stop_cover (though it doesn't support async calls, but that could be added). So I believe you could just write:

self.hass.add_job(cover.stop_cover, self.hass, self.covers[KEY_STOP])

Same applies for other service calls in here.



PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Optional(CONF_NAME, default='Group Cover'): cv.string,
Copy link
Member

@OttoWinter OttoWinter Feb 23, 2018

Choose a reason for hiding this comment

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

Don't know about the code guidelines, but most schemas (apart from citybikes (well, kinda)) put their default name in a DEFAULT_NAME global variable.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 26, 2018

Closed in favor of #12692 due to complexity of commits and conflicts from rebasing.

@cdce8p cdce8p closed this Feb 26, 2018
@cdce8p cdce8p deleted the multicover branch February 26, 2018 12:56
cdce8p added a commit that referenced this pull request Mar 15, 2018
* Added cover.group platform
* Added async/await, smaller changes
* Made (async_update) methods regular methods
* Small improvements
* Changed classname
* Changes based on feedback
* Service calls
* update_supported_features is now a callback method
* combined all 'update_attr_*' methods in 'async_update'
* Small changes
* Fixes
   * is_closed
   * current_position
   * current_tilt_position
* Updated tests
* Small changes 2
@balloob balloob mentioned this pull request Mar 30, 2018
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants