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

feat: Generate definition for unknown devices #6692

Merged
merged 12 commits into from
Dec 22, 2023
Merged

feat: Generate definition for unknown devices #6692

merged 12 commits into from
Dec 22, 2023

Conversation

ffenix113
Copy link
Contributor

Continuation of Koenkk/zigbee2mqtt#20173.

Support generating device definition for unknown devices. Only will be used when specifically allowed.

@Koenkk
Copy link
Owner

Koenkk commented Dec 13, 2023

Many thanks for this PR.

The biggest question for me now is more conceptual, for most automatically generated definitions (but not the ones for your use case since those are DIY) we want users to contribute them back such they e.g. show up on the supported devices page. There should be an easy way to contribute confirmed working generated definitions back. So I guess this is something that should be done from the frontend.

Maybe we can show a generated external converter in the frontend? Based on this users can then easily create a PR or issue.

CC: @kirovilya @nurikk @sjorge

@sjorge
Copy link
Contributor

sjorge commented Dec 13, 2023

I think this should be a manual action tied in with the frontend.

A simple way can just be to opt-in the device for auto generation, or a more complex, something like the rough flow below:

  • device join -> interview cycle -> not supported || supported
  • frontend can then show a button for not supported devices that when pressed generates all possible converters based on cluster (like in this PR)
    • allow for accept/reject of the convertor (since this is now tied into exports too it can be a checkbox in the frontend or something for the export entry (we should be able to detect read/write support based on the cluster information if we probe the device as it should theoretically tell us if it's RO or RW and if it's reportable even if we poke it correctly.
    • offer a create external converter that will include all the checked and include some extra info like db entry and stuff in the header.

We can then simply ask the user to submit the generated external converter, we could name it something like autogen-{modeid}.js although, this probably won't work for TuYa and some other manufacturer as they don't really follow spec well. It should cover most well behaving devices.

@ffenix113
Copy link
Contributor Author

for most automatically generated definitions (but not the ones for your use case since those are DIY) we want users to contribute them back such they e.g. show up on the supported devices page

I understand this idea and it is fair. But also my "vision"/understanding for this feature was to be kinda similar to ZHA from user perspective: even if device is not known - do your best to support it. So list of supported devices by this definition may not be necessary.
This also ties with how I thought this feature might be used in the end: no static device definitions, only device/manufacturer-specific "quirks". But maybe this is not realistic, as I am just jumping in to this topic.

General device information(i.e. description and picture) is more useful indeed.

A simple way can just be to opt-in the device for auto generation

I think for most this would be good approach in the beginning.

In the UI, on the device page with generated definition it can mention that device support is generated based on device information(as it does in the device description in this PR), so that if device is not supported 100% - users may do some action to improve the situation. For example, as you mentioned, to click a button in UI to create a new issue on Github with some text based on device make/model/endpoints & user description.
Adding functionality may be hard for users(imagine device with multiple endpoints with same clusters, optional attributes for clusters, how users should know what to do in this cases?), but it may be easier to disable functionality.

@kirovilya
Copy link
Contributor

I support the idea of supporting such general-devices (I even started working on it). These can be either DIY devices or numerous light bulbs, switches, and sensors. Those, this should work for devices that comply with the classic ZCL and HA profile. This works in ZHA and in many hubs.

Now about the process of identifying general-devices.
The first step is to determine the set of functionality (by cluster).
A situation may arise here when functional are duplicated on different endpoints. This could be a DIY device with multiple sensors or multiple relays. In this case, you need to specify a specific endpoint in modernExtension.
Also, based on the set of functionality, you can determine the type of device (sensors, switches, light bulbs...) and indicate this type in the name of the device and set the picture of the device (make a set of simple pictures for general-devices).

Yes, we talked earlier about the need to generate definition for such devices so that the user can make a PR to the converters repo.
But it seems to me that this should not be a mandatory action for the user - everything should work for him right away.
Generation can be done from the interface. If the device is defined as general, then a button should appear to generate an external converter and download it in the browser (and then the user decides whether he will use it or not) or even immediately open an issue to add a new device with the attached converter.

@Koenkk
Copy link
Owner

Koenkk commented Dec 15, 2023

The thing I have against generating the definitions without having any incentive to contribute it back, is that we are basically killing the supported device page. The supported device page is unique, (projects like Deconz and ZHA don't have this) and is highly valued by users as you know upfront whether a device is supported and what features it will support. We should therefore make it extremely easy to contribute it back (with a click on the button from the frontend)

@ffenix113
Copy link
Contributor Author

The supported device page is unique, (projects like Deconz and ZHA don't have this) and is highly valued by users as you know upfront whether a device is supported and what features it will support.

I see. In no way I wanted to say that it is not useful, or diminish it's value. I am sorry for misunderstanding. But now I also better understand the benefit of it.

In this case what I would think of is contributing back device information: user-facing(manufacturer, name, icon, description maybe) and for documentation generation(endpoints, clusters & available attributes). Having this information together with device doc generator would allow to generate documentation for supported features of the device.
While it now sounds to me like this is trading one set of information for another - I suppose the benefit of getting more technical information might be that users should not define it. Meaning that there could be a button in UI for generated device, that when clicks pops up a modal window asking for device description & icon. All other information can be inferred from polled device information. But then one would need to think about solution for device fingerprint and different devices with same model ID.

Again: I appreciate your(and this community's as well) work on this project, and don't want to "force" any opinions 🙂

@Koenkk
Copy link
Owner

Koenkk commented Dec 15, 2023

@ffenix113 I think basically what we can do is in the frontend link to an issue template where the user can fill in the generated definition, a description, a proper model names (because the zigbee ones are usually bad) and a link to the picture

I will probably have some time to work further on this during the Christmas holidays.

@ffenix113
Copy link
Contributor Author

On the other note: I wanted also to add to this PR ability to make extenders configurable to support optional features, but for that cluster attributes are required. Unfortunately they seem not to be present after device interview and I don't know if/where they could be fetched by using Discover Attributes Command(section 2.5.13).
I can try to add this, if proper direction for this will be shown.

@Koenkk
Copy link
Owner

Koenkk commented Dec 15, 2023

They should be fetched during the device interview somewhere around here. It would be great if you could already start on that!

@Koenkk
Copy link
Owner

Koenkk commented Dec 16, 2023

@ffenix113 I think we should already merge this PR without the changes in index.ts (a new draft PR can be created for that). The additions to modernExtend are required for other PRs for example.

Update: if I understand correctly, the functionality is disabled by default? (generateForUnknown: boolean = false) If yes, then leave index.ts as is.

src/index.ts Outdated Show resolved Hide resolved
@sjorge
Copy link
Contributor

sjorge commented Dec 20, 2023

Might need a rebase now that temperature and humidity modernExtends that handle reporting got merged

@Koenkk Koenkk merged commit 3468c09 into Koenkk:master Dec 22, 2023
2 checks passed
@Koenkk
Copy link
Owner

Koenkk commented Dec 22, 2023

Many thanks, this is a huge step for z2m 😄

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

Successfully merging this pull request may close these issues.

4 participants