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

Add transport_timeout_s parameter for setup and adb_connect methods #304

Merged
merged 3 commits into from Feb 14, 2022

Conversation

ollo69
Copy link
Contributor

@ollo69 ollo69 commented Feb 13, 2022

In most case during initial connection to my FireTV devices I receive back the error:

[androidtv.adb_manager.adb_manager_async] Couldn't connect to 192.168.10.18:5555.  TcpTimeoutException: Connecting to 192.168.10.18:5555 timed out (1.0 seconds)

With this PR it will be possible to increase a little bit the transport_timeout used in the connect method that is currently hard coded to 1 second.

@ollo69 ollo69 changed the title Add connect_transport_timeout parameter for setup and adb_connect Add transport_timeout parameter for setup and adb_connect methods Feb 13, 2022
@JeffLIrion
Copy link
Owner

I don't know why this exception is happening. Here's where it is in the code: https://github.com/JeffLIrion/adb_shell/blob/bf4d348e3aa0999b24976de9bac442b0e180a27e/adb_shell/transport/tcp_transport_async.py#L75-L79

I think 1 second should be plenty of time. Have you tested your changes and verified that allowing more time avoids the exception?

@ollo69
Copy link
Contributor Author

ollo69 commented Feb 13, 2022

Error normally occurs only during initial setup. I think that could be related to the hw of the fire TV stick that probably is not so fast. It fails on first connect, second retry that is normally fired after few seconds by HA ends without error.
And yes, I tested changes in HA and just increasing the timeout to 2 seconds seems to solve the issue.

@JeffLIrion
Copy link
Owner

Just to be clear, when you say "initial setup," are you talking about config flow or at every HA restart?

Also, a simpler change would be to increase the hard-coded value from 1.0 to, say, 5.0. Of course, adding a parameter allows for greater flexibility. I think either approach is probably fine.

@ollo69
Copy link
Contributor Author

ollo69 commented Feb 13, 2022

Talking about every HA restart, so when setup is called.
I agree with you that increase the hard-coded value is simpler, but I wouldn't change current default value. I also defined a max value (5 sec) to avoid set this timeout to excessive high value.

DEFAULT_TRANSPORT_TIMEOUT_S = 1.0

#: Maximum allowed transport timeout (in s) for :meth:`adb_shell.handle.tcp_handle.TcpHandle.connect` and :meth:`adb_shell.handle.tcp_handle_async.TcpHandleAsync.connect`
MAX_TRANSPORT_TIMEOUT_S = 5.0
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's better to not have a max. Whatever the user specifies, just use that value.

@JeffLIrion JeffLIrion changed the title Add transport_timeout parameter for setup and adb_connect methods Add transport_timeout_s parameter for setup and adb_connect methods Feb 14, 2022
@JeffLIrion JeffLIrion merged commit 90245ed into JeffLIrion:master Feb 14, 2022
@JeffLIrion
Copy link
Owner

It would still be good to get to the bottom of this. I would think that 1 second would be plenty of time and that there must be some other issue. But if changing it to 2 seconds fixed the problem for you, that strongly suggests the problem really was as simple as 1 second not being enough time.

Let me know if you want me to make a new release.

@ollo69
Copy link
Contributor Author

ollo69 commented Feb 14, 2022

It would still be good to get to the bottom of this. I would think that 1 second would be plenty of time and that there must be some other issue. But if changing it to 2 seconds fixed the problem for you, that strongly suggests the problem really was as simple as 1 second not being enough time.

Let me know if you want me to make a new release.

That's the idea, This is not a big or urgent issue, because at the end I just have a couple of Warning message during HA restart, but I think that having a new version to be able to increase the initial timeout to 2 seconds will be better and I don't see any possible side-effect on this.

@ollo69 ollo69 deleted the connect_transport_timeout branch February 14, 2022 09:01
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.

None yet

2 participants