puttyman / hass-amplifi

A home assistant integration for Ubiquiti Amplifi
27 stars 16 forks source link

Return better friendly names for connected devices #50

Closed atudor2 closed 1 month ago

atudor2 commented 2 months ago

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:

image

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

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

image

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

atudor2 commented 2 months ago

@hawksj (assuming it's you who will review this eventually 😉) I'm going to leave this in draft for now until 2.1.0 is released - I'll rebase once master is updated.

I'd appreciate in the meantime your opinion on whether you agree with this change or not? I previously just monkey patched my component source after each upgrade (since I was too lazy to update descriptions by hand), but I think having the friendly name match what is is configured in the app is useful?

hawksj commented 2 months ago

Thanks for the PR Alistair this looks great. I had wanted to do something similar but hadn't figured out how to.

While I worry about straying too far from puttyman's vision for this, I do think this would be a valuable change. I currently display devices in a markdown card that strips the amplifi prefix from the entity ID.

I'm curious what happens in cases where the device friendly name is unknown and the entity ID is either just an IP address or a MAC address? This seems to happen with iPhones that use MAC Randomization among others. I will try cloning your branch into my HA and experiment a little. Thanks for submitting this :)

hawksj commented 2 months ago

Sorry, I didn't get time to properly review the code before. The changes in device_tracker.py on line 85+ cover that possibility. I'll try to test this weekend

hawksj commented 1 month ago

@atudor2 #47 has been merged and v2.1.0 has been released (officially! 🥳)

Feel free to rebase when you're ready and we can test installing this patch on top of the current release. Thanks again for this :)

hawksj commented 1 month ago

Thanks Alistair. Upgrading from the current version seems to work fine, looks good on my end.

I've noticed that if there is a virtual switch connected to an Ethernet port (in my case, the Proxmox node that runs HA), the port is identified as the host but the guests don't get device trackers. I'm guessing this stems from the fact we're getting information from the port not the device which connects via the port. Seeing as the AmpliFi app displays this info, I'm assuming it should be possible to iterate through them as we do with WiFi devices and get all wired hosts too (even through third party switches).

With this in mind, do you want to proceed with naming the port with the name of the device, or leave it as "Ethernet Port 1" (as the app displays) and later adding functionality to get wired devices? I can't get the raw info file right now but I'll check it later to see if it's possible to do Ethernet devices the same way as wireless ones. IMG_0031

hawksj commented 1 month ago

Sorry, me again. I've done some testing this afternoon. I think this function can be dropped into coordinator.py and creates a list of ethernet devices that are connected. It is based on your code but slightly modified to get device info rather than port info.

    def extract_ethernet_devices(self):
        if self.data is None:
            return
        router_mac_addr = self.get_router_mac_addr()

        ethernet_devices = {}
        raw_devices_info = self.data[DEVICES_INFO_IDX]
        raw_device_to_eth_index = self.data[ETHERNET_PORT_TO_DEVICE_IDX][router_mac_addr]

        if raw_device_to_eth_index and raw_devices_info:
                for device in raw_device_to_eth_index:
                    device_info = raw_devices_info[device]
                    port = raw_device_to_eth_index[device]
                    device_info["connected_to_port"] = port
                    ethernet_devices[device] = device_info

        self._ethernet_devices = ethernet_devices

        _LOGGER.debug(f"ethernet_devices={self._ethernet_devices}")

The trouble is the AmplifiEthernetDeviceTracker class in device_tracker.py is not capable of handling data like this, it would need to be modified a lot to be able to create both port entities AND actual Ethernet device entities. I think it would be better to create a whole new class just for handling that and I don't fancy doing that at the moment, perhaps when I have more time. I've pushed the changes I've made (although it's broken and non-functional right now) to a branch in my fork, it's largely the same stuff you've done but with some bodged changes to the device tracker.

With that in mind, would you like to merge these changes as-is (and potentially revert the Ethernet Port naming bit later if we get proper Ethernet Device Tracking working) or modify the port naming function completely and just name them Ethernet Port 1-4? This is your PR so up to you either way, whatever you think the average user would prefer.

hawksj commented 1 month ago

@atudor2 I've built a lot on top of this. There is a new function for ethernet devices so devices and ports can be tracked separately. It was easier than expected actually. I think the best way to handle this is to merge this PR, then make another PR for my changes? Or is it best to merge into your branch and push the changes through together?

atudor2 commented 1 month ago

Hi @hawksj sorry for the delay in reply, been swamped recently and haven't had a chance to review things.

Reading back on your work (good stuff btw 👍), at this point I think it might be better if we close this PR and you open one with your additional changes?

I can then test it on Saturday/Sunday in my test instance and give feedback?

hawksj commented 1 month ago

Hi @atudor2 not a problem, sorry for spamming this PR!

Okay, if you think that's the best bet I'm happy to go with that. I just didn't want your contributions to be sidelined. I'll make a new PR from my branch later today.

If you could test at the weekend that would be really helpful! Which AmpliFi product do you use? I use the HD but it would be handy we had an Alien we could reliably test pre-releases on too to prevent issues like #10. Thanks, Sam

atudor2 commented 1 month ago

I also have an HD unfortunately - maybe I need to use this as an excuse to upgrade in the next few months 😄

I'll make a new PR from my branch later today.

👍 I'll do some testing on the weekend and leave feedback there. I will now close this PR so long