Huawei LTE SSDP improvements #81643
Huawei LTE SSDP improvements #81643
Conversation
|
Hey there @fphammerle, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Looks straightforward to me, I'm just wondering how often a device with IGD:1 from one of those two manufacturers is not a wanted device? I.e., how useful is it to perform extra checks considering this introduces a new whitelist to maintain.
edit: I'm wondering if the routers do expose some more specific service that could be used for detection?
|
Good questions indeed. Re the wantedness/allowlist: To be honest, I have no clue, and I have no access to devices we would not want. There used to be the huawei_router integration that dealt with some other than mobile/LTE devices, so I suppose we might encounter some. Anyway, I would be fine with just starting from a clean slate and dropping the current list, and seeing if we get any reports. (Aside, it's not a new whitelist -- one existed already, but was simpler.) I'm kind of indifferent here, if any of you is leaning towards keeping/removing, let's do that, just let me know. Re the services: the semi-recent device I can access at the moment exposes LANDevice:1, WANPPPConnection:1, WANConnectionDevice:1, WANDevice:1, and InternetGatewayDevice:1. None of those sounds any better or specific than the IGD we currently use, but if anyone knows better or has different opinions, do speak up. |
|
Another older dongle has services WANIPConnection:1, WANConnectionDevice:1, WANCommonInterfaceConfig:1, WANDevice:1, Layer3Forwarding:1, InternetGatewayDevice:1. Its friendly name is not something that we would have found as "ours" before or after the changes in 4bd2e28 (it's just "Huawei Internet Gateway Device"). Dropping the additional match would make that one discoverable via SSDP. Guess we could replace the friendlyname match with a probe at an endpoint that according to our best guess is known to be available without authentication in ~all devices if we're worried about false positives. If you have a better feeling about this approach than just removing the match, let me know and I'll have a look at that. |
Some older supported dongles have that.
There was a problem hiding this comment.
I think it is good as it is. After all, any potential false positives are a non-issue as the user can likely judge better and in the worst case just ignore the discovered device to stop showing it.
edit: re-reading your second comment, maybe "Huawei" should be added to the regexp to let that another dongle be discovered, too?
To me the regexp exists to filter out other Huawei(/SoyeaLink) devices besides the LTE'y ones, and adding "Huawei" there would more or less defeat the purpose. In that sense I would not add it, but then again, as said, I would be open to removing the regexp match altogether and see where that takes us. As you noted, the worst case scenario wouldn't be that much of a biggie, the user has the option to ignore false positives. |
|
Found a thinko with regards to unique id handling, addressed in c431a1f. |
|
Ahh, gotcha. Your call, both options are fine in my opinion. |
This is fragile at best, and actually prevents some devices from being considered. Leave it to the user to decide instead.
|
I'm leaning towards removing it, done so in f12e378. Going to merge this ~tomorrow unless there are objections. |
| CONF_URL: url, | ||
| } | ||
| ) | ||
| return await self._async_show_user_form() |
There was a problem hiding this comment.
Side note: We should not move directly to another step's form. The form is private to the step method. Await the step method to move to that step.
Proposed change
Various fixes and improvements to Huawei LTE SSDP discovery, see individual commits for details.
Type of change
Additional information
Checklist
black --fast 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..coveragerc.To help with the load of incoming pull requests: