Add multiple NICs in govee_light_local #128123
Conversation
|
Hey there @Galorhallen, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
3dd8d67 to
dda17e3
Compare
fe63b99 to
ad216d8
Compare
| from homeassistant.core import HomeAssistant | ||
|
|
||
|
|
||
| async def _async_get_all_source_ipv4_ips( |
There was a problem hiding this comment.
I don't think we should re-invent the wheel here. The Network integration has async_get_enabled_source_ips which can easily be filtered down to IPv4 addresses.
There was a problem hiding this comment.
@mill1000 the problem with async_get_enabled_source_ips is that it doesnt actually get all the IPs. There is some logic that decideds which NICs are enabled or not based on like mDNS hops or something. at least for me it was calculating my secondary nic as disabled. so i had to write a function that didn't filter out "disabled" devices.
There was a problem hiding this comment.
Oh. Is your secondary NIC configured via System > Network?
There was a problem hiding this comment.
it shows up in system network in the UI if that is what you are asking.
There was a problem hiding this comment.
Hm.. that's quite a bit different looking than mine. Must be a HAOS vs container thing.
Here's mine where I manually enable a 2nd adapter. But I'm broadly assuming this is what "enables" another interface.
There was a problem hiding this comment.
im running on supervised installer. i was looking for a UI like that....
There was a problem hiding this comment.
just to make sure i wasn't crazy i tested using the async_get_enabled_source_ips and it doesn't return the IP for my secondary NIC.
so idk if this is just a me problem, and i dont know how else i could work around it, or if some other problem.
5bd0cab to
5731ea1
Compare
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.
5731ea1 to
0e00b4b
Compare
|
i have no idea what i would add for new tests here |
| if adapter["ipv4"]: | ||
| addrs_ipv4 = [IPv4Address(ipv4["address"]) for ipv4 in adapter["ipv4"]] | ||
| sources.extend(addrs_ipv4) | ||
| if adapter["ipv6"]: |
There was a problem hiding this comment.
I think you can remove the if and the continue since it's the last instruction in the loop
There was a problem hiding this comment.
I am happy to make that change but based on the previous threads it would seem not ideal for this PR to include the custom function for getting all IPv4 addresses that works around what seems to be an issue only for me that my additional NIC showes up as deactivated when using the stalk get enabled IPv4 addresses function.
so i was thinking i should re-do this PR without the custom function and ill leave the custom work around on my own version.
i was thinking maybe doing another PR that exposes a configuration option for listing the IPs want to listen on.
thoughts? should I leave this fucntion in here, or update to use the stalk get enabled IPs function?
There was a problem hiding this comment.
I wouldn't rip out this code just on my behalf. It'd be great if a dev more familiar with the networking code could comment.
|
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. |
|
Anyone to merge this ? The Govee integration is not working for a lot of folks. |
|
i think it was on me to break up the two parts of this. ill try and do that this week. |
|
@itewk Any news on this? Thank you for your work! :) |
|
Any news @itewk ? Still waiting for this :) |
|
:( 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 ! :) |
|
Ha, I have no authority here. I'm just another user :) |
oh. okay. i assumed anyone who is a reviewer is also a comiter. |
|
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. |
|
still waiting for this 👀 |
|
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. |
|
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 |
|
@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. |
|
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 |
Do you mean |
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. |
|
So I was able to start up a devcontainer instance with access to my local network and with your code I get as soon as I click on submit
The same error number I get in my production homassistant running It seems to be similar to this #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 errno 98 means something else is already listening to port 4002. To my understanding, the govee integration creates one I think It should be simple to test this by subclassing 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 |
|
Yes, setting the Still my original issue persists, that my local light is not discovered: 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? |
|
I think the easiest way to test changes on prod is to copy the integration to The only change which is needed for this to work is to add a version string to the manifest, e.g.: 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. |
|
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 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. |
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. |
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: (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 :( |
It's under |
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. |
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 dev...mill1000:core:issue/govee_discovery
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: