Skip to content
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

Match list of possible candidates off of the library #2663

Open
3 tasks done
RubenKelevra opened this issue Nov 11, 2024 · 14 comments
Open
3 tasks done

Match list of possible candidates off of the library #2663

RubenKelevra opened this issue Nov 11, 2024 · 14 comments
Labels
enhancement New feature or request

Comments

@RubenKelevra
Copy link
Contributor

RubenKelevra commented Nov 11, 2024

Checklist

  • I have filled out the template to the best of my ability.
  • This only contains 1 feature request (if you have multiple feature requests, open one feature request for each feature request).
  • This issue is not a duplicate feature request of previous feature requests.

Is your feature request related to a problem? Please describe.

I noticed that there are several profiles for lights I have in PowerCalc, which do not show up with the auto detection feature.

For example the Livarno Lux HG08131A and the Müller Licht 45311.

The issue here is, that there are more than one Model with this Zigbee identification. For Livarno Lux they seem to use the Zigbee Manufacturer string as model identification, and then still release like 2-3 Lamps with the same string.

Describe the solution you'd like

I was wondering if we could add Zigbee Manufacturer + Zigbee Model to each profile and then build a lookup table based on that (or the details shown in HA if not a Zigbee device). And then show the list of matches to the user, so they can decide if it's on the list or not.

I think that would be a bit easier than the current approach, where I need to add each light which doesn't have an exact match and add it manually, with a lot of options a user usually doesn't want to touch, like these:

Screenshot_20241111_230347

So my suggestion would be to extend the current UI by having a choice option, if such a device is found, to select one of the already existing entries which match the manufacturer/device string.

Describe alternatives you've considered

None

Additional context

None

@RubenKelevra RubenKelevra added the enhancement New feature or request label Nov 11, 2024
@dagobert
Copy link
Collaborator

Powercalc already does a match of manufacturer and model - it is just not in the model.json but the folder structure. This was a descision of the creator of powercalc and will likely not be changed and does not need to adress your problem.

If your device model is not in the library, it might be as an alias. Since I have not had a device yet that was not a model but an alias, I am not sure how the config flow exactly handles it but I do know that @bramstroker descided to not show all the aliases of a model. His argument is that the list in some /many cases would be too long to reasonably manage. You need to lookup the aliases yourself.

Not exactly knowing the config flow in this case, as stated, it might be necessary to emphasize this facte more clearly.

To your second issue I think an extra step or more likely a checkbox could be added to ask the user if it wants to do advanced configuration and othervide skip that part.

@RubenKelevra
Copy link
Contributor Author

His argument is that the list in some /many cases would be too long to reasonably manage.

So instead of showing like 3–5 devices, we show all manufacturers and all devices, because that's easier to manage? ;)

@dagobert
Copy link
Collaborator

dagobert commented Nov 12, 2024

The livarno light is just added a few hours ago.

Can you send screenshots of the device page upper left part in home assistant. These are the information powercalc uses for identification.

@RubenKelevra
Copy link
Contributor Author

Can you send screenshots of the device page upper left part in home assistant. These are the information powercalc uses for identification.

The device info is in the PR.

@bramstroker
Copy link
Owner

I have started working on this. #2681
Essentially it changes the discovery flow when there are multiple profiles which have the same alias in the model information.
Than it will provide the user with a selection (dropdown) where he/she can select the correct model.
Far from finished yet, but made some big progress already.

@RubenKelevra
Copy link
Contributor Author

RubenKelevra commented Nov 16, 2024

Hey @bramstroker that's great, but I think that's the wrong way around:

Say Powercalc has two lamps, like "lamp-a" and "lamp-b" as profiles, z2m would list them as "lamp-a/lamp-b"

So following your approach Powercalc would need to add "lamp-a/lamp-b" as alias.

But if the manufacturer puts out a "lamp-c" z2m would modify the string to "lamp-a/lamp-b/lamp-c" or similar and the alias would stop working.

A better approach is to fetch the model name and vendor name and limit the search to the vendor. Then split the string into an array with "/" as split indicator and then search for each model name in the Powercalc database and show all matches to the user.

This way "lamp-a/lamp-b", "lamp-c/lamp-b/lamp-a" etc. would all match without any alias to "lamp-a" profiles and "lamp-b" profiles.

@bramstroker
Copy link
Owner

bramstroker commented Nov 16, 2024

That can also be supported. Wouldn't be too hard.
But the main thing there need to be architecture to return multiple profiles instead of one given certain search strings.
Needed to overhaul a lot of parts of the system to support that. And also build a new config flow for this scenario (multiple profiles).
That's what I have been focussing on right now and needs to be done first.

Mainly to support cases like this:
#2680
Where HA lists the model as "CCT Light"

Or these kind of lights:
#2052 (TS0505B)

There are numerous more examples where integrations don't expose actual specific model id, but some more generic descriptor.

Previously there was no way to support auto discovery for this lights as the model exposed in HA device info was way too generic.
Now we can add CCT Light to multiple Paulman models as alias and then the user will be provided by a list to choose from instead of not supporting auto discovery at all.

Basically currently Powercalc tries to find models by the following search phrases, and it search through the library on both the model_id and known aliases of a model if it matches any of these search phrases.

search = {
    model_identifier,
    model_identifier.replace("#slash#", "/"),
    model_identifier.lower(),
    model_identifier.lower().replace("#slash#", "/"),
    re.sub(r"^(.*)\(([^()]+)\)$", r"\2", model_identifier),
}

The last one is to support model ID between parenthesis.
We could extend this by exploding on / and also adding all that separate string as possible search phrases, so your use case can also be supported.

@RubenKelevra
Copy link
Contributor Author

Or these kind of lights:
#2052 (TS0505B)

Agreed. In this case we need aliases of "TS0505B", because z2m uses the "white label" option here, instead of listing the models: https://github.com/Koenkk/zigbee-herdsman-converters/blob/963d9a9eaf06e64c0751f3adb4c623d9692729b1/src/devices/tuya.ts#L1684

The reason for this is, that the vendor is different.

For those cases it might make more sense to add add support for identifies as "[vendor, model]" in the model files itself.

So if there's a white label device of a Tuya TS0503B we just add ["Tuya", "TS0503B"] into the model.json and no alias in the manufacturer.json. This avoids that we clutter the manufacturer.json with "Tuya" aliases, which would lead to wrong matches eventually.

@bramstroker
Copy link
Owner

bramstroker commented Nov 16, 2024

For current version I'm working on that won't happen. profiles are first "filtered" by manufacturer / vendor, and than the models are searched.
It's a lot of places where this assumption is done and I don't want to do such massive changes, at least not yet. Might reconsider in the future.
It's already a big refactor under the hood, and I want to keep with small iterations.

@RubenKelevra
Copy link
Contributor Author

Alright, but this means we have to add Tuya to each manufacturer.json as alias to have this matching work, right?

We should at least make sure we add comments to the model.json/manufacturer.json so we keep track about which device identifies as which manufacturer, without going through the full PR history.

@bramstroker
Copy link
Owner

Alright, but this means we have to add Tuya to each manufacturer.json as alias to have this matching work, right?

No this won't work as we cannot have duplicate aliases for manufacturers yet. So the case I mentioned about Tuya TS0505B won't be supported yet. But many other cases will be supported now, including the scenario you requested for the multiple models separated by slash.
Added test and support for that with f56a678#diff-17c82dfe7727a3d12d1cf161db1f8e04a91ae62bbc4b3b8c27d6c2ec96658578R44

We should at least make sure we add comments to the model.json/manufacturer.json so we keep track about which device identifies as which manufacturer, without going through the full PR history.

JSON does not support comments so that's not an option.
I suggest to keep track on a new discussion post

@RubenKelevra
Copy link
Contributor Author

RubenKelevra commented Nov 16, 2024

JSON does not support comments so that's not an option.

You can switch the files from .json to .yaml, as JSON is just a subset of yaml. Yaml supports comments. :)

Shouldn't require much change in the parsing code, normally.

No this won't work as we cannot have duplicate aliases for manufacturers yet. So the case I mentioned about Tuya TS0505B won't be supported yet.

Ah! okay, makes sense. Thanks for the explanation!

@bramstroker
Copy link
Owner

bramstroker commented Nov 16, 2024

Yes I know YAML supports comments. But I don't like YAML.
Besides it's not just one place, but really a lot which needs to change. And also have quite an extensive JSON schema (for validation). Just to give you a glance.

  • Measure tool needs to change
  • API server / middleware for the library
  • Library view tool is build on JSON, and need 3rd party libs to do YAML parsing
  • Integration needs changes on lot of places. Including extensive test suite.
  • Github actions
  • Documentation
  • Users have custom models in their library which are also JSON, so need to have a BC layer.
  • etc.
  • etc.

So this will not happen :-)

@bramstroker
Copy link
Owner

PR merged and initial version release with 1.16.0-beta.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants