kabbi / zigbee2mqtt-adapter

zigbee2mqtt adapter for WebThings gateway
Mozilla Public License 2.0
21 stars 13 forks source link

Use Exposes API for generic device support #25

Closed anvo closed 3 years ago

anvo commented 3 years ago

Since version 1.7.0, Zigbee2MQTT has the new API enabled by default. The new API includes a exposes section that describes which features a device provides.

My idea is to use this new Exposes API to include a generic device support for our zigbee2mqtt-adapter.

When creating a new device the flow is as follows:

The Exposes API is new and not yet feature complete. I guess, it will be extended over time. Right now it works especially nice for lights, that's why I have removed my previously listed lights from the devices.js file. They are better supported by the Exposes API.

Looking forward for some comments!

PS: I also included support for actions

flatsiedatsie commented 3 years ago

Very interesting. Sounds great to me.

To what extent does your code already implement this? How stable would you describe your code to be?

I take it there is a list of zigbee properties somewhere? How are you translating them?

flatsiedatsie commented 3 years ago

I wanted to try your code. I'm seeing this error:

/home/pi/.webthings/addons/zigbee2mqtt-adapter/ExposesDeviceGenerator.js:12
    ACCESS_BIT_STATE = 1;
                     ^

SyntaxError: Unexpected token =

But it could just be my limited understanding of nodejs.

flatsiedatsie commented 3 years ago

I've been able to get it to work (I think) by moving those constants into the generateDevice method.

An IKEA led showed up, and the logs indicate it was generated using this feature. But it seems to be in a read-only state.

zigbee2mqtt_generic

anvo commented 3 years ago

Thanks for your feedback! It looks like I was using some experimental feature which is not supported everywhere. I have updated the PR accordingly. In addition, I have also enable some debug logs in order to investigate any problems. I guess, your read-only problem is due to the missing constants. I have the exact same light bulb, and it is working pretty fine.

As you can see, the code is not yet stable and needs additional testing. I am not an expert in nodejs, so I am really looking forward for someone to do a review of the code to improve it such that it matches the expectation of an actual nodejs developer :smiley:

Concerning the Exposes API, this is a feature discussed in Koenkk/zigbee2mqtt#4466 for exactly this purpose: to specify devices only once and use it everywhere. The current definition is available here: https://www.zigbee2mqtt.io/information/exposes

My code implements the current definition as follows:

Generic Types: Type Support Comment
binary :heavy_check_mark: Mapped to boolean
numeric :heavy_check_mark: Mapped to integer
enum :heavy_check_mark: Mapped to string
text :heavy_check_mark: Mapped to string
composite :x: No idea yet how to map this
Specific Types: Type Support Comment
light :heavy_check_mark: Mapped to Light
switch :heavy_check_mark: Mapped to OnOffSwitch
fan :x: No capability type to map to
cover :x: No capability type to map to
lock :x: No device to test this
climate :x: No capability type to map to

The problem is that WebThings supports a lot more capabilities than what is currently provided by Zigbee2MQTT. But I think, this is something that will evolve over time.

flatsiedatsie commented 3 years ago

Interesting. And really nice work and idea.

I've been implementing your code simultanously, and have just uploaded version 0.3.0. I'll see if I can incorporate the new changes you made.

The goal is to start turning this into an official addon, since the feature you proposed makes it valueable as an addition to the existing zigbee adapter. It could help support more devices. See this issue for more details: https://github.com/WebThingsIO/zigbee-adapter/issues/278

Couldn't climate be mapped to the air conditioning capability?

flatsiedatsie commented 3 years ago

Good news: I can now turn my lightbulb on and off.

It's really cool to see all the additional features that are now exposed, such as the built-in blink ability.

Brightness seems to go up to 255 :-) Maybe that's something we can fix with some heuristics.

If you're ok with it, I'd also like to change the on/off property name into power to make it more voice control friendly.

zigbee2mqtt_works

anvo commented 3 years ago

Well .. it would have been nice to finalize the feature as part of this PR, instead of manually copying the code into your own commit. Plus immediately rushing out a public version.

As you've seen, the feature is not yet finished, needs additional testing, and some rework of a proper nodejs developer.

Going back on topic. The code only translates values provided by Zigbee2MQTT. It does not change any fields or variables. Applying any heuristics or changing values would contradict the idea of the whole Exposed API. If something does not look good, we should change it in Zigbee2MQTT such that everyone benefits, not just this adapter.

flatsiedatsie commented 3 years ago

Apologies for the swiftness. There were a couple of reasons why I thought it ok to get this in.

I also experimented with a recursive analysis of the data instead, in order to extract more properties automatically. That worked, but I accidentally lost that code. It might still be an interesting approach.

In any case, I hope you'll continue working on this, precisely because it's such a good idea.

flatsiedatsie commented 3 years ago

Version 0.4 changes quite a bit