Add temperature precision override for tuya platform #43970
Conversation
|
Hi @matitalatina, It seems you haven't yet signed a CLA. Please do so here. Once you do that we will be able to review and accept this pull request. Thanks! |
matitalatina
force-pushed
the
tuya-climate-override-precision
branch
from
8d589cd
to
d13794a
Compare
| @@ -40,7 +40,8 @@ | |||
| "support_color": "Forzar soporte de color", | |||
| "temp_divider": "Divisor de los valores de temperatura (0 = usar valor por defecto)", | |||
| "tuya_max_coltemp": "Temperatura de color m\u00e1xima notificada por dispositivo", | |||
| "unit_of_measurement": "Unidad de temperatura utilizada por el dispositivo" | |||
| "unit_of_measurement": "Unidad de temperatura utilizada por el dispositivo", | |||
| "precision_override": "Precisi\u00f3n de temperatura" | |||
There was a problem hiding this comment.
Do not provide additional translations. Just adjusting string.json is enough, the rest is handled by the translation platform.
Please remove all additional translations.
There was a problem hiding this comment.
Ops, it was easier than I thought.
Added in string.json and removed from translations.
There was a problem hiding this comment.
Since I removed all the translations and I kept only strings.json, I don't see any labels during testing in my local environment. Is that ok? How can I know that it will work?
I'm just worried.
If it's OK, please resolve this thread.
Thank you for your time.
There was a problem hiding this comment.
@ollo69 please help me here, too.
…from translations
|
I've proposed something similar in #43666, but I think it's better to rely on tuyah to get the precision, and leave HA only change that, no? |
|
Ah not only, this is not enough: you may see the right temperature, but not set it properly... You'd need https://github.com/PaulAnnekov/tuyaha/pull/54/files#diff-63c27f5cccf70e2d762795023d3a91279fcb4fb280ecd06da23a7d1a2b025590R167 |
I just followed the design that someone else used to fix the "wrong temperature" issue in tuya platform. Add the ability to override the behavior if it's wrong. I suspect that this platform handles so many devices that it is difficult to get it right automatically. I think that both PR can live together: your PR tries to get the precision automatically. Mine allows to override it if it's wrong again.
I didn't change the "set temperature part" because I can't test if it's working if you put, for example, 18.3 to a thermostat that supports only halves. I tried to be as more conservative as possible. The issue I try to solve here is reading the value that is different from what I see on the thermostat. BTW I can take some time to test it. Thank you for your help. |
|
I want to take some time to fix also the "set temperature" part. As suggested @3v1n0 |
|
@3v1n0 I can't override the "set temperature part" without touching the external tuya library. |
|
My opinion is that is normally better to work on library, on HA is not allowed in many case to force values provided by library. |
|
Now the precision override works also when we set the temperature. |
If you need upstream change you have to wait for upstream PR merged and new library version released and than update the library version used by the component in |
matitalatina
force-pushed
the
tuya-climate-override-precision
branch
from
45731fc
to
c944b0c
Compare
|
Waiting for the PR on external tuya library PaulAnnekov/tuyaha#59 |
| if self._override_precision != PRECISION_DEFAULT: | ||
| return self._override_precision |
There was a problem hiding this comment.
These days I have been thinking about this PR, target_temperature should be managed separately because could have different value not compatible with precision.
You should remove this part and continue with this PR that in this case will not require TuyaHA library change. We will manage the target_temperature with a separate PR.
There was a problem hiding this comment.
But just thinking more ... why this is required? What happen if we just set precision to PRECISION_TENTHS? There are some undesired side effect? I think that we are making this problem more complex. Thoughts?
There was a problem hiding this comment.
My thermostat needs also to be tweaked on target temperature step. In fact, I can only set one unit. But my thermostat support 0.5 step.
With this override I can put also 0.5 step (e.g. 18.5 C). But I realized that I need also this change in the library because there's a round(number) that prevent me to set 0.5 values.
So with both these changes I can set 17.5C 😊.
There's a drawback, as you pointed out: what happens if you set PRECISION_TENTH and your thermostat support only HALVES? My thermostat puts the lower possible bound (target temperature 17.9 C -> it sets 17.5C).
You should put the right precision on your thermostat. If you put higher precision, you're doing it wrong, but it's not a tragedy if all thermostats behave like mine (I obviously can't test it).
There was a problem hiding this comment.
I think that precision controls the visualization, temperature_step the step used to set the target. This 2 value is not related, you can set PRECISION_TENTH for precision and PRECISION_HALVES (0.5) for temperature_step and everything will work as expected. So i think that leaving PRECISION_TENTH for precision is not an issue at all, it is only important to set proper value for temperature_step.
The changes to the library proposed by you can potentially break other devices that do not accept decimal value in the payload, so should be reviewed.
I already implemented in Tuya Custom all required changes, in options page you can change independently precision and temperature_step and also contains local library changes to properly manage the set temperature command with decimal values. Please temporally replace standard integration with this custom component to perform test with all possible combination, than we will choose the correct change to implement.
There was a problem hiding this comment.
Thank you for the custom library. It is easier to test.
But I don't need it, if I understood correctly.
We decided to add the precision override only in "read mode", so I revert the temperature step part and set temperature.
In that way, I don't need to change the external library, it's safer and easier to digest as pull request.
Is that ok?
If we agreed, I'll revert the changes and we fix the "set the temperature" part in another pr.
There was a problem hiding this comment.
Not exactly. My idea is that create an option to override precision is not required, because we can simply set to PRECISION_TENTH and forget it.
Than we need to fix temperature_step, and for this we need PRs for both TuyaHA and HA.
I already made changes in custom component, but I don't have a climate so I need confirmation that this solve the issue and work properly before pushing correct PR.
There was a problem hiding this comment.
I tried all possible things.
All worked well except these:
-
If I don't check "Use divided Temperature value for set temperature command", the thermostat becomes unavailable and the logs say
Logger: custom_components.tuya_custom.tuyaha.tuyaapi Source: custom_components/tuya_custom/tuyaha/tuyaapi.py:273 Integration: Tuya Custom (documentation) First occurred: 5:50:03 PM (1 occurrences) Last logged: 5:50:03 PM control device error, error code is ValueOutOfRangeI have divider 2.
-
If I select a different sensor in "Sensor for current temperature", I don't see the change in UI. If I go to my lovelace thermostat I always see the thermostat temperature and not the sensor I put in the select box.
This behavior is OK for me, but it worth mentioning:
- If I put temperature_step to tenth even if my thermostat support halves, it sets the first lower bound acceptable. (E.g 18.9 -> it sets 18.5). This behavior is the same as I got in my PR.
There was a problem hiding this comment.
Thanks for your test.
- About point 1, it is correct because is to manage different devices behavior. But probably I have to set default to True because seems the most frequent cases
- Point 2 is only for devices that do not provide current temperature info, and anyway I cannot apply this change in official component because it is not allowed
- Point 3 is ok, as you said is acceptable. Just wondering if tenth step make sense, but can be left if required, because we are not able to determinate the correct step from device payload.
I'm going to do some additional changes:
- I will set default to true for point 1 as stated before
- I will remove the option to change the precision, I think set it always to tenth is OK (less option, less confusion).
- I will try to manage possible temperature step as library option because I'm not sure that is allowed define step directly in HA.
It would be great if you will make again some test when new version will be released.
There was a problem hiding this comment.
Tested with the new version
- I confirm it's true by default "Use divided Temperature value for set temperature command".
- The integration is working as expected.
I had an issue in changing the divider: I changed the value, but the value on UI didn't reflect the changes. I restarted HASS and it worked again. In my log I had an issue with "limit login reached". Maybe it simply lost the connection and so he could not update the UI.
Now it's working
There was a problem hiding this comment.
Limit login can occur during 1st configuration. It is again a Tuya API limitation.
That's fine, I will start pushing PR with new changes to TuyaHA later tomorrow.
| @@ -110,6 +112,7 @@ def __init__(self, tuya, platform): | |||
| self._def_hvac_mode = HVAC_MODE_AUTO | |||
| self._min_temp = None | |||
| self._max_temp = None | |||
| self._override_precision = PRECISION_DEFAULT | |||
There was a problem hiding this comment.
Please rename self._precision_override
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
What happened to this PR?! @frenck |
|
@majuss There are comments open, not being addressed and since the activity is running stale, it will be cleaned up after a month of 0 activity. |
|
Thank you for the quick answer. So how can I help out? Do I need to open a PR myself? |
|
We are blocked by this upstream pull request :( |
|
Part of this PR target ( This PR is in conflict with PR #44148 and should be closed. |
|
Thank you guys. I'm closing this one. |
Proposed change
Tuya platform has some issues in temperature rounding for some devices.
For example, my BTH-002-GCLW thermostat is rounded to unit (18, 19, 20) even if it should show in HA halves rounding (19.5, 20, 20.5).
I'm not the only one with this issue as stated in this long thread.
So I decided to fix not only for myself, but I'm happy to contribute to this awesome project.
I simply added an option that let me override the default behavior.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: