Add configurable number of connection attempts #46

gmoelter · 2022-01-09T17:07:06Z

Fixes #34

* default value=2 (->1 retry) * add variable conn_error * implement each conn.make_request in a try-except structure and set conn_error true if an exception occurs

rytilahti · 2022-01-10T22:56:45Z

eq3bt/eq3btsmart.py

+        try:
+            self._conn.make_request(PROP_WRITE_HANDLE, value)
+        except Exception as ex:


I don't currently have a test device at hand to check this, but wouldn't it make more sense to perform the error&retry checking inside make_request or leave the whole re-try machinery for upstreams to handle?

Inside the make_request function (connection.py) possible errors are fetched. But the calling instance (eg. update function) must recognize and handle the error. That's the reason for the stacktrace.

You don't need a real test device, try it with a random mac like 11:22:33:44:55:66.

Okay, I found a test device and did some quick testing.

Now, I think it's better to let the exceptions bubble up instead of having a device global error flag (e.g., if you query multiple different things, some of them may succeed and you can still display that information to the user), and handle the exception at user's end.

In the cli, instead of checking for the the error flag, it would be better to do something like this for the @cli.group to capture all exceptions: https://stackoverflow.com/questions/44344940/python-click-subcommand-unified-error-handling/44347763#44347763

* add cli option --connectionattempts

gmoelter · 2022-01-15T13:24:08Z

After the last commits eq3cli.py also works perfect!

eq3cli.py --mac 00:00:00:00:00:00 --connectionattempts 3 state
INFO:eq3bt.connection:00:00:00:00:00:00: Connection attempt #1(3) failed
INFO:eq3bt.connection:00:00:00:00:00:00: Connection attempt #2(3) failed
INFO:eq3bt.connection:00:00:00:00:00:00: Connection attempt #3(3) failed
INFO:eq3bt.connection:Got exception from bluepy while making a request: Failed to connect to peripheral 00:00:00:00:00:00, addr type: public
WARNING:eq3bt.eq3btsmart:Failed to connect to peripheral 00:00:00:00:00:00, addr type: public
Connection error

Finally README.md has to be updated.

eq3bt/eq3btsmart.py

eq3bt/connection.py

rytilahti · 2022-02-18T17:34:48Z

eq3bt/eq3btsmart.py

+        try:
+            self._conn.make_request(PROP_WRITE_HANDLE, value)
+        except Exception as ex:


Okay, I found a test device and did some quick testing.

Now, I think it's better to let the exceptions bubble up instead of having a device global error flag (e.g., if you query multiple different things, some of them may succeed and you can still display that information to the user), and handle the exception at user's end.

In the cli, instead of checking for the the error flag, it would be better to do something like this for the @cli.group to capture all exceptions: https://stackoverflow.com/questions/44344940/python-click-subcommand-unified-error-handling/44347763#44347763

rytilahti · 2022-02-18T17:36:23Z

eq3bt/eq3cli.py

+    if not dev.connection_error:
+        click.echo("Current target temp: %s" % dev.target_temperature)
+        if target:
+            click.echo("Setting target temp: %s" % target)
+            dev.target_temperature = target
+    else:
+        click.echo("Connection error")


Suggested change

if not dev.connection_error:

click.echo("Current target temp: %s" % dev.target_temperature)

if target:

click.echo("Setting target temp: %s" % target)

dev.target_temperature = target

else:

click.echo("Connection error")

if dev.connection_error:

click.echo("Connection error")

return

click.echo("Current target temp: %s" % dev.target_temperature)

if target:

click.echo("Setting target temp: %s" % target)

dev.target_temperature = target

It is better to return early for error cases, but see my commentary above regarding to how to handle errors in a nicer way with no need for adjusting each and every command separately.

rytilahti · 2022-02-18T17:38:47Z

eq3bt/eq3cli.py

@@ -22,16 +22,17 @@ def validate_mac(ctx, param, mac):
 @click.group(invoke_without_command=True)
 @click.option('--mac', envvar="EQ3_MAC", required=True, callback=validate_mac)
 @click.option('--interface', default=None)
+@click.option('--connectionattempts', default=2)


Suggested change

@click.option('--connectionattempts', default=2)

@click.option('--retry', default=2)

Or --retries would be shorter and still understandable name for this.

That's okay for me. But 'connectionattempts' is not the same as 'retries'. The change must also be reflected in eq3btsmart.py and connection.py. See commit 603b391

rytilahti · 2022-02-18T18:08:07Z

I realized one more thing, maybe it would make sense to lower the timeout for the tries in this same PR. At the moment it takes very long to retry, and most of the times if the device has not responded in a couple of seconds it likely makes more sense to retry instead of waiting long for an answer.

…q3btsmart and connection accordingly.

gmoelter · 2022-02-21T16:14:51Z

Maybe it would make sense to lower the timeout for the tries in this same PR.

Then we should make 'timeout' configurable. OK?

gmoelter · 2022-02-25T10:33:44Z

The last few commits should address your previous comments as far as they pertain the issue this PR is supposed to solve. Hence, from my side, I believe it should be ready to be merged.

As for your other suggestions, such as the error handling overall or the question about time delay in case when a connection could not be established, I think these are certainly worth investigating. However, at the same time, they do not directly pertain the issue of this PR, which is why they should rather be the subject of a separate PR.

For instance, in order to properly solve the issue with the time delay, one has to ultimately extend the bluepy interface, since this is precisely where I believe the delay is coming from.

rytilahti · 2022-03-09T15:03:16Z

However, at the same time, they do not directly pertain the issue of this PR, which is why they should rather be the subject of a separate PR.

I agree that the timeout handling should not be a part of this PR and is something to be considered later on, but the error handling should definitely be cleaned to keep the code base clean and maintainable.

The retry logic should only involve changes inside of Thermostat if possible, especially as there will be another backend to support (#48) and maybe in the future someone contributes a backend using bleak to add support for mac and windows systems.

So, after a brief investigation, I think the best option would be using a retry library like tenacity and simply decorate all I/O doing calls inside Thermostat. I'm not sure how to pass the cli parameter to it offhand so that'll need some investigation and testing to figure out.

gmoelter added 2 commits January 9, 2022 12:51

Add Configurable number of connection attempts

3d8ef81

Add configurable number of connection attempts

a44c11a

* default value=2 (->1 retry) * add variable conn_error * implement each conn.make_request in a try-except structure and set conn_error true if an exception occurs

gmoelter changed the title ~~Fix issue #34 and add configurable number of connection attemps~~ Fix issue #34 and add configurable number of connection attempts Jan 9, 2022

rytilahti reviewed Jan 10, 2022

View reviewed changes

gmoelter added 2 commits January 15, 2022 13:10

Add property connection_error

3f07968

Recognize connection_error

1afb63b

* add cli option --connectionattempts

Update README.md

f73035d

gmoelter mentioned this pull request Jan 16, 2022

error in code when connection fails #34

Open

rytilahti reviewed Feb 18, 2022

View reviewed changes

rytilahti changed the title ~~Fix issue #34 and add configurable number of connection attempts~~ Add configurable number of connection attempts Feb 18, 2022

gmoelter added 3 commits February 20, 2022 13:33

Changed logging levels

af880ea

Merge branch 'master' of https://github.com/gmoelter/python-eq3bt

be61fe4

Rename 'connectionattempts' to 'retries' and modify code in eq3cli, e…

603b391

…q3btsmart and connection accordingly.

gmoelter closed this Mar 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configurable number of connection attempts #46

Add configurable number of connection attempts #46

gmoelter commented Jan 9, 2022 •

edited by rytilahti

Loading

rytilahti Jan 10, 2022

gmoelter Jan 11, 2022 •

edited

Loading

rytilahti Feb 18, 2022

gmoelter commented Jan 15, 2022 •

edited

Loading

rytilahti Feb 18, 2022

rytilahti Feb 18, 2022

rytilahti Feb 18, 2022

gmoelter Feb 21, 2022

rytilahti commented Feb 18, 2022

gmoelter commented Feb 21, 2022

gmoelter commented Feb 25, 2022

rytilahti commented Mar 9, 2022

	@click.option('--connectionattempts', default=2)
	@click.option('--retry', default=2)

Add configurable number of connection attempts #46

Add configurable number of connection attempts #46

Conversation

gmoelter commented Jan 9, 2022 • edited by rytilahti Loading

rytilahti Jan 10, 2022

Choose a reason for hiding this comment

gmoelter Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

rytilahti Feb 18, 2022

Choose a reason for hiding this comment

gmoelter commented Jan 15, 2022 • edited Loading

rytilahti Feb 18, 2022

Choose a reason for hiding this comment

rytilahti Feb 18, 2022

Choose a reason for hiding this comment

rytilahti Feb 18, 2022

Choose a reason for hiding this comment

gmoelter Feb 21, 2022

Choose a reason for hiding this comment

rytilahti commented Feb 18, 2022

gmoelter commented Feb 21, 2022

gmoelter commented Feb 25, 2022

rytilahti commented Mar 9, 2022

gmoelter commented Jan 9, 2022 •

edited by rytilahti

Loading

gmoelter Jan 11, 2022 •

edited

Loading

gmoelter commented Jan 15, 2022 •

edited

Loading