-
Notifications
You must be signed in to change notification settings - Fork 302
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
base: develop
Are you sure you want to change the base?
Conversation
451aace
to
11ed9f4
Compare
All the The one I can see could be important to change is the |
@@ -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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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."
@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. |
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? |
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. |
This PR removes hardcoded timeout values in all backends in favor of default
BaseBleakClient._timeout
or kwargstimeout=...
argument.Otherwise it would be impossible to connect with slow devices even if timeout option is given: