core icon indicating copy to clipboard operation
core copied to clipboard

Add multiple NICs in govee_light_local

Open itewk opened this issue 1 year ago • 2 comments

Proposed change

Currently govee_light_local can only discover and control devices on the primary NIC. This adds support for discovering and controlling devices on any NIC with an IPv4 address.

This is built upon @mill1000 work in https://github.com/home-assistant/core/compare/dev...mill1000:core:issue/govee_discovery

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [x] New feature (which adds functionality to an existing integration)
  • [ ] Deprecation (breaking change to happen in the future)
  • [ ] Breaking change (fix/feature causing existing functionality to break)
  • [ ] Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #124339

Checklist

  • [x] The code change is tested and works locally.
  • [x] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [x] I have followed the perfect PR recommendations
  • [x] The code has been formatted using Ruff (ruff format homeassistant tests)
  • [ ] Tests have been added to verify that the new code works.

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

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

  • [n/a] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [n/a] New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • [n/a] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

itewk avatar Oct 10 '24 20:10 itewk

Hey there @galorhallen, mind taking a look at this pull request as it has been labeled with an integration (govee_light_local) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of govee_light_local can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign govee_light_local Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

home-assistant[bot] avatar Oct 10 '24 20:10 home-assistant[bot]

i have no idea what i would add for new tests here

itewk avatar Oct 10 '24 21:10 itewk

Prob not in scope, but can we extend this PR to support other network ranges? I have multiple networks via VLAN's and this integration doesn't work for me. In my case I have another subnet I would like to access, and not multiple network interfaces.

bryanyork avatar Dec 18 '24 09:12 bryanyork

Anyone to merge this ? The Govee integration is not working for a lot of folks.

moutonnoireu avatar Jan 25 '25 05:01 moutonnoireu

i think it was on me to break up the two parts of this. ill try and do that this week.

itewk avatar Jan 27 '25 14:01 itewk

@itewk Any news on this? Thank you for your work! :)

felixschndr avatar Mar 06 '25 20:03 felixschndr

Any news @itewk ? Still waiting for this :)

moutonnoireu avatar Apr 09 '25 05:04 moutonnoireu

:( i know. im sorry. life.

this code is still working as is in my home. i guess @mill1000 and @Galorhallen what do you need to see to want to merge this in?

itewk avatar Apr 09 '25 14:04 itewk

:( i know. im sorry. life.

this code is still working as is in my home. i guess @mill1000 and @Galorhallen what do you need to see to want to merge this in?

No worries, it happens ! :)

moutonnoireu avatar Apr 09 '25 14:04 moutonnoireu

Ha, I have no authority here. I'm just another user :)

mill1000 avatar Apr 09 '25 15:04 mill1000

Ha, I have no authority here. I'm just another user :)

oh. okay. i assumed anyone who is a reviewer is also a comiter.

itewk avatar Apr 09 '25 15:04 itewk

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes. Thank you for your contribution!

github-actions[bot] avatar Jun 09 '25 16:06 github-actions[bot]

still waiting for this 👀

flecmart avatar Jun 09 '25 16:06 flecmart

Would like to see this added. Being able to use govee devices on a different VLAN, which is becoming increasingly common as people separate out their IoT devices to a VLAN, should be considered a necessity at this point.

cavazos-apps avatar Jun 17 '25 14:06 cavazos-apps

i need someone(s) elses to test this, with and without the "hack" to ignore deactived nics tgo see if its just a me thing that my secondary nic shows up as deactivated even if its not.

Like if everyone has htis deactiated problem, then removing that work around (which is gross) would make this not useful for anyone.

Has anyone else tried this on their instance, with and without using the _async_get_all_source_ipv4_ips call?

itewk avatar Jun 17 '25 14:06 itewk

@flecmart @cavazos-apps @moutonnoireu @felixschndr if you're waiting for this, could you please help test it as requested by @itewk since October last year?

If it's not clear how to test, please mention that and I'm sure that can be worked out.

emontnemery avatar Jun 24 '25 13:06 emontnemery

I can test but actually I only have one network interface. Still my govee light with local control enabled is not detected by the integration which I hoped this change will resolve.

Should I just test the code comitted to this PR?

flecmart avatar Jun 24 '25 15:06 flecmart

I can test but actually I only have one network interface. Still my govee light with local control enabled is not detected by the integration which I hoped this change will resolve.

Should I just test the code comitted to this PR?

This is the code I have been running for months now. I have not yet rebased on top of the latest.

you could try this code as is. you could also try by removing _async_get_all_source_ipv4_ips and using the provided funciton that wasn't working for me, maybe because of my type of install.

itewk avatar Jun 24 '25 16:06 itewk

I can test but actually I only have one network interface. Still my govee light with local control enabled is not detected by the integration which I hoped this change will resolve. Should I just test the code comitted to this PR?

This is the code I have been running for months now. I have not yet rebased on top of the latest.

you could try this code as is. you could also try by removing _async_get_all_source_ipv4_ips and using the provided funciton that wasn't working for me, maybe because of my type of install.

Do you mean async_get_enabled_source_ips mentioned here? I am happy to test both. Will setup your branch in a devcontainer and test this tomorrow.

flecmart avatar Jun 24 '25 16:06 flecmart

Do you mean async_get_enabled_source_ips mentioned (here)[https://github.com/home-assistant/core/pull/128123#discussion_r1796059529]? I am happy to test both. Will setup your branch in a devcontainer and test this tomorrow.

Yes. For me my secondary nic shows up to the default function as disabled and so it won't return its IP. I was never able to figure out why HA thinks that nic is disabled. So I worked around it.

itewk avatar Jun 24 '25 16:06 itewk

So I was able to start up a devcontainer instance with access to my local network and with your code I get

2025-06-24 17:56:49.155 ERROR (MainThread) [homeassistant.components.govee_light_local.config_flow] Start failed on IP 127.0.0.1, errno: 98

as soon as I click on submit

image

The same error number I get in my production homassistant running 2025.6.2. I looked in the code and this happens during start of GoveeController... Do not have time to dig deeper at the moment but my feeling is it is unrelated to this PR

It seems to be similar to this https://github.com/home-assistant/core/issues/140100 - I have only home assistant core running in that devcontainer.

Edit: This happens only after the 2nd time clicking on submit. So I guess the first process is still blocking the 4002 port and does not end? However - I cannot discover my local control enabeld govee light not with your code and not with the original code.

Sorry I feel like this is not really helping.

flecmart avatar Jun 24 '25 18:06 flecmart

@flecmart errno 98 means something else is already listening to port 4002.

To my understanding, the govee integration creates one GoveeController for each config entry and for each discovered light, and each GoveeController will bind to port 4002.

I think GoveeController needs to set the flag reuse_port to True when calling create_datagram_endpoint from GoveeController.start, or the govee_light_local integration needs to create a single controller shared among all config entries and discovery flows.

It should be simple to test this by subclassing GoveeController:

class HackGoveeController(GoveeController):
    async def start(self):
        self._transport, self._protocol = await self._loop.create_datagram_endpoint(
            lambda: self,
            local_addr=(self._listening_address, self._listening_port),
            reuse_port=True,
        )

        if self._discovery_enabled or self._registry.has_queued_devices:
            self.send_discovery_message()
        if self._update_enabled:
            self.send_update_message()

And then use HackGoveeController in homeassistant/components/govee_light_local/config_flow.py and homeassistant/components/govee_light_local/coordinator.py

emontnemery avatar Jun 24 '25 19:06 emontnemery

@emontnemery

Yes, setting the reuse_port solves that issue. Should Probably be another Pull Request. I will fork and take care of this one.

Still my original issue persists, that my local light is not discovered:

2025-06-25 08:28:22.391 DEBUG (MainThread) [homeassistant.components.govee_light_local.config_flow] No devices found with IP 172.17.0.2
2025-06-25 08:28:52.302 DEBUG (MainThread) [homeassistant.components.govee_light_local.config_flow] Source IPs: [IPv4Address('127.0.0.1'), IPv4Address('172.17.0.2')]
2025-06-25 08:28:52.452 DEBUG (MainThread) [homeassistant.components.govee_light_local.config_flow] Starting discovery with IP 127.0.0.1
2025-06-25 08:28:52.485 DEBUG (MainThread) [homeassistant.components.govee_light_local.config_flow] Starting discovery with IP 172.17.0.2
2025-06-25 08:28:57.551 DEBUG (MainThread) [homeassistant.components.govee_light_local.config_flow] No devices found with IP 127.0.0.1
2025-06-25 08:28:57.572 DEBUG (MainThread) [homeassistant.components.govee_light_local.config_flow] No devices found with IP 172.17.0.2

It is only scanning the docker network though... I am using the devcontainer in wsl2 with bridged network mode. I can imagine that the broadcasting can cause some problems in that setup. Unfortunately I cannot check if the scanning works in my prod system because debug logs are never displayed even if I configure the loglevel in my yaml config.

How do you test this stuff on your end?

flecmart avatar Jun 25 '25 09:06 flecmart

I think the easiest way to test changes on prod is to copy the integration to custom_components, a custom integration with the same domain as a built-in integration has priority.

The only change which is needed for this to work is to add a version string to the manifest, e.g.: "version": "0.0.0"

I don't know why it's not working for you in the development container, I don't use govee myself, I just stumbled upon this PR when looking at PRs which seem to be stuck.

emontnemery avatar Jun 25 '25 09:06 emontnemery

I setup a dedicated venv locally to start home assistant dev and no ips in my local network are scanned but my actual govee light is still not discovered. I don't know what is going on in my network here. I have govee H6076, lan control is enabled and the thing is also connected to my wifi.

I will test the code again in my prod environment as custom component as suggested, but I guess someone else must jump in testing.

Edit: Testing this code as a custom_component finally worked for me! Both this PRs code and the other function are working. Weirdest thing is, that now also the officially deployed component can detect my light.

So I can confirm this code works, but I don't have the initial issue that this PR tries to solve. I think someone esle with the right network setup needs to jump in.

flecmart avatar Jun 25 '25 10:06 flecmart

Edit: Testing this code as a custom_component finally worked for me! Both this PRs code and the other function are working. Weirdest thing is, that now also the officially deployed component can detect my light.

So I can confirm this code works, but I don't have the initial issue that this PR tries to solve. I think someone esle with the right network setup needs to jump in.

do you have two NICs on two different VLANs and subnets with Govee devices on one and the other maybe used for internet traffic?

My setup is my govee devices are on an IoT VLAN with not internet access and my Home Assistant has one NIC on the IoT vlan and one NIC on a vlan with internet access. My problem this was solving was that govee integration only scans the first NIC/subnet it finds, which for me was the internet connected vlan where govee does not exist. so the point of this was to get it to scan both nics/vlans/subnets.

if i was being fancy / could figure it out, would have added configuration options to allow people to specifically pick with NICs/subnets to scan. but i was lazy.

I also had the problem that for whatever reason HA does not recognize my second NIC (the IoT) one as enabled (even though it is). hence that extra function to get IPs/subnets from all NICs whether or not HA thinks they are enabled or not. That part is definitely a hack and I need to remove it from this PR. but i had no way of testing this without it. hence trying to find someone who could test this without that bit. maybe figure out how to write some automated tests.

this has fallen into the "its currently working for me locally, and life is priority management" problem, so I haven't been able to pick it up to get it over the finish line, hence asking for help from any of the others who seem to find this PR with some frequency to polish it off.

itewk avatar Jun 25 '25 14:06 itewk

if i was being fancy / could figure it out, would have added configuration options to allow people to specifically pick with NICs/subnets to scan. but i was lazy.

It's good you're lazy, because we don't want this kind of detail in an integration, we want integrations to respect this setting where it makes sense: image

(Note that the text is outdated, this setting does not, and is not meant to, only influence multicast)

emontnemery avatar Jun 25 '25 15:06 emontnemery

if i was being fancy / could figure it out, would have added configuration options to allow people to specifically pick with NICs/subnets to scan. but i was lazy.

It's good you're lazy, because we don't want this kind of detail in an integration, we want integrations to respect this setting where it makes sense: image

(Note that the text is outdated, this setting does not, and is not meant to, only influence multicast)

If only i could find that elusive setting in my install.....i have heard it mentioned of, but i have never been able to find it. probably cuz im using the now deprecated suprvised install. not sure what install i can move to now on my Orange PI 5 as there is no HAOS build for Orange PI :(

itewk avatar Jun 25 '25 18:06 itewk

If only i could find that elusive setting in my install

It's under /config/network. Not all integrations respect it though.

emontnemery avatar Jun 25 '25 19:06 emontnemery

do you have two NICs on two different VLANs and subnets with Govee devices on one and the other maybe used for internet traffic?

No, I don't have this separation at this moment. Initially I thought my problem was related to this issue but I had a misconception.

What I will do is create a PR to govee_local_api and set the reuse port flag. This is also unrelated to this PR so I will communicate in the relevant channels. Sorry that I cannot be more helpful in finishing your work here.

flecmart avatar Jun 26 '25 08:06 flecmart