Return better friendly names and generate entities for Ethernet devices

[hawksj]

Based on PR #50 by @atudor2.

It has always bothered me a little that the default friendly names for the entities has been the unique ID instead of the configured name within the Amplifi app:

This change extracts the user-defined name and returns that as the 'friendly name' within HA while keep the entity_id as-is:

In addition, there is an extension to tease out more information for Ethernet connected devices and return that as the friendly name as well

For completions sake, the WAN upload/download speed sensor has been adjusted too:

Rather than naming Ethernet ports after the devices connected to them, it leaves Ethernet ports named 0-4 and creates separate entities to track Ethernet devices. This experience reflects how the AmpliFi app presents Ethernet devices.

The entity also reports which upstream Ethernet port it is connected to, as well as IP information like WiFi devices. Unfortunately the router does not provide the same level of detail for Ethernet devices it does for WiFi devices, but you can at least see them now.

[hawksj]

This PR is for a new dev branch. I will merge these changes (despite being untested) so that I can make a pre-release version. This will make it easier to swap between versions in HACS while testing @atudor2

[hawksj]

Hm, this workflow doesn't really work if the PR is closed after merging, does it?

@atudor2 somewhere it seems a bug has been introduced where the Ethernet port entities and Ethernet device entities are not enabled by default. This must be related to the changes I've made but I can't see where because, as far as I can see, the AmplifiEthernetDeviceTracker class has never had the entity_registry_enabled_default property and yet they were previously enabled by default.

It's not a problem to add this property, I just don't understand why I need it now when we never have before...

[hawksj]

This issue is 100% introduced in 2.1.1. 2.1.0 works totally fine, even in a fresh instance of the integration, the entities enable normally, but in 2.1.1 beta, both the ports and the ethernet devices are disable irrespective of what you select in the config entry about enabling entities by default. The thing is, the default behaviour SHOULD be to enable new entities.

This can be fixed by adding the property but why is it doing this? I've made a mistake somewhere

[hawksj]

@atudor2 don't suppose you had an opportunity to test this at the weekend?

Can you suggest a better way to manage beta tags/branches? I don't think the way I've done it this time round works very well.

[atudor2]

Hi @hawksj sorry I didn't get a chance last weekend due to a last second change of plans 😞 but I will try to take a look further this Saturday

[hawksj]

No worries @atudor2 no rush. Hope everything is well.

[atudor2]

Hi @hawksj sorry again for the delay in feedback...

I did a run within my test instance and it looks good to me 👍 In fact just installed the beta version into 'production' and 2 hours in no issues either, but will keep an eye out

This issue is 100% introduced in 2.1.1. 2.1.0 works totally fine, even in a fresh instance of the integration, the entities enable normally, but in 2.1.1 beta, both the ports and the ethernet devices are disable irrespective of what you select in the config entry about enabling entities by default. The thing is, the default behaviour SHOULD be to enable new entities.

This can be fixed by adding the property but why is it doing this? I've made a mistake somewhere

On this, I attached the debugger and watched what happens when HA adds the entity and I think I traced the issue to:

homeassistant/helpers/entity_platform.py -> _async_add_entity() (https://github.com/home-assistant/core/blob/aac31059b0166729787aafe87fc4e7860dafb730/homeassistant/helpers/entity_platform.py#L807)

If disabled_by is not null then it will be added as disabled.

Checking the default implementation in homeassistant/components/device_tracker/config_entry.py -> entity_registry_enabled_default() (https://github.com/home-assistant/core/blob/aac31059b0166729787aafe87fc4e7860dafb730/homeassistant/components/device_tracker/config_entry.py#L308)

The change is due to exposing mac_address as a property in commit https://github.com/puttyman/hass-amplifi/commit/9ea4ba8a567f7de3b26da324c171a705d69d74fe - because mac_address is no longer None, device_info is final and always None and we don't have a device entry, it adds as disabled.

In fact I think this is the reason why the old Wifi device tracker also used to get added as disabled by default 😃

I don't fully understand HAs reasoning for this logic but I think the simplest thing to do would be to override entity_registry_enabled_default() like AmplifiWifiDeviceTracker

Can you suggest a better way to manage beta tags/branches? I don't think the way I've done it this time round works very well.

To be honest I don't see a problem with how you've done it? Is there something specific that is causing an issue? I guess if I was feeling pedantic, I'd rename the 'dev' branch to 'beta' 😉

[hawksj]

This is really useful investigation, thanks so much for doing that @atudor2. That explains why it was doing that, just weird logic. It seems to be related to making device entries? I guess the MAC address property is the attribute that links entities into devices? It would be nice if this integration was able to link device tracker entities to entities from other integrations.

I will set entity_registry_enabled_default() as we have with WiFi devices. I'm leaning towards enabling them by default as they cause considerably fewer state updates compared to WiFi devices and there are likely to be fewer Ethernet devices connected than WiFi ones. The other options are: 1) proceed with enabling (or disabling) by default 2) following the setting chosen in the config entry that currently applies to WiFi devices 3) making a new config entry setting separately for Ethernet devices (ports should always be enabled)

I'm not sure what would be in users best interest here.

To be honest I don't see a problem with how you've done it? Is there something specific that is causing an issue? I guess if I was feeling pedantic, I'd rename the 'dev' branch to 'beta' 😉

Only that once the PR is merged into the dev branch, which wasn't checked over by you or anyone else before I did it, the PR is closed and it makes it more difficult to discuss the changes. Maybe the proper procedure is to make one PR for dev and merge quickly, and a second PR at the same time for master to discuss the changes on the dev branch? I'm not sure the best way to allow open discussion of beta releases before being merged into master, basically.

[atudor2]

Hmm I'm leaning towards option 2 to be honest - seems the most consistent to me. Option 3 could work but if I'm honest I think its overkill.

Ah I see what you mean re PR closure. Hmm I wonder, maybe it would be better for larger features that a PR branch gets tagged as a released? Then the PR can be left open and HACs should show it as a pre-release version?

But I think this is only really needed for the bigger changes - something small I don't mind just pulling the code manually and testing in my dev instance as needed

[hawksj]

Hi @atudor2 sorry I've been busy for a few weeks and this completely slipped my mind.

I have pushed a commit today that appears to fix this by way of option 2. If the entity is_device and the boolean is enabled, the entity will be enabled. If the entity is not is_device it will always be enabled (ports) and they will be disabled like WiFi devices if the boolean is disabled. Below is the current state of all types of entities, WiFi, Ethernet and Ports.

I've checked out what you were saying about tags but it seems GitHub doesn't allow you to tag a PR per se. You might be able to tag a specific commit, I will have to check and see how to work around it. The important thing for HACS is that there is a release which you can do per-commit.

I will try to get a 2.1.1-beta2 out ASAP and create a new corresponding PR (assuming I can do it in a way that makes sense) so we can discuss the changes there. Thank you.