Fix for stateless covers #22962

giefca · 2019-04-10T09:17:14Z

Description:

Stateless covers didn't work with previous trait

Related issue (if applicable): fixes #22740

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.

If the code does not interact with devices:

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

balloob · 2019-04-10T22:58:18Z

homeassistant/components/google_assistant/trait.py

-                            cover.DOMAIN, cover.SERVICE_CLOSE_COVER, {
-                                ATTR_ENTITY_ID: self.state.entity_id
-                            }, blocking=True, context=data.context)
+                if position is not None:


merge this into else on line above and make it elif

balloob · 2019-04-10T22:58:23Z

homeassistant/components/google_assistant/trait.py

-                            cover.DOMAIN, cover.SERVICE_OPEN_COVER, {
-                                ATTR_ENTITY_ID: self.state.entity_id
-                            }, blocking=True, context=data.context)
+                    if self.state.state != cover.STATE_CLOSED:


Also, we should remove this logic. This makes no sense. We get explicit commands from Google, not toggle commands. We should not have the current state influence how we handle it.

balloob

Please add a test

balloob · 2019-04-10T23:00:02Z

I have reached out to Google about this issue, this is their suggested fix:

The easiest way to do this is to send willReportState:false and return the error code notSupported for queries. CommandOnly support is oncoming, but it's been delayed. Internally, it works the same way though

balloob · 2019-04-10T23:02:54Z

homeassistant/components/google_assistant/trait.py

@@ -1050,6 +1051,8 @@ def query_attributes(self):
            # open, even if that is currently incorrect.
            if self.state.attributes.get(ATTR_ASSUMED_STATE):
                response['openPercent'] = 50
+            elif self.state.state == STATE_UNKNOWN:


See my comment with the response from Google. We should raise NotSupported error instead when it's an assumed state or unknown state.

giefca · 2019-04-11T09:23:15Z

Accidentally closed the PR while pushing corrections.

codecov · 2019-04-11T10:14:49Z

Codecov Report

Merging #22962 into dev will decrease coverage by 0.11%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##              dev   #22962      +/-   ##
==========================================
- Coverage   93.95%   93.84%   -0.12%     
==========================================
  Files         448      449       +1     
  Lines       36750    36767      +17     
==========================================
- Hits        34529    34503      -26     
- Misses       2221     2264      +43

Impacted Files	Coverage Δ
homeassistant/components/google_assistant/trait.py	`96.57% <75%> (+0.43%)`	⬆️
homeassistant/components/mqtt/light/schema_json.py	`73.06% <0%> (-20.67%)`	⬇️
homeassistant/components/mqtt/lock.py	`91.83% <0%> (-8.17%)`	⬇️
homeassistant/loader.py	`94.11% <0%> (-0.2%)`	⬇️
homeassistant/helpers/service.py	`93.19% <0%> (-0.08%)`	⬇️
homeassistant/helpers/entity_component.py	`96.18% <0%> (-0.03%)`	⬇️
homeassistant/components/deconz/deconz_device.py	`100% <0%> (ø)`	⬆️
homeassistant/helpers/aiohttp_client.py	`95.34% <0%> (ø)`	⬆️
homeassistant/components/demo/media_player.py	`95.42% <0%> (ø)`	⬆️
...assistant/components/mqtt/light/schema_template.py	`80.27% <0%> (ø)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d078e50...3c506e2. Read the comment docs.

balloob · 2019-04-11T20:29:12Z

homeassistant/components/google_assistant/trait.py

@@ -1049,7 +1050,9 @@ def query_attributes(self):
            # Google will not issue an open command if the assumed state is
            # open, even if that is currently incorrect.
            if self.state.attributes.get(ATTR_ASSUMED_STATE):
-                response['openPercent'] = 50
+                response['willReportState'] = False


This is wrong. reportState is a sync attribute and we already set it to False. You need to raise an error instead

raise SmartHomeError( ERR_FUNCTION_NOT_SUPPORTED, 'Querying state is not supported')

And you need to raise that error when it's both assumed state and when it's unknown.

Ok, but it won't fix the issue. It looks like google assistant is making a query before executing an open/close command. When we raise an error on query it raises the same error while executing the command. While CommandOnly is not available can we just assume openPercent = 50? It would allow us to open and close stateless covers.

I wasn't raising the right error. Now with ERR_NOT_SUPPORTED it works. Stateless covers are moving but google assistant says otherwise due to the raised error.

balloob · 2019-04-11T20:32:16Z

homeassistant/components/google_assistant/trait.py

-                            cover.DOMAIN, cover.SERVICE_OPEN_COVER, {
-                                ATTR_ENTITY_ID: self.state.entity_id
-                            }, blocking=True, context=data.context)
+                if params['openPercent'] < 100:


These two checks can be removed? We should not get a position and not be able to deal with it. Google should return just 0 or 100 for covers that we won't support getting the state from.

It can be removed, I can raise an error instead. I though it was better to open and close the cover with a position command even if it didn't support setting a position.

raise SmartHomeError( ERR_FUNCTION_NOT_SUPPORTED, 'Setting a position is not supported')

In what cases do we get here?

We get here if we ask for a position and the cover doesn't support setting position. Raising this error may be less confusing for the user than a cover not doing what he asked.

pergolafabio · 2019-04-13T10:25:17Z

Hey, do you want is to test it again?

balloob · 2019-04-13T15:08:04Z

tests/components/google_assistant/test_trait.py

    # Assumed state
    trt = trait.OpenCloseTrait(hass, State('cover.bla', cover.STATE_OPEN, {
        ATTR_ASSUMED_STATE: True,
    }), BASIC_CONFIG)

    assert trt.sync_attributes() == {}
-    assert trt.query_attributes() == {


Can you update the test to make sure that we raise for unknown and assumed state?

with pytest.raises(SmartHomeError): trt.query_attributes()

balloob

Awesome, thanks ! This will be part of the beta on Wednesday, released a week later 👍

HarrisonPace · 2019-04-28T01:55:25Z

This PR isn't ideal:

As of the 0.92 update it will issue the command thanks to #22962, however it replies with "that mode isn't available for the "cover name"". Even though the command is sent and the cover opens. Very annoying to hear that each time.

I opened an issue here: #23226

homeassistant added cla-signed core integration: google_assistant small-pr PRs with less than 30 lines. labels Apr 10, 2019

ghost added the in progress label Apr 10, 2019

balloob reviewed Apr 10, 2019

View reviewed changes

balloob requested changes Apr 10, 2019

View reviewed changes

balloob added this to the 0.92.0 milestone Apr 10, 2019

balloob reviewed Apr 10, 2019

View reviewed changes

giefca closed this Apr 11, 2019

giefca force-pushed the dev branch from d159c33 to bbedf09 Compare April 11, 2019 09:15

ghost removed the in progress label Apr 11, 2019

Corrections and test

7d5d8db

giefca reopened this Apr 11, 2019

ghost added the in progress label Apr 11, 2019

balloob reviewed Apr 11, 2019

View reviewed changes

giefca added 2 commits April 13, 2019 10:51

Raise error if does not support setting position

6cf2a41

Raising error for stateless and assumed state covers

e0b25d7

Fix pylint

15469fa

balloob reviewed Apr 13, 2019

View reviewed changes

giefca added 2 commits April 13, 2019 18:36

Tests

40c0715

Update test_trait.py

3c506e2

balloob approved these changes Apr 15, 2019

View reviewed changes

balloob removed this from the 0.92.0 milestone Apr 15, 2019

balloob merged commit b804919 into home-assistant:dev Apr 15, 2019

ghost removed the in progress label Apr 15, 2019

giefca mentioned this pull request Apr 15, 2019

Optimistic Covers lack persistent state - prevents Google Assistant working. #23069

Closed

HarrisonPace mentioned this pull request Apr 19, 2019

Optimistic Covers - Google Assistant Cloud Component "that mode isn't available for covername" #23226

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for stateless covers #22962

Fix for stateless covers #22962

giefca commented Apr 10, 2019 •

edited

balloob Apr 10, 2019

balloob Apr 10, 2019

balloob Apr 10, 2019

balloob left a comment

balloob commented Apr 10, 2019

balloob Apr 10, 2019

giefca commented Apr 11, 2019

codecov bot commented Apr 11, 2019 •

edited

balloob Apr 11, 2019

balloob Apr 11, 2019

giefca Apr 11, 2019

giefca Apr 13, 2019

balloob Apr 11, 2019

giefca Apr 11, 2019 •

edited

balloob Apr 13, 2019

giefca Apr 13, 2019 •

edited

pergolafabio commented Apr 13, 2019

balloob Apr 13, 2019

balloob left a comment

HarrisonPace commented Apr 28, 2019

Fix for stateless covers #22962

Fix for stateless covers #22962

Conversation

giefca commented Apr 10, 2019 • edited

Description:

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob left a comment

Choose a reason for hiding this comment

balloob commented Apr 10, 2019

Choose a reason for hiding this comment

giefca commented Apr 11, 2019

codecov bot commented Apr 11, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giefca Apr 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giefca Apr 13, 2019 • edited

Choose a reason for hiding this comment

pergolafabio commented Apr 13, 2019

Choose a reason for hiding this comment

balloob left a comment

Choose a reason for hiding this comment

HarrisonPace commented Apr 28, 2019

giefca commented Apr 10, 2019 •

edited

codecov bot commented Apr 11, 2019 •

edited

giefca Apr 11, 2019 •

edited

giefca Apr 13, 2019 •

edited