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

Duplicate entities on discovery #18074

Merged
merged 12 commits into from Nov 5, 2018

Conversation

Projects
None yet
4 participants
@ehendrix23
Contributor

ehendrix23 commented Oct 31, 2018

Description:

Clean-up coding for detecting duplicate entities (devices) upon discovery.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

ehendrix23 added some commits Oct 16, 2018

Enhancements for DirecTV media player
Following enhancements have been made:

1. Added debug logging
2. Added ability to change channel using select_source service of the remote platform.
3. State will now show paused if a recorded program is paused, for live TV playing will always be returned.
4. Added the following attributes:
    a. media_position: current position of the media (in seconds)
    b. media_position_updated_at: timestamp when media_position was updated.
   c. source: current source (channel).
   d. media_isbeingrecorded: if current media is being recorded or not.
   e. media_rating: TV/Movie rating of the media
   f. media_recorded: if current media is recorded or live TV
   g. media_starttime: Timestamp media was aired

Reordered properties to follow same order as how they are in __init__.py of remote platform.
Fixed error and cleaned up few items
Fixed an issue when determining if a program is recorded or not.
Cleaned up some coding.
Fix issue in checking if DTV device is already configured
If a DTV device was configured before, then discovery would add this device again seperately if the name specified in the configuration is different from the name on the DTV.

This issue is fixed now. Part of the fix also ensure to allow multiple "primary" devices on the network to be discovered.
Further also added debug logging to the setup_platform.
Further improvements
Some additional improvements related to handling the DATA_DIRECTV in hass.data.
Fixed flake8 issue
Fixed flake8 issue
Added available property
Added available property
DEFAULT_PORT, loc["clientAddr"]])
_LOGGER.debug("Known devices: %s", known_devices)
for loc in resp.get("locations") or []:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 31, 2018

Member

This part of the code, requests and data parsing, should really move to the integration library. Preferably try to update https://github.com/sentry07/DirectPy.

This comment has been minimized.

@ehendrix23

ehendrix23 Nov 1, 2018

Contributor

@MartinHjelmare ,
Using getLocations() instead to retrieve the locations. Going through the data received is done to confirm that the device has not already been created as an entity in HASS.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 1, 2018

Member

That's better. But we should not have to parse the json response and know the correct strings to look for to use the library. The library should implement an object model where we access the attributes of the objects. The objects and attributes of the objects should represent the features of the device.

https://developers.home-assistant.io/docs/en/creating_platform_code_review.html#6-communication-with-devices-services

I think it could be ok to merge this for now, but in my opinion the library should take more responsibility before we can take any future changes to this integration.

ehendrix23 added some commits Oct 31, 2018

Updated to use get_locations()
Replaced doing the request for getLocations with the get_locations() API from DirectPy instead.
Fix for checking if device is available
Fix for checking if device is available and small update to debug log message.
Fixed lint issue
Fixed lint issue with unused variable by adding ingore for it as this is for a enumerate
# Make sure that this device is not already configured
# Comparing based on host (IP) and clientAddr.
device_unknown = True
# pylint: disable=unused-variable

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 3, 2018

Member

This is already globally disabled.

# Comparing based on host (IP) and clientAddr.
device_unknown = True
# pylint: disable=unused-variable
for idx, device in enumerate(known_devices):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 3, 2018

Member

idx doesn't seem to be used. If we just need the item in the list, just iterate the list.

for device in known_devices:
# Make sure that this device is not already configured
# Comparing based on host (IP) and clientAddr.
device_unknown = True
for device in enumerate(known_devices):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 3, 2018

Member

enumerate returns a tuple of index and value for each round of iteration.

@@ -104,28 +136,43 @@ def __init__(self, name, host, port, device):
self._last_position = None
self._is_recorded = None
self._assumed_state = None
self._available = False

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 3, 2018

Member

Is there a reason that we need to add availability in this PR, and not in a separate PR? A PR should be as small as possible. One feature or fix per PR is best.

This comment has been minimized.

@ehendrix23

ehendrix23 Nov 3, 2018

Contributor

@MartinHjelmare, no specific reason. I can move it into another PR. My only thought here was cause it is a small change to just add it here. Based on my understanding otherwise I need to wait till this PR is merged before then submitting the next one right?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 3, 2018

Member

You can base one on the other and submit both, but put one as WIP, and then rebase that one when the other is merged.

Or wait, like you say, and submit the second PR when the first is merged.

_LOGGER.debug("Discovered device %s on host %s with "
"client address %s is already configured",
str.title(loc["locationName"]),
host, loc["clientAddr"])
except requests.exceptions.RequestException:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 3, 2018

Member

Only scope the try/except around the code that needs that. Eg if it's only dtv.get_locations that we need to catch the requests exception for we should move this block up under that.

@pvizeli

This comment has been minimized.

Member

pvizeli commented Nov 3, 2018

Implement unique_id and the core is aware that there are no multiple entities around.

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Nov 3, 2018

@pvizeli the platform still has to take care of not adding entities that are non unique. The core doesn't take care of this even with unique_id.

@ehendrix23

This comment has been minimized.

Contributor

ehendrix23 commented Nov 3, 2018

@pvizeli, that is probably then why when I noticed this issue 1 of my DVRs went in as a duplicate (but with different entity id) whereas the other one did not. One of my DVRs has its name as "Family Rm" but in the HASS configuration I state "Family Room". On discovery it then is added to HASS as another entity ID.
The one named "Basement" both in the HASS configuration and the name on the DVR does not come in as duplicate.

Updated try/except and removed available
Updated tr/except having the except by the statement we're doing except on.
Removed available, will be a different PR.
# Comparing based on host (IP) and clientAddr.
device_unknown = True
for device in known_devices:
if host in device and loc["clientAddr"] in device:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 4, 2018

Member

Instead of storing a list of four items [name, host, port, client_address] in known_devices, just store a tuple of (host, client_address). Tuples are hashable so then we can use a single for loop to check membership by creating the tuple to search for from host and client_address.

We should avoid nesting for loops when possible.

new_device = (host, client_address)
if new_device in known_devices:
   ...

ehendrix23 added some commits Nov 5, 2018

Updated known_devices to be tupples in a set
Updated known_devices to be a tupple in a set, removing loop to determine if client was already added.
@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Nov 5, 2018

Maybe update the description since the other PR was merged before this one.

@ehendrix23

This comment has been minimized.

Contributor

ehendrix23 commented Nov 5, 2018

@MartinHjelmare, updated description in this PR and also the other one since the other one now included the functionality of this one without the clean-up. :-)

@MartinHjelmare MartinHjelmare merged commit 561f699 into home-assistant:dev Nov 5, 2018

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on Discovery_Duplicate_Devices_Issue at 93.1%
Details

@wafflebot wafflebot bot removed the in progress label Nov 5, 2018

@ehendrix23 ehendrix23 deleted the ehendrix23:Discovery_Duplicate_Devices_Issue branch Nov 5, 2018

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment