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

Remove hardcoded timeout values #679

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

linnik
Copy link

@linnik linnik commented Oct 26, 2021

This PR removes hardcoded timeout values in all backends in favor of default BaseBleakClient._timeout or kwargs timeout=... argument.

Otherwise it would be impossible to connect with slow devices even if timeout option is given:

Traceback (most recent call last):
  File "/usr/lib64/python3.9/asyncio/locks.py", line 226, in wait
    await fut
asyncio.exceptions.CancelledError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/lib64/python3.9/asyncio/tasks.py", line 492, in wait_for
    fut.result()
asyncio.exceptions.CancelledError

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/user/.../test.py", line 70, in <module>
    asyncio.run(main(address))
  File "/usr/lib64/python3.9/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/usr/lib64/python3.9/asyncio/base_events.py", line 642, in run_until_complete
    return future.result()
  File "/home/user/.../test.py", line 30, in main
    await client.connect(timeout=60)
  File "/home/user/.../__pypackages__/3.9/lib/bleak/backends/bluezdbus/client.py", line 317, in connect
    await self.get_services()
  File "/home/user/.../__pypackages__/3.9/lib/bleak/backends/bluezdbus/client.py", line 592, in get_services
    await asyncio.wait_for(self._services_resolved_event.wait(), 5)
  File "/usr/lib64/python3.9/asyncio/tasks.py", line 494, in wait_for
    raise exceptions.TimeoutError() from exc
asyncio.exceptions.TimeoutError

@linnik linnik force-pushed the develop branch 4 times, most recently from 451aace to 11ed9f4 Compare October 26, 2021 20:50
@hbldh
Copy link
Owner

hbldh commented Oct 28, 2021

All the timeout values that you change here are not critical timeouts, are they? They are merely waiting for the event in question to be set to continue the program? For instance, it will wait for the device to disconnect, which wil not be improved by waiting longer...

The one I can see could be important to change is the get_services-timeout in the BlueZ backend and maybe the read_characteristic in the macOS, but if you haven't gotten a response in 5 seconds I don't think it is useful to wait any longer... The disconnect does not need to be increased in any circumstance I think.

@@ -90,7 +90,7 @@ async def connect(self, **kwargs) -> bool:
"""Connect to the specified GATT server.

Keyword Args:
timeout (float): Timeout for required ``BleakScanner.find_device_by_address`` call. Defaults to 10.0.
timeout (float): Defaults to 10.0.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps specify the unit as seconds; so "timeout (float): Defaults to 10.0 seconds."

"""Disconnect from the specified GATT server.

Keyword Args:
timeout (float): Defaults to 10.0.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps specify the unit as seconds; so "timeout (float): Defaults to 10.0 seconds."

@@ -575,6 +581,9 @@ def mtu_size(self) -> int:
async def get_services(self, **kwargs) -> BleakGATTServiceCollection:
"""Get all services registered for this GATT server.

Keyword Args:
timeout (float): Defaults to 10.0.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps specify the unit as seconds; so "timeout (float): Defaults to 10.0 seconds."

"""Disconnect from the specified GATT server.

Keyword Args:
timeout (float): Defaults to 10.0.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps specify the unit as seconds; so "timeout (float): Defaults to 10.0 seconds."

@mdxs
Copy link
Contributor

mdxs commented Jul 24, 2022

@hbldh As I read this PR code changes, it just tries to remove hard-coded timeouts with something that can be specified. If not provided it defaults to 10.0 seconds; perhaps you would rather see that default timeout be set to 5.0 seconds?

Then as I read the PR it would allow someone to specify the timeout if so desired, to a longer or a shorter timeframe.

@dlech
Copy link
Collaborator

dlech commented Jul 25, 2022

it just tries to remove hard-coded timeouts with something that can be specified

I think the question is: is there an actual technical problem that is fixed with a different timeout or are we just doing this for the sake of making everything configurable?

@mdxs
Copy link
Contributor

mdxs commented Nov 18, 2022

Though I'm not the OP, for me this looks like something that could help me in testing. The use case being that I know something about the Device Under Test (DUT) such as a unique name or the MAC address from reading a NFC tag or scanning a QR code; after which I would like to connect to the DUT via BLE within a given amount of time, then discover its services within another given amount of time. The purpose being to figure out if the DUT is able to meet certain minimal criteria (which of course are parameters in a test script to be executed against the DUT when the test starts).

Thus for me the "make everything configurable" does seem interesting enough indeed; the alternative being of course that I would schedule time-outs differently (another thread running a clock and scheduling the time-out for each part of the test); and then if the DUT doesn't meet the test, I will "fail" it and terminate the DUT in some way... which may cause some side effects, so more graceful controlled time-outs sound interesting.

But my use case might be very specific and not fit the purpose of bleak, so it is just an input for your consideration of course.

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.

4 participants