Fix #65: cannot obtain values from entity that exists

[pfefferle]

Description

There seems to be an issue on the latest version of the skill that is descibed here: #65

Type of PR

If your PR fits more than one category, there is a high chance you should submit more than one PR. Please consider this carefully before opening the PR. Either delete those that do not apply, or add an x between the square brackets like so: - [x]

• [X] Bugfix
• [ ] Feature implementation
• [ ] Refactor of code (without functional changes)
• [ ] Documentation improvements
• [ ] Test improvements

[Tony763]

Hi, I already proposed fix to this in issue #65 but nobody tested it. I think, base problem is when no unit is returned from HA, ha_client.py:116 sets it as None but in __init__.py:481 we check if unit variable is empty string. Rather than add next check I would like to synchronize these values.

[pfefferle]

@Tony763 I tested your branch, but got the same issue.

[pfefferle]

I also changed line 128 to return "", but this didn't work out either.

[Tony763]

@pfefferle I digged in and I think I found what is going on. I misinterpreted 'uncertainty', and maybe also author of this. Uncertainty is not how much Quantulum is certain about unit name, but it is number uncertainty aka 1.2 s ± 0.1 s. Function parse will return uncertainty only when it's found in text (Temperature is set to 22±1 ˚C) else it return as None. From HA we will get only certain values so we do not need whole check. quantity.unit.name != "dimensionless" should be everything that is needed. If Unit is not found in known units, quantity set unit name to 'dimensionless' and we just keep original one returned from HA or empty string if unit is not set in HA.

if len(quantity) > 0:
  quantity = quantity[0]
  if (quantity.unit.name != "dimensionless"):
    sensor_unit = quantity.unit.name
    sensor_state = quantity.value

Test: With dimension (˚C):

Give me value of Mycroft sensor                                                                                                                                             
 >> Mycroft sensor has the state: 122 degree Celsius. 

Dimensionless (in HA set without unit):

Give me value of aqua temp                                                                                                                                                               
 >> !aqua_temp is 24.37 . 

Quantulum debug:

Original text: "Mycroft sensor is 122.0 °C"
 Clean text: "Mycroft sensor is 122.0 °C"
 Text after numeric conversion: "Mycroft sensor is 122.0 °C"
 Quantity found: {'value': '122.0', 'unit1': '°C'}
 After exponent resolution: 122.0
        Uncertainty: None
        Values: [122.0]
        Unit: degree celsius
        Entity: Entity(name="temperature", uri=Temperature)
        Initial span: (18, 26) ("122.0 °C")
        Shifted span: (18, 26) ("122.0 °C")
        Final span: (18, 26) ("122.0 °C")

 Original text: "!aqua_temp is 24.37 "
 Clean text: "!aqua_temp is 24.37 "
 Text after numeric conversion: "!aqua_temp is 24.37 "
 Quantity found: {'value': '24.37'}
 After exponent resolution: 24.37
        Uncertainty: None
        Values: [24.37]
        Unit:
        Entity: Entity(name="dimensionless", uri=Dimensionless_quantity)
        Initial span: (14, 20) ("24.37 ")
        Shifted span: (14, 20) ("24.37 ")
        Final span: (14, 19) ("24.37")

[Tony763]

@pfefferle We also want to change ha_client.py:116 unit_measur = '' It fix dialog messages saying None in dimensionless thermostats. (I see it now why it did not work for you as I only fixed first part of problem and waited to tests result :wink:)

Without change:

set Mycroft climate to 20 degrees
 >> Successfully set Mycroft climate to 20 None.

With:

set Mycroft climate to 20 degrees 
>> Successfully set Mycroft climate to 20 .

So what do You think? Could You test these options, please?

[pfefferle]

Seems to work, I get the result Temperatur Wohnzimmer ist 25 und 2 fünftel degree Celsius.

But I would recommend to also change line 128 to return "".

[Tony763]

But I would recommend to also change line 128 to return "".

Hi @pfefferle I would not recommend that. With line 116 you insert empty string into variable that expect string data in next processing. But line 128 correspond with line 127 where you return whole dictionary so with empty string you would mix two types of variables in next processing. Maybe it not causing any trouble now, but changing it will not bring anything.

[pfefferle]

@Tony763 valid point! changed the PR based on your recommendations!

[Tony763]

Hi @krisgesling, could you check this PR? It should be ready to go.

[Tony763]

@pfefferle do you work with git rebase? Would by good to squash commits to keep commit history clean. But it's optional.

[Tony763]

Hi @krisgesling, could You recheck this, please?

[krisgesling]

Looks good to me.

I'm not actively running Homeassistant right now, so I'm presuming that someone has given this a functional test?

You can also squash the commits on merge if that's easier - hit the little down arrow on the right of the "Merge pull request" button and it gives you the option to "squash and merge".

[pfefferle]

I am so sorry, I totally missed your question @Tony763 :(

@krisgesling I am running Home Assistant and the latest version of the skill and had no issues so far. Lights and Climate entities working fine!

[Tony763]

Hi @pfefferle, I recheck this and found two things. As always, when I asked to change to from None to '' I forget to mention that also compare symbols need to be change: is None to =='' in lines 380 and 402, my apologies. :slightly_smiling_face:

Second is that when I ask: Give me the value of thermostat kids room heater Mycroft will complain that entity was not found. It is due to missing 'climate' in line 458. It should be: ha_entity = self._find_entity(entity, ['climate', 'sensor', 'switch']

With that Mycroft will find entity, but it will return it's state instead aka 'on', 'off', 'auto'.

As OP mentioned in issue #65 , it should return value of actual or targeted temperature. For that, there will be a bigger change needed, so I would like to do it in separate PR and not blocking this, one.

[pfefferle]

@Tony763 do you want to add your changes directly to this PR? I can give you access to my fork if needed?

[Tony763]

@pfefferle I have it almost ready, I will test it a little and send it tomorrow as PR to your fork, so You check changes before merge.

[Tony763]

Hi @pfefferle, PR sended.

give me value of kids room heater
>> Kids room heater is off, temperature is 22 and set to 25.

give me value of kids room heater
>> Kids room heater mode is auto, current temperature is 22 and targeted 25.

give me value of kids room heater
>> Kids room heater is unavailable.

[Tony763]

Hi @antipiot, @kamir86, would You help with translation, please? We need to translate new dialog for sensor thermostat. https://github.com/pfefferle/skill-homeassistant/tree/fix-65 'dialog/en-us/homeassistant.sensor.thermostat.dialog'

[pfefferle]

@Tony763 I checked an merged your PR and also added the german translation. It works like expected!

@krisgesling can you have another look?

[pfefferle]

@krisgesling @Tony763 added all changes, based on your feedback :)