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: allow setting both entity and field in deconz.configure #17722

Merged
merged 1 commit into from Oct 26, 2018

Conversation

Projects
None yet
6 participants
@lbschenkel
Contributor

lbschenkel commented Oct 23, 2018

Description:

Problem:

Currently the deconz.configure service allows either specifying a field or an entity, but not both. This makes it impossible to set states in deCONZ when all you have is an entity id (think AppDaemon or other automations), since an entity will resolve to a device path such as /lights/1 (that you can use to change configuration attributes such as the device name) and in order to change states you need to do a PUT to /lights/1/state.

Motivation:

Why setting states via deconz.configure instead of (for example) light.turn_on? Because right now the feature of increasing/decreasing a light's current brightness (or color temperature) which is exposed by deCONZ (which is actually from the Hue bridge API so it applies to the Hue bridge as well) via the bri_inc (and ct_inc) attributes is not accessible via the light platform.

Two examples of why it's necessary to use this:

  1. This is essential to implement a remote control that can smoothly control the lights in HASS, because for example when you hold a "increase brightness" button you want to tell the light to increase its brightness to the max with a transition of (let's say) 5 seconds (bri_inc: 254, transitiontime: 50) and when the button is released you send bri_inc: 0 (also defined by Hue API) to tell the light to stop the fade (this gets translated to a Zigbee "stop" command). Right now via the light platform you can only set explicit brightness targets, and you have to do that in a loop, however this tends to overload the Zigbee network and it's not smooth. The algorithm I describe is how remote controls manage to implement smooth fades with just two Zigbee commands (one to start, one to stop).

  2. Some lights such as IKEA trådfri have firmware bugs in which they fail to respond to commands in case they're in the middle of a long transition (dresden-elektronik/deconz-rest-plugin#894). Sending bri_inc: 0 (which gets translated into a "stop" command in the Zigbee layer) can "ressurect" the lamp. Right now you can't send such a command from HASS, which is useful when you need to work around these kinds of firmware bugs.

However, I'm going to submit a separate WIP PR to introduce that feature (fading brightness/temperature) on the light platform. This PR is about offering a way to set state attributes that exist in the Hue/deCONZ API that for whatever reason are not (yet) exposed in "higher-level" platforms, and all you have is the entity id and do not know the device path. I believe that is even the original intent, since the documentation explicitly refers to /lights/1/state and the fact you can't do that by specifying an entity was probably an oversight.

Solution:

Allow both entity and field to be set simultaneously. This won't break any existing setups because they're currently mutually exclusive. If you set them both, entity is used to resolve the device path and field is added as a subpath. In this way you can set state attributes by setting:

{ "entity": "light.name", "field": "/state", "data": { ... } }

I also considered introducing a new service such as deconz.put_state but I deemed it not to be necessary and probably redundant, given the above. Personally I have no strong preference to one way or another, so I can do that if you believe it's a superior alternative.

Related issue (if applicable): N/A

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

Example entry for configuration.yaml (if applicable):

N/A

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 communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable N/A
  • New dependencies are only imported inside functions that use them N/A
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py N/A
  • New files were added to .coveragerc N/A

If the code does not interact with devices:

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

Great suggestion!

Also don't forget to update services.yaml and component documentation to reflect these changes.

vol.Exclusive(SERVICE_FIELD, 'deconz_id'): cv.string,
vol.Exclusive(SERVICE_ENTITY, 'deconz_id'): cv.entity_id,
vol.Optional(SERVICE_FIELD, 'deconz_id'): cv.string,
vol.Optional(SERVICE_ENTITY, 'deconz_id'): cv.entity_id,
vol.Required(SERVICE_DATA): dict,
})

This comment has been minimized.

@Kane610

Kane610 Oct 23, 2018

Contributor

Use "has_at_least_one_key" to make sure that either field or entity is used.

This comment has been minimized.

@lbschenkel

lbschenkel Oct 23, 2018

Contributor

I was expecting comments on this. I did not know how to do it properly. I'll fix it.

This comment has been minimized.

@Kane610

Kane610 Oct 23, 2018

Contributor

I didn't either. Just looked at alternatives to Exclusive

@@ -44,8 +44,8 @@
SERVICE_DATA = 'data'
SERVICE_SCHEMA = vol.Schema({
vol.Exclusive(SERVICE_FIELD, 'deconz_id'): cv.string,
vol.Exclusive(SERVICE_ENTITY, 'deconz_id'): cv.entity_id,
vol.Optional(SERVICE_FIELD, 'deconz_id'): cv.string,

This comment has been minimized.

@Kane610

Kane610 Oct 23, 2018

Contributor

Change order to have entity listed before field, alphabetical order. This will also help with the description later on.

This comment has been minimized.

@Kane610

Kane610 Oct 23, 2018

Contributor

Use matches_regex to make sure that field starts with an '/'

This comment has been minimized.

@lbschenkel

lbschenkel Oct 23, 2018

Contributor

All right. Will do.

@@ -141,7 +141,8 @@ def async_add_remote(sensors):
Field is a string representing a specific device in deCONZ
e.g. field='/lights/1/state'.
Entity_id can be used to retrieve the proper field.
Entity_id can be used to retrieve the proper device (when both

This comment has been minimized.

@Kane610

Kane610 Oct 23, 2018

Contributor

List entity id first. extend field with how it can be used either by itself or with enitity id

@@ -166,6 +174,8 @@ def async_add_remote(sensors):
_LOGGER.error('Could not find the entity %s', entity_id)
return
field = field + subpath

This comment has been minimized.

@Kane610

Kane610 Oct 23, 2018

Contributor

subpath only exists if entity_id exist.

This comment has been minimized.

@lbschenkel

lbschenkel Oct 23, 2018

Contributor

Ooops. I didn't spend too much effort on correctness TBH, since I wanted to present the idea as a PoC first. Will fix it.

@@ -157,6 +158,13 @@ def async_add_remote(sensors):
deconz = hass.data[DOMAIN]
if entity_id:
subpath = ''
if field:

This comment has been minimized.

@Kane610

Kane610 Oct 23, 2018

Contributor

regex in service schema will take care of this. Just combine entity field and subfield

deconz: Allow setting both 'entity' and 'field' in deconz.configure
Allow both 'entity' and 'field' to be set simultaneously.
In this scenario, 'field' is meant to be a sub-path of the device path
that corresponds to the 'entity'.

This allows device states to be set for entities by setting the
appropriate 'entity' and a value of `/state` in 'field'. This wasn't
possible previously since 'entity' resolves to the device path and
there was not a mechanism to simultaneously specify a sub-path.

@lbschenkel lbschenkel force-pushed the lbschenkel:deconz branch from 09fa751 to 5383465 Oct 23, 2018

@lbschenkel lbschenkel changed the title from WIP: deconz: allow setting both entity+path to WIP: deconz: allow setting both entity and field in deconz.configure Oct 23, 2018

@lbschenkel lbschenkel force-pushed the lbschenkel:deconz branch from 5383465 to 838819b Oct 23, 2018

@lbschenkel

This comment has been minimized.

Contributor

lbschenkel commented Oct 23, 2018

Commit amended and comments addressed. services.yaml updated.
I'll need some guidance for introducing unit tests. I'm new to this codebase.

Regarding documentation, I would appreciate suggestions for the wording to be used. I'm not a native English speaker.

@lbschenkel lbschenkel force-pushed the lbschenkel:deconz branch from 838819b to 992d4d4 Oct 23, 2018

vol.Exclusive(SERVICE_ENTITY, 'deconz_id'): cv.entity_id,
SERVICE_SCHEMA = vol.All(vol.Schema({
vol.Optional(SERVICE_ENTITY, 'deconz_id'): cv.entity_id,
vol.Optional(SERVICE_FIELD, 'deconz_id'): cv.matches_regex('/.*'),

This comment has been minimized.

@Kane610

Kane610 Oct 23, 2018

Contributor

Since you're not using exclusive any more you can remove the 'deconz_id'

This comment has been minimized.

@lbschenkel

lbschenkel Oct 23, 2018

Contributor

I wasn't sure what entity_id was doing there, so I left it as it was but I got the intent now. Removed.

_LOGGER.error('Could not find the entity %s', entity_id)
return
field = entity_path + (field if field is not None else '')

This comment has been minimized.

@Kane610

Kane610 Oct 23, 2018

Contributor

Simplify this by expecting field to be a string by changing call.data.get(SERVICE_FIELD) to call.data.get(SERVICE_FIELD, '')

This comment has been minimized.

@lbschenkel

lbschenkel Oct 23, 2018

Contributor

Of course! Should have thought of that.

field = entities.get(entity_id)
if field is None:
entity_path = hass.data.get(DATA_DECONZ_ID, {}).get(entity_id)

This comment has been minimized.

@Kane610

Kane610 Oct 23, 2018

Contributor

this got really ugly ;)

Maybe do a try/except with KeyError instead?
try: entity_path = hass.data[DATA_DECONZ_ID][entity_id]

This comment has been minimized.

@lbschenkel

lbschenkel Oct 23, 2018

Contributor

TBH I find the code as I wrote it more elegant. I think using exceptions for flow control is an anti-pattern. Plus it has the extra benefit of being more concise. However, that's my own opinion and that's not my codebase. If you insist I'll change it.

This comment has been minimized.

@lbschenkel

lbschenkel Oct 23, 2018

Contributor

I missed the fact when I wrote the above that your suggestion would also eliminate the if, so in fact when doing it the code becomes both more readable and more concise, so my argument does not apply. Amended.

@lbschenkel lbschenkel force-pushed the lbschenkel:deconz branch 2 times, most recently from 09f94c5 to 28bc03f Oct 23, 2018

@lbschenkel

This comment has been minimized.

Contributor

lbschenkel commented Oct 23, 2018

Amended again. New comments addressed.

@Kane610

This comment has been minimized.

Contributor

Kane610 commented Oct 23, 2018

Looks good! I'd like a test or two now that it is a bit more complex

@lbschenkel

This comment has been minimized.

Contributor

lbschenkel commented Oct 23, 2018

I'll need some guidance for introducing unit tests. I'm new to this codebase.

Could you help me out with this, please? Maybe pointing to some unit tests that are similar enough that I could draw inspiration from?

@Kane610

This comment has been minimized.

Contributor

Kane610 commented Oct 23, 2018

You need to find a test that calls a service. You can base the test on

async def test_setup_entry_successful(hass):

You can probably remove most asserts
Then just a couple of tests that runs the different paths in the service
Give it a couple of hours. I'll be at the computer tonight I think

@lbschenkel

This comment has been minimized.

Contributor

lbschenkel commented Oct 23, 2018

OK, thanks. It's getting late in my time zone, so I'm probably only going to give it a go tomorrow.

@lbschenkel lbschenkel force-pushed the lbschenkel:deconz branch from 28bc03f to 33700e3 Oct 25, 2018

# field does not start with /
service_data = {'entity': 'light.test', 'field': 'state', 'data': data}
with patch('pydeconz.DeconzSession.async_put_state', return_value=True) as async_put_state:
await hass.services.async_call('deconz', 'configure', service_data=service_data)

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2018

line too long (88 > 79 characters)

async_put_state.assert_not_called()
# field does not start with /
service_data = {'entity': 'light.test', 'field': 'state', 'data': data}
with patch('pydeconz.DeconzSession.async_put_state', return_value=True) as async_put_state:

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2018

line too long (95 > 79 characters)

# non-existing entity (or not from deCONZ)
service_data = {'entity': 'light.nonexisting', 'field': '/state', 'data': data}
with patch('pydeconz.DeconzSession.async_put_state', return_value=True) as async_put_state:
await hass.services.async_call('deconz', 'configure', service_data=service_data)

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2018

line too long (88 > 79 characters)

# non-existing entity (or not from deCONZ)
service_data = {'entity': 'light.nonexisting', 'field': '/state', 'data': data}
with patch('pydeconz.DeconzSession.async_put_state', return_value=True) as async_put_state:

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2018

line too long (95 > 79 characters)

async_put_state.assert_called_with('/light/1/state', data)
# non-existing entity (or not from deCONZ)
service_data = {'entity': 'light.nonexisting', 'field': '/state', 'data': data}

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2018

line too long (83 > 79 characters)

async_put_state.assert_called_with('/light/42', data)
# only entity
service_data = {'entity': 'light.test', 'data': data}
with patch('pydeconz.DeconzSession.async_put_state', return_value=True) as async_put_state:

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2018

line too long (95 > 79 characters)

# only field
service_data = {'field': '/light/42', 'data': data}
with patch('pydeconz.DeconzSession.async_put_state', return_value=True) as async_put_state:
await hass.services.async_call('deconz', 'configure', service_data=service_data)

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2018

line too long (88 > 79 characters)

# only field
service_data = {'field': '/light/42', 'data': data}
with patch('pydeconz.DeconzSession.async_put_state', return_value=True) as async_put_state:

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2018

line too long (95 > 79 characters)

with patch('pydeconz.DeconzSession.async_get_state',
return_value=mock_coro(CONFIG)), \
patch('pydeconz.DeconzSession.start', return_value=True), \
patch('homeassistant.helpers.device_registry.async_get_registry',

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2018

continuation line over-indented for visual indent

entry.data = {'host': '1.2.3.4', 'port': 80, 'api_key': '1234567890ABCDEF'}
with patch('pydeconz.DeconzSession.async_get_state',
return_value=mock_coro(CONFIG)), \
patch('pydeconz.DeconzSession.start', return_value=True), \

This comment has been minimized.

@houndci-bot

houndci-bot Oct 25, 2018

continuation line over-indented for visual indent

@lbschenkel lbschenkel force-pushed the lbschenkel:deconz branch 2 times, most recently from 599bdf1 to cb98f40 Oct 25, 2018

@lbschenkel

This comment has been minimized.

Contributor

lbschenkel commented Oct 25, 2018

Pushed tentative unit tests.

I have to be honest and say I they are completely based on cargo cult. I know exactly what needs to be done in abstract terms, but I'm not a Python developer and I have no previous exposure to (Python) async, mock testing and the HASS codebase (and how much of HASS gets mocked during testing). I do not really know if those tests are actually technically correct (for example, free of race conditions between invoking the service and asserting), or they only happen to pass due to luck. I made them after lots of trial and error, bashing my head against the wall, fighting with await et al.

Usually I prefer to have small test methods, so each test would be in its own, but in order to cut down the amount of setup boilerplate I ended up testing all cases within the same method.

To improve these tests beyond the state they're in, I really need to be spoon fed and told exactly what needs to be done and where. I'm afraid we reached the limit of what I can currently do unassisted.

@lbschenkel lbschenkel referenced this pull request Oct 25, 2018

Merged

Attributes 'entity' and 'field' can both be used #7078

2 of 2 tasks complete

@lbschenkel lbschenkel changed the title from WIP: deconz: allow setting both entity and field in deconz.configure to deconz: allow setting both entity and field in deconz.configure Oct 25, 2018

@lbschenkel lbschenkel force-pushed the lbschenkel:deconz branch from cb98f40 to cf092b4 Oct 25, 2018

@Kane610

Looks good!

The test is fine, I'm doing some refactoring of the component and will do a rework of it when this PR is merged.

Ok to merge if tests pass.

@lbschenkel lbschenkel force-pushed the lbschenkel:deconz branch from cf092b4 to 47cd586 Oct 25, 2018

@lbschenkel

This comment has been minimized.

Contributor

lbschenkel commented Oct 25, 2018

Got a green build, @Kane610. Tests were passing previously, but I had to change one docstring to please the linter.

@Kane610

This comment has been minimized.

Contributor

Kane610 commented Oct 25, 2018

I'd just like a quick second opinion by @MartinHjelmare or @balloob to top this off.

@MartinHjelmare

Looks good!

@Kane610 Kane610 merged commit 47003fc into home-assistant:dev Oct 26, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.005%) to 93.273%
Details

@wafflebot wafflebot bot removed the in progress label Oct 26, 2018

@balloob balloob referenced this pull request Nov 9, 2018

Merged

0.82 #18335

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment