| CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 36.3k
Add multiple NICs in govee_light_local #128123
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
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
|
itewk
force-pushed
the
feature/govee-multi-nic
branch
2 times, most recently
from
3dd8d67 to
dda17e3
Compare
itewk
force-pushed
the
feature/govee-multi-nic
branch
2 times, most recently
from
fe63b99 to
ad216d8
Compare
| from homeassistant.core import HomeAssistant | ||
|
|
||
|
|
||
| async def _async_get_all_source_ipv4_ips( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. Is your secondary NIC configured via System > Network?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it shows up in system network in the UI if that is what you are asking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im running on supervised installer. i was looking for a UI like that....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
itewk
force-pushed
the
feature/govee-multi-nic
branch
8 times, most recently
from
5731ea1 to
0e00b4b
Compare
|
i have no idea what i would add for new tests here |
MartinHjelmare
changed the title
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
|
@itewk based on the test done by @jhollowe, do you think it's OK to merge the PR now, or do you need also a test with the modification suggested here: #128123 (comment) Also, did you find the settings under To find it via the UI, first click settings: Then system: And finally network: Where you have the setting (note that the text is now improved compared to my previous screenshot): |
Yes. but i need to remove the use of the custom function that bypasses disabled NICS because of my own issue. Trying to do that now.
My network settings look different, see: #128123 (comment) |
itewk
force-pushed
the
feature/govee-multi-nic
branch
4 times, most recently
from
cd8c611 to
c5fa7f0
Compare
|
@jhollowe i removed my personal workaround for my NIC problem. would you mind testing again? |
itewk
force-pushed
the
feature/govee-multi-nic
branch
3 times, most recently
from
d211609 to
2867bb8
Compare
|
rebased. |
That screenshot is almost a year old though, does it still look the same? |
it looks the same. but now i have a warning that my install type is being depreciated. so i am going to have to change install types. sigh |
|
@itewk your screenshot is of the first panel in the network settings, which allows configuring IP address etc. You may need to enable "advanced mode" in user settings. |
ooop. i see it. but it only lists one of my interfaces, it doesn't list both. so I don't know whats up with that. when i use my little cheat work around in code it definitely finds both, just one is listed as disabled. |
nevermind! OMG! i had to uncheck |
itewk
force-pushed
the
feature/govee-multi-nic
branch
from
547dcff to
fc02277
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.
itewk
force-pushed
the
feature/govee-multi-nic
branch
from
fc02277 to
097c3fc
Compare
|
fixed issue where it was trying to listen on IPv6 addresses. I have tested trhis locally and it is all working for me with no weird hacks. thanks @emontnemery |
emontnemery
left a comment
emontnemery
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @itewk 👍
|
Wahooooo! Thanks! |
Labels
12 participants
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: