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

deCONZ - Support device registry #16115

Merged
merged 14 commits into from
Aug 24, 2018

Conversation

Kane610
Copy link
Member

@Kane610 Kane610 commented Aug 21, 2018

Description:

Use device registry #15980

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@@ -119,6 +119,13 @@ def async_add_remote(sensors):

deconz.start()

device_registry = await \
hass.helpers.device_registry.async_get_registry()
device_registry.async_get_or_create(

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

@@ -119,6 +119,13 @@ def async_add_remote(sensors):

deconz.start()

device_registry = await \
hass.helpers.device_registry.async_get_registry()

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
continuation line over-indented for visual indent

'identifiers': [['serial', serial]],
'manufacturer': self._sensor.manufacturer,
'model': self._sensor.modelid,
'connection': [['Zigbee', serial]],
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 that we need to extract 'zigbee' and 'serial' into constants and store them in helpers/device_registry.py.

Also, I thought Zigbee had mac addresses to identify them on the network ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can create the constants for the once in this PR.

Yes, it is just that they name it serial in deconz. I also need to filter out the extra characters that define each sensor on device. I can rename it Mac if you want

device_registry = await \
hass.helpers.device_registry.async_get_registry()
device_registry.async_get_or_create(
[['bridgeid', deconz.config.bridgeid]], 'Dresden Elektronik',
Copy link
Member

Choose a reason for hiding this comment

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

This should not be 'bridgeid' but DOMAIN, as it's a unique identifier of the Deconz domain.

hass.helpers.device_registry.async_get_registry()
device_registry.async_get_or_create(
[['bridgeid', deconz.config.bridgeid]], 'Dresden Elektronik',
deconz.config.modelid, [['Ethernet', deconz.config.mac]],
Copy link
Member

Choose a reason for hiding this comment

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

WiFi and Ethernet share the same mac address space, so we should also group them into the same connection category.

Copy link
Member Author

Choose a reason for hiding this comment

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

So network?

@@ -119,6 +119,13 @@ def async_add_remote(sensors):

deconz.start()

device_registry = await \
hass.helpers.device_registry.async_get_registry()
device_registry.async_get_or_create(
Copy link
Member

Choose a reason for hiding this comment

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

As this example might be copied a lot, I would prefer that we use named arguments to clarify what is what:

device_registry.async_get_or_create(
  identifiers=[[DOMAIN, deconz.config.bridgeid]],
  manufacturer='Dresden Elektronik',
  # etc
)

else:
return None
dev = {
'identifiers': [['serial', serial]],
Copy link
Member

Choose a reason for hiding this comment

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

constant.

@Kane610 Kane610 requested a review from a team as a code owner August 22, 2018 14:44
return_value=mock_coro(True)):
with patch('pydeconz.DeconzSession.async_get_state',
return_value=mock_coro(CONFIG)), \
patch('pydeconz.DeconzSession.start', return_value=True):

Choose a reason for hiding this comment

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

continuation line over-indented for visual indent

return_value=mock_coro(True)):
patch('pydeconz.DeconzSession.async_get_state',
return_value=mock_coro(CONFIG)), \
patch('pydeconz.DeconzSession.start', return_value=True):

Choose a reason for hiding this comment

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

continuation line over-indented for visual indent

with patch.object(deconz, 'async_dispatcher_send') as mock_dispatch_send, \
patch('pydeconz.DeconzSession.async_load_parameters',
return_value=mock_coro(True)):
patch('pydeconz.DeconzSession.async_get_state',

Choose a reason for hiding this comment

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

continuation line over-indented for visual indent

entry.add_to_hass(hass)
with patch('pydeconz.DeconzSession.async_get_state',
return_value=mock_coro(CONFIG)), \
patch('pydeconz.DeconzSession.start', return_value=True):

Choose a reason for hiding this comment

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

continuation line over-indented for visual indent

@Kane610 Kane610 changed the title WIP: deCONZ - Support device registry deCONZ - Support device registry Aug 23, 2018
@Kane610
Copy link
Member Author

Kane610 commented Aug 23, 2018

Something weird is happening with the tests, they don't want to finish.

@property
def device(self):
"""Description for device registry."""
if (self._sensor.uniqueid is not None and
Copy link
Member

@balloob balloob Aug 23, 2018

Choose a reason for hiding this comment

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

Can you inverse this logic to make the method a bit easier to read

if (self._sensor.uniqueid is None or
        self._sensor.uniqueid.count(':') != 7):
    return None

return {
    …
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I can evaluate positive on the counter instead. Then leave return None to be at the end of the method instead.

@property
def device(self):
"""Description for device registry."""
if (self._light.uniqueid is not None and
Copy link
Member

Choose a reason for hiding this comment

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

Same

@property
def device(self):
"""Description for device registry."""
if (self._device.uniqueid is not None and
Copy link
Member

Choose a reason for hiding this comment

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

Same

@property
def device(self):
"""Description for device registry."""
if (self._switch.uniqueid is not None and
Copy link
Member

Choose a reason for hiding this comment

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

Same

return None
dev = {
'connection': [[CONNECTION_ZIGBEE, mac]],
'identifiers': [[IDENTIFIER_MAC, mac]],
Copy link
Member

Choose a reason for hiding this comment

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

There should not be an identifier MAC. There is already a connection one.

Is there a serial number for this sensor? In that case you could add [DECONZ_DOMAIN, serial]

Also MAC should never be used as just a standalone type, as there is Network MAC, Zigbee MAC etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point with DECONZ_DOMAIN since it should only map inside of deconz context 👍

Previously you asked if the sensor had a mac, I will go back to using serial :)

Should I just call them NETWORK_MAC and ZIGBEE_MAC?

Copy link
Member

Choose a reason for hiding this comment

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

yes

@balloob
Copy link
Member

balloob commented Aug 24, 2018

ok to merge when tests pass.

@balloob balloob added this to the 0.77 milestone Aug 24, 2018
@Kane610 Kane610 merged commit e91a152 into home-assistant:dev Aug 24, 2018
@ghost ghost removed the in progress label Aug 24, 2018
balloob pushed a commit that referenced this pull request Aug 25, 2018
Add support for device registry in deCONZ component
@Kane610 Kane610 deleted the deconz-device-registry branch August 29, 2018 08:30
@balloob balloob mentioned this pull request Aug 29, 2018
girlpunk pushed a commit to girlpunk/home-assistant that referenced this pull request Sep 4, 2018
Add support for device registry in deCONZ component
@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 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.

4 participants