Eq3btsmart more reliable

[karlkar]

Description:

Applying these changes makes eq3btsmart thermostat remember the temperature/operation mode set by the user.
During update (performed every minute) it performs a check if the current thermostat settings are the same as user wants them to. If not, temperature/operation mode are changed.

After applying the changes temperature/operation mode are set almost immediately in most cases, as update is performed after every set_temperature/set_operation_mode and repeated in case of failure 2 times.

Code was running on my local HA instance for about 1 week and everything was working much better than before.

Related issue (if applicable): fixes #11554

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

Example entry for configuration.yaml (if applicable):

Checklist:

• The code change is tested and works locally.

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

• Local tests with tox run successfully. Your PR cannot be merged unless tests pass
• New dependencies have been added to the REQUIREMENTS variable (example).
• New dependencies are only imported inside functions that use them (example).
• New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
• New files were added to .coveragerc.

If the code does not interact with devices:

• Local tests with tox run successfully. Your PR cannot be merged unless tests pass
• Tests have been added to verify that the new code works.

[houndci-bot]

line too long (92 > 79 characters)

[houndci-bot]

line too long (108 > 79 characters)

[houndci-bot]

visually indented line with same indent as next logical line

[houndci-bot]

trailing whitespace

[houndci-bot]

continuation line over-indented for visual indent

[houndci-bot]

trailing whitespace

[houndci-bot]

trailing whitespace

[houndci-bot]

visually indented line with same indent as next logical line

[balloob]

This is not a good change. If the target temperature is changing outside of Home Assistant control, it's not up to Home Assistant to enforce it's target temperature. Instead, Home Assistant should just adopt the target temperature. It's up to the user to write automation if they want to enforce a specific target temperature.

[karlkar]

@balloob
You are right. My changes were disallowing any external change of temperature. I must admit I was not thinking about this case, nor tested it, as my thermostat is in a place, which is not easily accessed. However I've fixed it already.
Currently home assistant will force the temperature setting until it is set as user wanted in HA. If it will notice that the setting is applied, it won't enforce the setting anymore.
You may be sceptic about this "until" part, however in practice - after a week of tests in my environment, with many temperature changes it was taking up to 1 minute (in 80% of cases less than 10 seconds) to successfully set the temperature. So assuming user won't set the temperature in HA, and right away go to the thermostat to change the setting manually, it will work and not cause any issues. Even in that case - if user will set the temperature in HA, and right away change the temperature manually - in real life scenarios he will set manually the same temperature that he did in HA. So even if HA overwrites the setting it will be fine.

In case of operation mode this was not the issue, as my changes were assuming turning on "Boost" mode which turns back to "Manual"/"Auto" automatically after few minutes. HA with my changes was allowing it to change back from "Boost" and it was working fine.

[tinloaf]

While I have the same objection as @balloob , I was actually thinking about implementing something like this myself. That bluetooth connection is really unreliable. And there's only like two meters between my raspberry pi and the thermostat.

I think "retry until the requested value was reported back once, then behave as before" might be a good compromise. Also, one should probably make this configurable, and set the default to False. This avoids making this a breaking change. Haven't looked at the code yet, but if we can agree that this kind of retry logic is acceptable, I'll happily review this.

[rytilahti]

So, I have discussed about this topic now multiple times on different topics (wrt eq3bt, ble tracker, mi flora), and instead of simply stating that it's a problem that has to be fixed on a higher level, e.g. by doing bluetooth access sequentially I decided that it's time to do some non-scientific measurements on it.

For that I created a small script which connects to the device, fetches its state and disconnects, and does this first sequentially for a hundred time per device. Then the same test is repeated with multiple thread setups to verify if the hypothesis about thread usage / concurrent access could indeed the problem.

The script can be seen in https://gist.github.com/anonymous/8e303b8a01e9fc9c44cbc08e3a96cfdc . To try this you need to modify eq3bt/connection.py to raise the exceptions on line 78, and you may also want to suppress some logging output, too. Do not forget to add your mac addresses to macs in the beginning.

I run this script on a raspberry pi 3 against two thermostats with no other bluetooth-accessing programs active (besides bluetoothd). Results:

$ python testeq3.py 
total time: 61.239463567733765
success: 200/200
mean: 0.3061171519756317, median: 0.20590782165527344, max: 4.398439407348633
==========
with 1 threads
total time: 92.96748900413513
success: 200/200
mean: 0.46429364204406737, median: 0.2055732011795044, max: 7.7488977909088135
==========
with 2 threads
total time: 22.192440509796143
success: 173/200
mean: 0.2511330119447212, median: 0.20197701454162598, max: 4.22044563293457
==========
with 3 threads
total time: 6.076045274734497
success: 11/200
mean: 0.5811628211628307, median: 0.21947693824768066, max: 2.570474147796631
==========
with 5 threads
total time: 6.104005813598633
success: 2/200
mean: 4.334805250167847, median: 4.334805250167847, max: 6.091694355010986
==========

Some remarks:

• As can be seen, using two threads already drops the success rate, which gets much worser when there are three concurrently accessing threads.
• For some reason the thermostats start accepting connections / doing connections much faster after the initial connections, the reason is unknown.
• Whether the limitation is in bluetooth of raspbi3 or in the thermostats themselves, this test seems to indicate that there is indeed a problem when accessing the device parallel. As some people have stated elsewhere, they have had better success e.g. using an external dongle for bluetooth tracking, which may suggest a problem on the raspbi's bt stack.

So based on this, my suggestion remains the same as before. Assuming this is really a limitation on the hardware / bluetooth stack of the Pi, the bluetooth accesses done by homeassistant should be done sequentially (no matter what backend lib they are using, if this is not a bluepy limitation, which I doubt). Looking at results with 2 threads this would indicate that if 15% failure rate is accepted, up to 2 btle connections can be made on pi3 concurrently. It has to be noted though, that the execution ordering in threaded use was not enforced (if it's not done by the threadpoolexecutor), which may skew the results.

What could be interesting however would be the following:

• Try this on more bluetooth devices concurrently, it's likely unfair try to make that many connections on the same devices.
• Try this on other bt(le) devices.
• Try this on other bluetooth backend libs.
• Try this with a bluetooth dongle or multiple thereof.
• Introduce some bluetooth noise (enabling scanning in the background?)

Please feel to experiment and report back on your findings & ideas :-)

edit: to the original proposal, it may very well make sense to batch the change to happen during the upcoming update instead of trying to do it like it is currently done.

edit2: on related note to miflora, I think I have seen recommendations to use a miflora-mqtt gateway and let homeassistant fetch from it. That would corroborate that the issue is indeed with concurrent accessing.

[rytilahti]

python-eq3bt 0.1.8 raises the exceptions like it should, so no need to modify the connection.py for testing this.

[karlkar]

@balloob
So, do you agree on applying this changes?

[zewelor]

In case someone finds it useful, I've wrote simple eq3 <-> mqtt gateway that works stable for me. Its doing sequential updates over defined thermostats ( and possible other bt devices like xiaomi scale ). I hope you don't mind hijacking thread. https://github.com/zewelor/bt-mqtt-gateway

[balloob]

Current implementation is ok as it is only temporarily enforced.

[balloob]

@rytilahti I would be 👍 if we come up with our own bluetooth controller that enforces sequential access. Would just be a pain to make sure that all integrations use it 😞 makes integrations no longer generic.

[quenbert]

Thanks @karlkar and @rytilahti for the work done on eq3bt in 0.64.0, the valves management is indeed much more reliable now!

[rytilahti]

It is all thanks to @karlkar, I was merely an observer here :-)