core icon indicating copy to clipboard operation
core copied to clipboard

Make temperature sensor value displayed in Google Home

Open Nardol opened this issue 1 year ago • 2 comments

Proposed change

In Google Home, temperature sensor values are not displayed. It is because TemperatureControl trait does not allow any value to be displayed. To display the temperature in Google Home, we have to use TemperatureSetting treat, like a thermostat. With queryOnlyTemperatureSetting set to True no buttons are shown to change any setting which is expected for a sensor. At last, target temperature must be equal to the ambient one and mode set to heat to have only the temperature announced when asking for the State by voice. If things are better for Google Home, precision by voice changes. Before precision was 0.1, with these changes precision is 0.5° as it is for thermostats, which should not really be a regression.

Type of change

  • [ ] Dependency upgrade
  • [x] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [ ] New feature (which adds functionality to an existing integration)
  • [ ] Deprecation (breaking change to happen in the future)
  • [ ] Breaking change (fix/feature causing existing functionality to break)
  • [ ] Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #92242
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • [x] The code change is tested and works locally.
  • [x] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [x] I have followed the perfect PR recommendations
  • [x] The code has been formatted using Black (black --fast homeassistant tests)
  • [x] Tests have been added to verify that the new code works.

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

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

  • [ ] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [ ] New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • [ ] Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

Nardol avatar May 02 '23 11:05 Nardol

Hey there @home-assistant/cloud, mind taking a look at this pull request as it has been labeled with an integration (google_assistant) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of google_assistant can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign google_assistant Removes the current integration label and assignees on the pull request, add the integration domain after the command.

home-assistant[bot] avatar May 02 '23 11:05 home-assistant[bot]

There is a undesired behavior with this implementation. Even if I did not set any thermostatHumidityAmbient Google Assistant replies the temperature sensor cannot be found when I ask the hydrometry of a room by voice.

Nardol avatar May 02 '23 17:05 Nardol

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar May 02 '23 19:05 home-assistant[bot]

Some tests are failing after you apply the suggested changes, also some additional tests might be needed to get the changes covered.

jbouwh avatar May 02 '23 19:05 jbouwh

I'm going to keep "heat" for the mode because if mode is different from heat Google Assistant specifies the thermometer is off instead of only announcing the temperature. For example : "Bedroom thermometer is off and the temperature is 19°" is announced when mode is not set to "heat" instead of "The temperature is 19°". It is the case in French, I don't know about English for example. "on" looks useless so keeping only "heat" is OK.

Nardol avatar May 03 '23 08:05 Nardol

I'm going to keep "heat" for the mode because if mode is different from heat Google Assistant specifies the thermometer is off instead of only announcing the temperature. For example : "Bedroom thermometer is off and the temperature is 19°" is announced when mode is not set to "heat" instead of "The temperature is 19°". It is the case in French, I don't know about English for example. "on" looks useless so keeping only "heat" is OK.

I tested with none which worked okay. Remind that the sensor is not part of a climate, and has no heat mode.

jbouwh avatar May 03 '23 09:05 jbouwh

I tested with none which worked okay. Remind that the sensor is not part of a climate, and has no heat mode.

Another fantastic thing from Google, I tested again in French and with none it announces my thermometer is off which is really ugly in an end user perspective, that's why I thought it could be a good idea to use heat mode even if I fully agree to say it is not logic at all and the way to do, I don't see what technical issue it could cause.

Nardol avatar May 03 '23 12:05 Nardol

Another fantastic thing from Google, I tested again in French and with none it announces my thermometer is off which is really ugly in an end user perspective, that's why I thought it could be a good idea to use heat mode even if I fully agree to say it is not logic at all and the way to do, I don't see what technical issue it could cause.

Could you test with on. That should be valid too https://developers.home.google.com/cloud-to-cloud/traits/temperaturesetting

jbouwh avatar May 03 '23 13:05 jbouwh

Could you test with on. That should be valid too https://developers.home.google.com/cloud-to-cloud/traits/temperaturesetting

I had already tested it yesterday but I tested again in the case I would have missed something and unfortunately I observe the same behavior, in French Google Assistant replies:

Thermomètre chambre est éteint et la température est actuellement de 20°

If I translate:

Bedroom thermometer is turned off and the temperature is currently 20°

I totally agree it is completely illogical... Also the difference we have when using a language or another; it looks like only heat mode allows to have no useless state announced.

Nardol avatar May 03 '23 14:05 Nardol

Could you test with on. That should be valid too https://developers.home.google.com/cloud-to-cloud/traits/temperaturesetting

I had already tested it yesterday but I tested again in the case I would have missed something and unfortunately I observe the same behavior, in French Google Assistant replies:

Thermomètre chambre est éteint et la température est actuellement de 20°

If I translate:

Bedroom thermometer is turned off and the temperature is currently 20°

I totally agree it is completely illogical... Also the difference we have when using a language or another; it looks like only heat mode allows to have no useless state announced.

Should we not be using this trait: https://developers.home.google.com/cloud-to-cloud/traits/sensorstate

jbouwh avatar May 03 '23 14:05 jbouwh

Should we not be using this trait: https://developers.home.google.com/cloud-to-cloud/traits/sensorstate

I have not found temperature in the list of compatible sensors: https://developers.home.google.com/cloud-to-cloud/traits/sensorstate?hl=fr#supported-sensors Or did I miss-understand something?

Nardol avatar May 03 '23 14:05 Nardol

Should we not be using this trait: https://developers.home.google.com/cloud-to-cloud/traits/sensorstate

I have not found temperature in the list of compatible sensors: https://developers.home.google.com/cloud-to-cloud/traits/sensorstate?hl=fr#supported-sensors Or did I miss-understand something?

No you did not miss understand, I did. About the modes. maybe off should work. I do not see the sensor it self is off some how. How can I reproduce your issue (BTW, to understand French I need Google to translate it for me :-))

jbouwh avatar May 03 '23 14:05 jbouwh

afbeelding This is what I see with on. Is it switched off?

jbouwh avatar May 03 '23 14:05 jbouwh

I cannot confirm you I have the same because I am blind, so I attach two screenshots to this comment: Screenshot_20230503-170527 Screenshot_20230503-170553 Hop we talk about the same.

Nardol avatar May 03 '23 15:05 Nardol

That is similar indeed. I tested with none, on and off. I believe none makes sense. As it means there is no active mode.

Found at:

https://developers.home.google.com/cloud-to-cloud/traits/temperaturesetting?hl=fr#device-attributes

jbouwh avatar May 03 '23 15:05 jbouwh

That is similar indeed. I tested with none, on and off. I believe none makes sense. As it means there is no active mode.

Found at:

https://developers.home.google.com/cloud-to-cloud/traits/temperaturesetting?hl=fr#device-attributes

I totally agree in a purely logical and technical point of view, however I still throw the same objection because of the result I have in French anyway. Unfortunately we cannot know if only French is impacted.

About another subject: if you have a separate sensor for humidity in the same room as a thermometer, do you also observe Google Assistant will try to get hydrometry from the thermometer even if no hydrometry is sent by this thermometer using TemperatureSetting treat? Or is it another kind of privilege (bug) Google reserved for French? :slightly_smiling_face:

Nardol avatar May 03 '23 15:05 Nardol

I've read again and in fact, none is not listed in the sync attribute, only in the device states https://developers.home.google.com/cloud-to-cloud/traits/temperaturesetting?#device-states

Until now I modified it everywhere, I'm going to test within setting it to none only for query.

Result is not miraculous, Google Assistant still replies my sensor is off. I defined only "on" when syncing and none for mode when querying.

Nardol avatar May 03 '23 16:05 Nardol

I will have a closer look. I could try changing the language. My localization is nl (Dutch) btw.

jbouwh avatar May 04 '23 06:05 jbouwh

Thank you very much. If I can also help you in any manner...

I also created a bug report hopping it would be useful: https://issuetracker.google.com/issues/280571184 I am not really confident because sadely Google have always been not really efficient to fix bugs in French in my experience.

I now have to provide more information, especially the result of sync request. IIRC I have to set log level to debug for homeassistant.components.google_assistant to have these information... I think I'll do it this afternoon.

Nardol avatar May 04 '23 07:05 Nardol

In the mean while I tested with none as operation mode and returning none here:

https://github.com/home-assistant/core/pull/92361/files#diff-610fcbdd05a06183431282ef0d29febefc0c5d56166c572cb35e6d6bdf4ef857R903-R905

I added both humidity and a temperature sensor. And tested with Dutch, German, English and French. In all scenario's the sensor show the correct reading. May be this is an accessibility bug so you screen reader results in an incorrect response?

jbouwh avatar May 04 '23 15:05 jbouwh

I added both humidity and a temperature sensor. And tested with Dutch, German, English and French. In all scenario's the sensor show the correct reading. May be this is an accessibility bug so you screen reader results in an incorrect response?

I don't think so, as I tested with a Nest Mini. And with Android I have the same result not using feedback from my screen reader but from Google Assistant's voice.

I hop it is not because I also have a thermostat (Nest thermostat E) connected to Google Assistant which provide both humidity and temperature which might make some strange behavior with sensors using the same trait. It looks our accounts don't have the same version of Google Assistant (in the cloud) or I really don't understand why we don't have the same behavior...

To be sure, when asking for humidity you only have the humidity sensor value read and no error about the thermometer which is in the same room and when asking for the temperature, Google Assistant only replies with the temperature and nothing else like " is off"? Sadly it is not the first time Google Assistant does not react the same way from a Google account to another...

Nardol avatar May 04 '23 16:05 Nardol

To be sure, when asking for humidity you only have the humidity sensor value read and no error about the thermometer which is in the same room and when asking for the temperature, Google Assistant only replies with the temperature and nothing else like " is off"? Sadly it is not the first time Google Assistant does not react the same way from a Google account to another...

I could reproduce your issue! When asking for humidity it could not reach Temperature. When asking for Temperature it returns the temperature but as switched off.

jbouwh avatar May 04 '23 17:05 jbouwh

When defaulting to your PR the complaints about Temperature turned off is indeed gone. When querying humidity (same room) google assistant tells me the correct humidity but adds, that it cannot reach humidity. I suppose that is what you mean.

jbouwh avatar May 04 '23 17:05 jbouwh

I could reproduce your issue! When asking for humidity it could not reach Temperature. When asking for Temperature it returns the temperature but as switched off. [...] When defaulting to your PR the complaints about Temperature turned off is indeed gone. When querying humidity (same room) google assistant tells me the correct humidity but adds, that it cannot reach humidity. I suppose that is what you mean.

Yes, it is. Nice, I am not crazy nor alone anymore :smile: so it looks like I did not explain correctly before... Maybe the issue on the Google issue tracker should be updated, can you reproduce in all languages or only French?

For the case of humidity I did not find any workaround for now. I opened a separate issue: https://issuetracker.google.com/issues/280475503

Nardol avatar May 04 '23 17:05 Nardol

Given this comment:

https://github.com/home-assistant/core/pull/92361/files#diff-610fcbdd05a06183431282ef0d29febefc0c5d56166c572cb35e6d6bdf4ef857R930-R933

Using head seems not a bad idea after all.

jbouwh avatar May 04 '23 17:05 jbouwh

Maybe the issue on the Google issue tracker should be updated, can you reproduce in all languages or only French?

At least in Dutch. The other languages I only had a visual inspection, but then there is nothing to see.

jbouwh avatar May 04 '23 17:05 jbouwh

Given this comment:

https://github.com/home-assistant/core/pull/92361/files#diff-610fcbdd05a06183431282ef0d29febefc0c5d56166c572cb35e6d6bdf4ef857R930-R933

Using head seems not a bad idea after all.

Should it be better to set no mode at all so we would have heat only? Even if I am not sure, because except if I read the wrong line the comment states we cannot change the temperature (so with execute command) if there is no mode. And in our case, the sensor is query only so no need to change the temperature, it is done automatically by the sensor value.

At least in Dutch. The other languages I only had a visual inspection, but then there is nothing to see.

Strange, so in this case maybe the screen reader read invisible information. If I am correct with file names, when I ask the temperature: Screenshot_20230504-155530 And when I ask for humidity: Screenshot_20230504-164541 The screen reader announces the thermometer is off when reading the reply, same for humidity I have the voice reply telling the temperature sensor cannot be found for humidity.

Nardol avatar May 04 '23 17:05 Nardol

sorry, meant to say heat

jbouwh avatar May 04 '23 17:05 jbouwh

yes, lets only use heat as that seems to work.

jbouwh avatar May 04 '23 17:05 jbouwh

About the screenshots, I have the same. We set a flag to only query the humidity, but still it reports a missing temperature value.

jbouwh avatar May 04 '23 17:05 jbouwh