Skip to content
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

Enhance temperature regulation when working with internal device temperature #453

Conversation

jdeksup
Copy link
Contributor

@jdeksup jdeksup commented Apr 23, 2024

Hi,
I use Versatile thermostat from several weeks and really appreciate it to regulate my climate devices that have a bad thermal regulation because of the position of the sensor. Thank you for your big work on this.
I suggest a small adjustment : a behavior that prevents the system from forgetting the regulation when using the device's internal temperature as an offset.

Although the regulation itself remains unchanged, variations in the device's internal temperature necessitate adjustments to the target temperature to maintain effective regulation. This code modification specifically bypasses the forget mechanism for minor adjustments, while the forget mechanism based on duration is still maintained.

Here is an history of the underlying climate before :
image
and after (with 2 minutes of interval which is fast) :
image

@jmcollin78
Copy link
Owner

jmcollin78 commented Apr 23, 2024

Hello @jdeksup ,

I understand the idea and it could be interesting. It has one important drawback: we should avoid to send to much regulation command (setpoint change) on some equipment like TRV. Else we will drawn the battery very fast. (see some issue relative to this particular point).

This threshold have been done to avoid this battery drawn and I fear that your improvement will rollback this feature.

Without touching the code, you can do what you want by setting the dtemp threshold to 0, I guess. So it is configurable Vtherm by Vtherm and not forced for every one.

@jdeksup
Copy link
Contributor Author

jdeksup commented Apr 23, 2024

I understand your point.
I already tried to configure dtemp threshold to 0 and got an error "assert x > 0" on round_to_nearest method,
So I just update my pull request to fix this and it works.

…tion send when using underlying device temperature for offset
@jdeksup jdeksup force-pushed the feature/autoregulation-send-for-underlyingtemp branch from 7b23a12 to 76c84be Compare April 23, 2024 21:35
@jmcollin78
Copy link
Owner

I understand your point. I already tried to configure dtemp threshold to 0 and got an error "assert x > 0" on round_to_nearest method, So I just update my pull request to fix this and it works.

This seems good. Can you please add a unit test with dtemp equal to 0 to prove the correct behavior ?
You have a good example in the test method:

async def test_over_climate_regulation(
    hass: HomeAssistant, skip_hass_states_is_state, skip_send_event
):

in the tests/test_auto_regulation.py. Of course I can help if needed.

@jdeksup
Copy link
Contributor Author

jdeksup commented Apr 30, 2024

Hi,
I added a unit test based on your suggestions.
It tests standard regulation with dtemp = 0, and tests if a small temperature change is taken into account.

Copy link
Owner

@jmcollin78 jmcollin78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jdeksup

@jmcollin78 jmcollin78 merged commit 549423b into jmcollin78:main May 13, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants