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

[WIP] HomeMatic KeyMatic works as a lock device #12384

Closed
wants to merge 7 commits into from
Closed

[WIP] HomeMatic KeyMatic works as a lock device #12384

wants to merge 7 commits into from

Conversation

chrismanivong
Copy link
Contributor

@chrismanivong chrismanivong commented Feb 13, 2018

Description:

Related issue (if applicable): fixes #

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

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
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@homeassistant
Copy link
Contributor

Hi @krys1976,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@@ -460,7 +462,8 @@ def _system_callback_handler(hass, config, src, *args):
('cover', DISCOVER_COVER),
('binary_sensor', DISCOVER_BINARY_SENSORS),
('sensor', DISCOVER_SENSORS),
('climate', DISCOVER_CLIMATE)):
('climate', DISCOVER_CLIMATE),
('lock', DISCOVER_LOCK)):

Choose a reason for hiding this comment

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

indentation contains tabs
indentation contains mixed spaces and tabs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

solved in newer commit

('sensor', DISCOVER_SENSORS),
('climate', DISCOVER_CLIMATE)
('lock', DISCOVER_LOCK)):
# Get all devices of a specific type

Choose a reason for hiding this comment

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

indentation contains mixed spaces and tabs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

solved in newer commit

('binary_sensor', DISCOVER_BINARY_SENSORS),
('sensor', DISCOVER_SENSORS),
('climate', DISCOVER_CLIMATE)
('lock', DISCOVER_LOCK)):

Choose a reason for hiding this comment

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

indentation contains tabs
indentation contains mixed spaces and tabs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

solved in newer commit

@chrismanivong
Copy link
Contributor Author

Hope this push request works so far.

Copy link
Member

@OttoWinter OttoWinter left a comment

Choose a reason for hiding this comment

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

Not a reviewer or something like that, just pointing out some things I noticed.

@@ -0,0 +1,858 @@
"""
Copy link
Member

Choose a reason for hiding this comment

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

Is this components/homematic/__init__.py? If so, why did you copy it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked out current master and made my changes. In current master, this file still exists. May be dropped if not needed.

devices = []
for conf in discovery_info[ATTR_DISCOVER_DEVICES]:
new_device = HMLock(conf)
devices.append(new_device)
Copy link
Member

Choose a reason for hiding this comment

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

devices.append(HMLock(conf))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it the same way like in switch.homematic

def _init_data_struct(self):
"""Generate the data dictionary (self._data) from metadata."""
self._state = "STATE"
self._data.update({self._state: STATE_UNKNOWN})
Copy link
Member

Choose a reason for hiding this comment

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

I would just write self._data[self._state] = STATE_UNKNOWN (I know the other homematic devices do it your way, but this is just much easier to parse in your head)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it the same way like in switch.homematic

def update(self):
"""Update state by device callback."""
self._state = (STATE_LOCKED if self.is_locked()
else STATE_UNLOCKED)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a Lock component expert (nor a Homematic user), but I think this call is unnecessary because LockDevice already handles this for you:

https://github.com/home-assistant/home-assistant/blob/80d2c76e85afbf2da3786492bfa2ab3977bd601a/homeassistant/components/lock/__init__.py#L171-L177

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh ... sure .. ;-)

@pvizeli
Copy link
Member

pvizeli commented Feb 16, 2018

#11980

Maby you can merge it or address the comments from this like the is_open property.

@chrismanivong chrismanivong changed the title HomeMatic KeyMatic works as a lock device [WIP] HomeMatic KeyMatic works as a lock device Feb 21, 2018
@balloob
Copy link
Member

balloob commented May 1, 2018

This PR seems to have gone stale. Closing it. You can reopen it when you're ready to finish it.

@balloob balloob closed this May 1, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Sep 5, 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

6 participants