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

Stab at checking for OpenTherm Lite (OT/-) in response to #19 #27

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

FreeBear-nc
Copy link

Quick stab at checking for OpenTherm Lite in response to #19
Not having an OT/- boiler, can't test to see if it works.

@olegtarasov
Copy link
Owner

If I understand your code correctly, it tries to send a STATUS message to the boiler, wait for a result, and if there is no answer, assume OT/-. I'm afraid this exact implementation won't work, since send() and listen() are asynchronous, and your code clearly assumes messages are sent and received in full when send and listen are done. This is not the case, and that's why there are spin_wait_() calls in sync_loop_(). Furthermore, even if we implement this correctly, we will not pass esphome review, because in the worst case scenario we will blocking component setup() for 5 seconds. Another problem is that some boilers expect messages other than STATUS to start a conversation, while others don't support those messages.

All this leads me to suggesting a different approach. It would be better to integrate these checks into existing message pipeline: if the first message we send doesn't get a response, we can assume OT/-.

These checks need to be added anyway, so I will try to implement them in a couple of days. In the meantime I want to make a new PR to esphome to merge all the features I've added to develop so far (config messages, lambda overrides etc).

@FreeBear-nc
Copy link
Author

As per commit message, it was only a stab at addressing the issue. Status and t_set are mandatory for OT/- as well as OT/+ boilers, so it kinda made sense to my mind to use status. However, elsewhere in the v2.2 documentation, it recommends using a DEVICE_CONFIG (ID:3) and CONTROLLER_CONFIG (ID:2) exchange at start up.

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.

2 participants