-
-
Notifications
You must be signed in to change notification settings - Fork 49
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
Fix the frequency of AC temperature update #292
Conversation
src/devices/AirConditioner.ts
Outdated
@@ -663,7 +663,7 @@ export default class AirConditioner extends baseDevice { | |||
type: ValueType.Range, | |||
min: 0, | |||
max: 0, | |||
step: 0.01, | |||
step: 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we use 1
in step, some AC with 0.5 step not worked correcly, and fahrenheit unit not worked too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway to detect the step from AC model json? I looked but I couldn't find it.
If not then it could be added in a map for each model.
As for Fahrenheit, step is still one my devices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway to detect the step from AC model json? I looked but I couldn't find it.
I couldn't found anything too
If not then it could be added in a map for each model.
I tried this way before, but somehow, I got some device same model number but different step (as I remember)
As for Fahrenheit, step is still one my devices.
in Home app, you can adjust fahrenheit temperature in step 1 by 1, but Homekit always handle it in celcius, it's mean they will convert fahrenheit value to celcius and send it to server
btw, I have config for AC ac_temperature_unit
, we can use this config value to change this step value I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you prefer if I remove the stepping change for now and proceed with the PR and create a dedicated PR for step changes?
It seems that it will require a better look to make sure it's compatible with wide range of devices and to make sure that temp conversion handling is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was mistaken about the Fahrenheit unit, a step of 1 or 0.01 works perfectly with Fahrenheit, but not with Celsius, for example people can't set temperature to 23.5 *C even their AC supported that range
so yes, we need revert it to step 0.01
to make sure it's compatible
what problem do you have about that step? we can discuss more
I tried to change temperature display unit in Home app to Fahrenheit and everything still look good even with step 0.01
This reverts commit fe2fc8a.
This PR fixes an issue with the temperature update where it takes a lot of time to update sometime up to hours or days without opening the LG app.
The correct endpoint to send MQTT commands to the device is /control not /control-sync as implemented on this fix.
The fix implements the following as well:
Please review and let me know if things could be implemented in a better way and I would be happy to update as needed.