search

Koenkk / zigbee-herdsman

A Node.js Zigbee library
MIT License
456 stars 277 forks source link

[Question] Strategy for manufacturer-specific attributes in standard clusters? #4

Closed felixstorm closed 4 years ago

felixstorm commented 4 years ago

As already mentioned in my last pull request, I am implementing support for some ubisys devices at the moment. To e.g. calibrate their shutter controller to the run-time from fully open to fully closed, they use quite a lot of manufacturer-specific attributes within standard clusters (e.g. attributes 0x1000 and up in closuresWindowCovering). Currently, I have these implemented as numeric values in code but thought it might be nicer to have them somewhere in a central location, but could not really figure out an obvious way to do it. One idea is to implement them in additional clusters that have numerical ids with the manufacturer id as upper bytes, e.g. for ubisys (manufacturer id is 0x10f2) and the cluster closuresWindowCovering (258 = 0x102) it would be 0x10f20102 = 284295426. But even this would mean that I will need to touch quite some central code inside zigbee-shepherd and zigbee2mqtt to implement it correctly...
Do you have any opinion or suggestions for me or should I rather stick with the way I have it implemented currently?

Current implementation in my code: https://github.com/felixstorm/zigbee2mqtt/blob/f41f754135f918a9b60ad14501311e48f5ae1114/lib/extension/ubisys.js#L166 ubisys documentation (if interested): https://www.ubisys.de/wp-content/uploads/ubisys-j1-technical-reference.pdf

Thanks, Felix

Koenkk commented 4 years ago

Also the answer to #5, I would just add them zcl-id/zcl-packet and prefix it with ubysis.

The aim of zigbee-herdsman is more on "real world usability" instead of the "perfectly follows ZCL specification" part.

felixstorm commented 4 years ago

Ok, I'll check that - but are you sure that adding a new cluster with the same numeric id as an existing one (see #5 ) is a good idea? I would be a bit worried when it comes to reverse lookup on incoming Zigbee packages...

Koenkk commented 4 years ago

In this case, the cluster should be renamed, e.g. manuSpecificPhilipsUbisys, as id's cannot be duplicated.

felixstorm commented 4 years ago

Ok, but then we will be in trouble again as soon as we hit duplicate attribute ids (ubisys uses e.g. 0 and 1).

I would rather go for clearer definitions, and I only need the additional clusters and attributes for configuring the devices inside the ubisys extension where I anyway have my own read and write attribute functions. Would it therefore be ok for you if I define them as suggested above with the manufacturer id prefixed and only parse them inside my own code without touching anything general?

Koenkk commented 4 years ago

My biggest struggle with https://github.com/felixstorm/zigbee2mqtt/blob/f41f754135f918a9b60ad14501311e48f5ae1114/lib/extension/ubisys.js#L166 is that it is a lot of code (= extra maintenance) while adding very less feature wise (only for ubysis users, which you are the only reported one now).

Koenkk commented 4 years ago

As I'm currently completely refactoring zigbee-herdsman anyway, I'm thinking it maybe nice to have 'conditional' manuf specific clusters, e.g. only when the manufcode matches. This would solve your problem and keep all parsing inside zigbee-herdsman.

felixstorm commented 4 years ago

Great, sounds very good to me! If you find a way to also take care of manufacturer-specific properties inside standard clusters that would sure be helpful as well (so we won't get in trouble in case of duplicate attribute ids either).

I'll then go ahead with my personal implementation for now and will update it to the new spec as soon as it is available.

Thanks, Felix

felixstorm commented 4 years ago

Ok, I have done some more work and implemented everything inside the ubisys extension: https://github.com/felixstorm/zigbee2mqtt/blob/master/lib/extension/ubisys.js

My JavaScript knowledge really is a bit rusty and I need to get used to promises, but I like it really a lot better than Python (used to play around with https://github.com/zigpy/zigpy also) :wink:

If you have any comments or suggestions, please feel free to do so. I need to do some more testing (especially binding an input module C4 to dimmers or shutter controllers) and might also implement the metering part and then I will do a PR.

By the way, I can really recommend the ubisys devices. They are not cheap, but small, work very reliably, are very well documented (e.g. Shutter Control J1 Technical Reference) and provide a lot of different functions directly on the Zigbee level. I have around 15 of them in my house for 2 years now and I am perfectly satisfied.

Koenkk commented 4 years ago

@felixstorm great! Maybe I misunderstood but as mentioned in https://github.com/Koenkk/zigbee-herdsman/issues/4#issuecomment-509570632 I cannot merge this extensions.

felixstorm commented 4 years ago

Sorry - this really seems to be a misunderstanding. I thought it would be helpful to keep everything self-contained inside ubisys.js? From my side, there is currently no need anymore to have the ubisys manufacturer specific clusters or attributes inside zigbee-herdsman since they are only needed to configure the devices and this is now all inside ubisys.js.
Or do you think that ubisys.js in general is too much code to take into zigbee2mqtt? But I hardly see another way to configure the devices. Or is there any other interface I could use to communicate with zigbee2mqtt on a low level so I could put the ubisys configuration code into a separate project?

Koenkk commented 4 years ago

All configuration code can be put here: https://github.com/Koenkk/zigbee-shepherd-converters/blob/master/devices.js#L571

felixstorm commented 4 years ago

Maybe that was not clear, but the whole stuff inside ubisys.js is for one-time-configuration.
An example: the shutter controller needs to know how many seconds the blinds take to go from fully open to fully closed and also for the other way around (which sometimes differs a bit) in order to be able to go to a specific position, e.g. to close the blinds 80 percent. Therefore there is a one-time calibration procedure that can be controlled by setting specific attributes, moving up and down in a predefined order and in the end setting the attributes back to normal. Another example: The dimmer can work with spring loaded buttons or with a normal rocker switch and can potentially have separate up and down buttons or just one - this can be configured by means of attributes in the manufacturer-specific cluster...

Koenkk commented 4 years ago

@felixstorm the two examples you mention can be done perfectly fine in the toZigbee converters, this seems similar to me as e.g. setting a sensor sensitivity (https://github.com/Koenkk/zigbee-shepherd-converters/blob/master/converters/toZigbee.js#L903). Note that you can also very easily address non-standard clusters there (just put the hex number as show in the link).

felixstorm commented 4 years ago

@Koenkk that was exactly the route I had taken in the first place, but then did not find a way to insert the necessary delays to wait for a complete run up or down (60-90 seconds) during shutter calibration. Therefore I had then switched to implementing an extension instead.

Anyhow I am fine for the ubisys extension to not be merged into the mainline currently if there apparently is no demand for it besides myself (I can simply keep and maintain it in my repo for now).

As support for ubisys devices in general does not depend on the extension, I will test it a bit more in my environment and then (probably in a few weeks when I am back from vacation) open a PR for it excluding the extension.
And in case other people should show interest for the calibration stuff in the future, then just drop me a note and we can then figure out what the best way is to implement it.

Thanks again for spending your time and effort on creating and maintaining this great ZigBee solution and feel free to close this issue if there is nothing more to add from your side.

Koenkk commented 4 years ago

@felixstorm could you provide me a bit more context on how such a calibration is done? We can look into the possibilities of adapting zigbee-shepherd-converters.

felixstorm commented 4 years ago

@Koenkk basic calibration sequence is as follows:

Just some ideas:

Koenkk commented 4 years ago

Option 2 sounds great, I think we can also do it without the zigbee object by passing the result of the previous action?

felixstorm commented 4 years ago

Sorry for the delay, just came back from vacation.

Option 2 without the zigbee object could probably work if the array returned from the converters is allowed to contain both objects to publish to zigbee as well as promises which would then all be processed sequentially so that we can write an attribute, wait, send a command, wait etc.

Koenkk commented 4 years ago

@felixstorm zigbee-herdsman has now landed in the zigbee2mqtt dev branch, you should be able add the calibration to https://github.com/Koenkk/zigbee-shepherd-converters/blob/zigbee-herdsman-converters/converters/toZigbee.js.

Could you also make a PR with the manufacturer specific clusters? I will then modify this further.

felixstorm commented 4 years ago

@Koenkk Cool, looks good, thanks!

I am a bit busy at the moment, so it might take a few days until I get to it, but I will let you know as soon as I have added the calibration.

felixstorm commented 4 years ago

@Koenkk I did a first implementation:
https://github.com/felixstorm/zigbee-herdsman/commit/bd0a8e056a5a3b337e262f231a729ba9cdd2a81a
https://github.com/felixstorm/zigbee-shepherd-converters/commit/f30632743c8ea61fd0c1bb258087c3b3f61d59ce Does this look ok for you or would you like me to make some changes (console.log is just in there as a placeholder, it's definitely not meant to go into production :wink:)?

There are some open questions where I could use some advice from you:

Koenkk commented 4 years ago
  1. Yes, just make this PR and I will continue with that.
  2. OK with that
  3. Can't we abstract this to a higher level? e.g. it might be useful to the user at which point it has been calibrated.
felixstorm commented 4 years ago
  1. Thanks!
  2. Thanks again!
  3. To be honest, I do not think that reading the configuration parameters is of much use besides troubleshooting or verifying that everything is configured correctly. It might be an idea to have some sort of repository similar to state.json were various (configuration) parameters from all devices can be saved for reference (potentially with a timestamp like LastRead or so), but I did not deem it important enough at the moment to justify the effort...
Koenkk commented 4 years ago
  1. I agree, just remove the convertGet for now.
felixstorm commented 4 years ago
  1. Couldn't we just leave convertGet in there and use a logger to output the results? Then there would be at least some possibility to read the attributes in order to verify that the calibration has generally succeeded for a specific device. I would have no problem streamlineing the logger output for readability a bit more than just raw JSON...
Koenkk commented 4 years ago

Also ok with that!

felixstorm commented 4 years ago

Ok, thanks - then I'll wait for the logger to become part of meta and then file a PR for the ubisys stuff, or would you like me to also file a PR for adding a logger to meta?

Koenkk commented 4 years ago

@felixstorm you can also file a pr for that, it shouldn't be more than changing https://github.com/Koenkk/zigbee2mqtt/blob/dev/lib/extension/entityPublish.js#L137 

const meta = {
                endpoint_name: topic.postfix,
                options,
                message: json,
            };

to 

const meta = {
                endpoint_name: topic.postfix,
                options,
                message: json,
                logger,
            };
Koenkk commented 4 years ago

Necessary changes are applied so I assume that this can be closed now.

felixstorm commented 4 years ago

Thanks! Will open a PR for it now...