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

TpLink Device Tracker Error #15918

Merged

Conversation

TimBailey-pnk
Copy link
Contributor

@TimBailey-pnk TimBailey-pnk commented Aug 10, 2018

Description:

I am a Software developer (C#/NodeJS) and this is my first ever fix for Python code.
I have a TPLink EAP245 Access Point which when I configured the device_tracker was causing a Error setting up platform tplink log entry. On further investigation it seems that the code tries a number of scanners in order to try to get the user list. In my case the scanner that works is the second in the list but it never gets there due to an exception in the first.

My fix is to log any errors but just pass so that the next scanner can have a try.
I've run the tests and they all pass locally.

Related issue (if applicable): fixes #

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.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated 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:

  • Tests have been added to verify that the new code works.

@ghost ghost added the in progress label Aug 10, 2018
@TimBailey-pnk TimBailey-pnk changed the title Fix 'Error setting up platform tplink' error when a scanner fails eve… TpLink Device Tracker Error Aug 10, 2018
@awarecan
Copy link
Contributor

Thanks for the contribution, please change your PR description following our template.

@TimBailey-pnk
Copy link
Contributor Author

@awarecan Updated the PR description and fixed the build issues...

scanner = cls(config[DOMAIN])
if scanner.success_init:
return scanner
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

This try/catch should move into TplinkDevice[*]Scanner._update_info(), wrapping self.tplink_client.get_connected_devices()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only TplinkDeviceScanner uses self.tplink_client.get_connected_devices() the others use requests.get or requests.session You want me to wrap each individual call in a try except?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since scanner use a flag success_init to indicate whether initialize success, the try/catch should be done in scanner.__init__ if need.

if scanner.success_init:
return scanner
except ConnectionError:
_LOGGER.warning("ConnectionError in scanner %s", cls.__name__)
Copy link
Contributor

Choose a reason for hiding this comment

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

_LOGGER.debug() is enough, this exception was expecting since some device only be able to init as specific type of scanner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And then remove this, but use _LOGGER.debug() in these individual except blocks?

@MartinHjelmare
Copy link
Member

@TimBailey-pnk it would be awesome if you can work together with @JackNova and add support for your router in the tplink library:
https://github.com/JackNova/tplink

@TimBailey-pnk
Copy link
Contributor Author

@awarecan Pushed some changes based on your comments. Pls review

@TimBailey-pnk
Copy link
Contributor Author

@awarecan Any thoughts?

@TimBailey-pnk
Copy link
Contributor Author

@awarecan Sorry to hassle, anything more you want me to do on this?

@pvizeli pvizeli merged commit 975befd into home-assistant:dev Aug 20, 2018
@ghost ghost removed the in progress label Aug 20, 2018
@TimBailey-pnk TimBailey-pnk deleted the tplink_device_tracker_error_2 branch August 20, 2018 22:25
@balloob balloob mentioned this pull request Aug 29, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants