joeyhage / homebridge-alexa-smarthome

Connect Alexa devices to HomeKit
MIT License
62 stars 20 forks source link

feat (#82): Added support to relative humidity levels for thermostats #92

Closed nyirsh closed 6 months ago

nyirsh commented 6 months ago

In reference to feature request #82

This is just one of my first times playing around with homebridge plugins so there might be better ways to accomplish this, but the implementation seemed pretty straightforward to me. Consider this as a PoC for I'm not totally sure on what would happen if a thermostat doesn't measure humidity (to my understanding HB normalizes missing/wrong values so actually nothing bad?).

While checking the logs generated by logDeviceInfo for my thermostat current state, I noticed this:

[02/01/2024, 15:46:33] [homebridge-alexa-smarthome] Thermostat ::: Current state: [
{
...
{
    "namespace": "Alexa.HumiditySensor",
    "name": "relativeHumidity",
    "value": 37
  },
...
}]

I then followed what handleCurrentTempGet did to retrieve the current temperature and made a modified copy of it to wotk on the Alexa.HuniditySensor namespace and, sure enough...

92D5B841-B8E4-489C-9BF7-E2E035CC1C16_1_102_o

nyirsh commented 6 months ago

In reference to https://github.com/joeyhage/homebridge-alexa-smarthome/issues/79#issuecomment-1874367766

I understand that the plugin keeps all of the thermostat states in cache and that the cache is refreshed every X number of seconds. @louis-test likely turned off his thermostat before the cache was refreshed and caused the error, which is also how I was able to replicate his issue.

Despite not seeing accurate information at the time of changing the temperature, one could argue that the desire of the user was to keep whatever mode was displayed (assumed choice) and just adjust the temperature of it.

I changed the code so that, before setting the new target temperature, it would retrieve the current mode displayed on HK (cached) just to try to set that same mode right after, assuring that the combination of mode / temperature selected by the user is preserved.

joeyhage commented 6 months ago

@nyirsh thanks again for the PR! Humidity sensor code looks great. I'm hesitant to merge the change related to #79 (comment) because this will affect every request to set the temperature, resulting in longer loading times.

It seems to me the reported comment won't be an issue for the vast majority of situations because it would be odd to change the mode to off using a physical button or the alexa app and then go into the iOS home app to change the temperature just a few seconds later. Thoughts?

nyirsh commented 6 months ago

I can totally drop it, I'm honestly not a big fan of the solution either.

If there was a way to tell if the user actually clicked on the thermostat widget in HK I would have ignored the cache remaining lifespan and forcefully reloaded them because that's when the user is actually getting in position to manually change the temp/mode. Not sure if that's even possible tho (from the few experiments I ran so far it's not)

If you want to still give the users the choice, I could probably add a flag in the settings with a big warning, something like "Thermostats - Override mode when temperature is set (AFFECTS PERFORMANCE)" or similar.

joeyhage commented 6 months ago

@nyirsh I'll keep that in mind, for now I would rather wait to see if others report the same issue seeing as the person who reported it knows a workaround.

Also, I made some formatting changes and added a conditional so humidity is only checked if the device supports it. Are you able to test what is currently in the main branch before I create a release?

joeyhage commented 6 months ago

Please and thank you. The thermostat I own doesn't expose the humidity sensor via the Alexa Skill so I want to make sure it still works for you. Also, I made some improvements to how temperatures are changed when the thermostat mode is auto.

nyirsh commented 6 months ago

I just tested it and it seems to work just fine!

joeyhage commented 6 months ago

@nyirsh awesome, thanks! I’ll publish another release tonight then with Humidity support. Closing this PR then since your Humidity commit is already in main.