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.

Sign up for GitHub

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

Jump to bottom

Add ability to add endpoints and clusters to a device via the XML #260

Merged
merged 2 commits into from
Nov 26, 2018
Merged

Add ability to add endpoints and clusters to a device via the XML #260

merged 2 commits into from
Nov 26, 2018

Conversation

cdjackson
Copy link
Contributor

@cdjackson cdjackson commented Sep 12, 2018 •
edited
Loading

Adds the ability to add clusters to a device through the standard XML file. The endpoint can be defined and added, then a list of input and output clusters that will be added to any clusters already in the device.
Closes #247
Signed-off-by: Chris Jackson chris@cd-jackson.com

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/zigbee-binding/15763/1335

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/eu-zigbee-dongle/31069/144

hsudbrock
hsudbrock reviewed Oct 22, 2018
View reviewed changes
Copy link
Contributor

@hsudbrock hsudbrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Chris; to verify my understanding of the change, do I understand this PR correctly as follows?

  • In a static thing type for the ZB binding, for each channel type one can now (in addition to the endpoint that can already be specified as channel property) also specify as channel property (a) a set of input clusters, (b) a set of output clusters, and (c) a profile ID (all optional);
  • if the device did not report the endpoint specified as channel property, then the endpoint will be added to the node object by the thing handler, together with the provided sets of input and output clusters;
  • if the device did report the endpoint specified as channel property, then the endpoint on the node object is augmented with the specified input/output clusters by the thing handler (keeping all input/output clusters reported by the device, even if they are not specified in the channel property).

hsudbrock
hsudbrock reviewed Oct 22, 2018
View reviewed changes
...nhab.binding.zigbee/src/main/java/org/openhab/binding/zigbee/handler/ZigBeeThingHandler.java

List<Integer> staticClusters;
boolean modified = false;
staticClusters = processClusterList(endpoint.getInputClusterIds(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must admit, that it was a little hard for me to understand what is happening here. I think the source of this - for me - is that the intention of processClusterList was not evident to me, maybe because the method name is somewhat generic, maybe because the list returned by the method may mean different things (if the list is empty, it signifies that no additional clusters were statically defined, otherwise it contains the list of all statically defined and non-statically defined clusters), maybe something else.

I have toyed around with the code, trying to make it easier to read from my personal view, and came up with the following code (replacing lines 230--246 and the method processClusterList) - do you also think that it makes the code somewhat easier to understand? Or is this just personal preference?

            boolean modifiedClusterIds = false;

            Collection<Integer> staticInputClusterIds = getStaticClusterIds(properties,
                    ZigBeeBindingConstants.CHANNEL_PROPERTY_INPUTCLUSTERS);
            if (!endpoint.getInputClusterIds().containsAll(staticInputClusterIds)) {
                logger.debug("{}: Forcing endpoint {} input clusters {}", nodeIeeeAddress, endpointId,
                        staticInputClusterIds);
                staticInputClusterIds.addAll(endpoint.getInputClusterIds());
                endpoint.setInputClusterIds(new ArrayList<Integer>(staticInputClusterIds));
                modifiedClusterIds = true;
            }

            Collection<Integer> staticOutputClusterIds = getStaticClusterIds(properties,
                    ZigBeeBindingConstants.CHANNEL_PROPERTY_OUTPUTCLUSTERS);
            if (!endpoint.getOutputClusterIds().containsAll(staticOutputClusterIds)) {
                logger.debug("{}: Forcing endpoint {} output clusters {}", nodeIeeeAddress, endpointId,
                        staticOutputClusterIds);
                staticOutputClusterIds.addAll(endpoint.getOutputClusterIds());
                endpoint.setOutputClusterIds(new ArrayList<Integer>(staticOutputClusterIds));
                modifiedClusterIds = true;
            }

where

    private Set<Integer> getStaticClusterIds(Map<String, String> channelProperties, String propertyName) {
        return Arrays.stream(channelProperties.get(propertyName).split(",")).map(Integer::new)
                .collect(Collectors.toSet());
    }

@cdjackson
Copy link
Contributor Author

Hi Chris; to verify my understanding of the change, do I understand this PR correctly as follows?

Yes, you have the correct understanding.

for each channel type one can now (in addition to the endpoint that can already be specified as channel property) also specify as channel property (a) a set of input clusters, (b) a set of output clusters, and (c) a profile ID (all optional);

Now that your write this, it makes me thing this is wrong. However, it does look like this is what I coded, and thinking about this some more, I'm not sure it's the best approach. I think this should be implemented at THING level rather than at CHANNEL level. This doesn't fundamentally change the approach - just that I think it makes more sense to override the endpoint and clusters at thing level rather than for each channel.

WDYT?

@hsudbrock
Copy link
Contributor

I think this should be implemented at THING level rather than at CHANNEL level.

I agree, Chris - I also think that adding non-discovered clusters to an endpoint or even non-discovered endpoints fits better to the thing than to the channel.

Maybe this relates to another thing I was thinking about in the context of this PR: With the currently implemented approach, one must define channels in a static thing type (and cannot rely on the automatic channel creation by the binding). When moving the static cluster/endpoint configuration to the thing level, maybe there's an option auto configure the thing type such that channels are automatically created (e.g., via a thing type property autoCreateChannels or something alike). I cannot say whether this is an edge case or not (as I do not know how many devices are out there with incomplete/wrong simple descriptors - and for which of those devices the automatic channel creation is useful).

@cdjackson
Copy link
Contributor Author

When moving the static cluster/endpoint configuration to the thing level, maybe there's an option auto configure the thing type such that channels are automatically created

I'm not sure I understand this - so you mean you want to auto detect channels, even if using a static thing definition? I guess that's possible, although is there a use case for this? If you aren't going to define the channels, then the only thing you're effectively setting is the label?

@hsudbrock
Copy link
Contributor

If you aren't going to define the channels, then the only thing you're effectively setting is the label?

No, besides the label one would also define statically those endpoints and clusters that are not correctly reported by the device (which, as far as I understood, is the goal of this PR, right?).

I guess that's possible, although is there a use case for this?

That's what I meant with "I cannot say whether this is an edge case or not". Will one usually need to define a static thing type with static channel definitions for those devices that do not correctly advertise their endpoints and clusters, or is automatic channel creation good enough for those devices? I don't know, it was just a thought. If you think that it is more likely that one will need a static thing type including channel definitions anyway, than there's no need to provide an option for automatic channel creation...

@cdjackson
Copy link
Contributor Author

cdjackson commented Oct 22, 2018 via email

@hsudbrock
Copy link
Contributor

I see your point; no need from my side to add now, we can still add it in case it turns out to be useful for several devices...

@puzzle-star
Copy link
Contributor

Hi @cdjackson and @hsudbrock

When moving the static cluster/endpoint configuration to the thing level, maybe there's an option auto configure the thing type such that channels are automatically created

Well, indeed we had some discussion on the forum and I proposed this approach (and implemented the code changes to test it), so at least it is possible.

Having static device descriptions in the XML is good (BTW, I confirm this pull request is working, as I tested it yesterday), but there some use cases where some users (maybe not normal users or not under normal situations) may need a "real" static thing definitition with its endpoints and clusters defined in the thing configuration itself:

  • Non statndard devices failing initialization (i.e. not responding to node descriptor requests): Xiaomi two button switch (battery powered)
  • Developers wanting to test devices without having the rebuild the whole JAR and restart the binding every time they change something (myself, at least... :-) )

Modifying the xml and adding these at channel level was my initial approach (indeed @cdjackson's code is based on my initial implementation), but later I came to the thought it might be better to do this at thing configuration level, at least for some other use cases such as the mentioned above, but IMHO both approaches should coexist.

BTW, if waiting for somebody to test this pull request, as I said I have and it does work. As soon as this is merged, I will add a new pull request with the xml definition for the (old version) of the Xiami ht sensor needing this to work.

Pedro

puzzle-star
puzzle-star approved these changes Oct 27, 2018
View reviewed changes
Copy link
Contributor

@puzzle-star puzzle-star left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works OK (tested wit Xiaomi ht sensor needing this patch)

image

@cdjackson
Copy link
Contributor Author

cdjackson commented Oct 27, 2018 via email

@puzzle-star
Copy link
Contributor

As we discussed on the forum, I don’t like the idea of adding configuration parameters that allow the user to add clusters etc to the device. To me this is a real mess, and it’s open to serious abuse by people who have no real idea what they are doing - and then it will be up to others to solve the issue.

Hi @cdjackson, by this we are closing the door to anybody needing this based on a potencial "misuse" from somebody not knowing what they are doing: if somebody abuses the functionality, on their own system, up to them, but really, why is it so bad to allow advanced users to define their own devices by configuration, specially when it is not really hard to implement in the binding as is?

I proposed to code it myself, and once it is done to your satisfaction I really do not understand why vetoing the feature.

@cdjackson
Copy link
Contributor Author

cdjackson commented Oct 27, 2018 via email

@puzzle-star
Copy link
Contributor

puzzle-star commented Oct 27, 2018 •
edited
Loading

Fair enough, and I agree we discussed already on it, but this still does not solve the situation where a device is capable of joining the network, but does not correctly report the power decriptor or any endpoint at all, as the discovery participants (upon which the xml solution is based) are not called util discovery is complete, but discovery hangs forever on these devices.

Any thought on how to solve this without pure static thing definition? Or do we just discard this kind of devices (there are several, they are cheap, and people like them...) ?

@cdjackson
Copy link
Contributor Author

cdjackson commented Oct 27, 2018 via email

@puzzle-star
Copy link
Contributor

puzzle-star commented Oct 27, 2018 •
edited
Loading

It does.

Discovery participants are not called till a device is discovered. In the code "discovered" means:

return nodeDescriptor.getLogicalType() != LogicalType.UNKNOWN && endpoints.size() != 0;

So if a device does not report any endpoint, or fails to respond to the nodedescriptor, it will never be handed over to a discovery participant.

This can be solved by allowing a static thing definition with a configuration parameter to setup an initial description and endpoints for the node. This is not "just somebody asking for whatever feature", just trying to solve an issue with existing devices.

Of course, if there is another way round, such as as passing the newly discovered devices early to a discovery participant (or maybe a two pass approach, once before discovery is complete and once after), I do not have a specific preference for the static thing part (even if not willing to write a full discovery participant just for this, as it would be generic and then what is the point in not doing it in the binding itself?)

@cdjackson
Copy link
Contributor Author

cdjackson commented Oct 27, 2018 via email

@puzzle-star
Copy link
Contributor

puzzle-star commented Oct 27, 2018 •
edited
Loading

Well, I do have these devices, and many more people :) There is plenty of people who is trying to use Xiaomi battery powered switches. These are quite unexpensive and suprisingly better looking than other expensive options.

For some reason, these fail to report the node descriptor and endpoints 99.99% of the times. Indeed, in all my tests, I only saw this work once, and have not been able to reproduce it. There are custom drivers for these in SmartThings, MQ gateway, and seen implementations all around, so these seem to be quite popular.

Agreeing with you anyway these are not the best "ZigBee" devices... I do not think devices that do not properly report their endpoints are better either, and we are trying to support these through this PR.

I have two ways of implemeting the PR (always under the discovery participant approach):

  • Make it in such a way that the device still needs a xml and discovery.txt input (I think you will like more this approach)
  • Allow for the endpoints and clusters to be input in config without need for a specific xml (would you approve this if clean enough?)

@cdjackson
Copy link
Contributor Author

cdjackson commented Oct 27, 2018 via email

@puzzle-star
Copy link
Contributor

puzzle-star commented Oct 27, 2018 •
edited
Loading

I deleted the PR with these changes, but will post it back, as it is probably clearer seeing the code.

I answer your questions (and flow below):

If the device gets added, and all you know about it is its IEEE address, then I guess you are now adding everything manually?

Not everything, only these devices. But yes, I use something like:

Thing zigbee:device:523a066c:00158d000163e591 "Two Button Switch (Bedroom - 1)" (zigbee:coordinator_telegesis:523a066c) [zigbee_macaddress="00158D000163E591", zigbee_endpoints="1:6;2:6;3:6"]
Thing zigbee:device:523a066c:00158d00023a8315 "Two Button Switch (Bedroom - 2)" (zigbee:coordinator_telegesis:523a066c) [zigbee_macaddress="00158D00023A8315", zigbee_endpoints="1:6;2:6;3:6"]
Thing zigbee:device:523a066c:00158d00023393a7 "Two Button Switch (Living Room)" (zigbee:coordinator_telegesis:523a066c) [zigbee_macaddress="00158D00023393A7", zigbee_endpoints="1:6;2:6;3:6"]

For a proper PR, I would remove part of the needed config (i.e. manually setting the bridge). I just took the quick approach for testing, as I did not see a fast path to do it automatically in code.

I know that people want to buy cheap sensors etc, but on the other hand, this binding needs to be ZigBee compliant and there are requirements here that may conflict.

The idea is to make it in such a way that it won't interfere with normal discovery or devices. It does not in my installation, and I have >40 devices at home under the binding

One point though - if the device isn’t reporting its node descriptor and simple descriptor, then the following check that you referenced will always fail:
return nodeDescriptor.getLogicalType() != LogicalType.UNKNOWN && endpoints.size() != 0;
This code that you referenced is not even in the binding, so if the device is not returning this fundamental data, then I’m not sure how to handle this completely within the binding?

I did this in ZigBeeThingHandler.doNodeInitialization():

         // Check if discovery is complete and we know all the services the node supports
-        if (!node.isDiscovered()) {
+        if (!configStatic && !node.isDiscovered()) {
             logger.debug("{}: Node has not finished discovery", nodeIeeeAddress);
             updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.NONE,
                     ZigBeeBindingConstants.OFFLINE_DISCOVERY_INCOMPLETE);

Being configStatic a check previously done on the static config being present.

If you can find a way to code this up in a separate bundle, that would be fine with me and I think a nice way forward. If this needs changes to the discovery participant concept, then feel free to propose them and we can evaluate.

There are a couple of minor changes needed to the discovery participant concept as above (just to skip the need for the discovery to be complete on certain devices), and if this is done, together with the XML approach, no separate bunde would be needed, as it would benefit from the XML definition working.

The needed changes to the existing participant are really small to even skip the need for the XML, but if you are not supporting this I would focus on making it work with the XML, and once it is working try building a bundle for pure static definition for use by anybody who wants it... but this could come later.

The flow (will be better if implemented properly, but this is how it works today for me):

  • I add the device normally to the network through OH
  • I appears as unknown device (and it is never passed to a discovery participant as is does not complete discovery)
  • I manually add it to a thing file as above
  • I then goes through the discovery participant part, as it has configuration that makes (!configStatic && !node.isDiscovered()) evaluate to false (as configStatic evaluates to true)
  • The thinghandler adds the endpoints and cluster to the device (this could be replaced by just adding the type so that it matches a XML, and then this part would do it)

I will cleanup and send back the PR with my changes as they are, in case you want to have a look, and we could start changing things from there, if OK for you

@cdjackson
Add ability to add endpoints and clusters to a device via the XML
2264c6b 
Signed-off-by: Chris Jackson <chris@cd-jackson.com>
@cdjackson
Copy link
Contributor Author

@hsudbrock it's actually not so easy to add this at thing level. The problem is there are multiple properties required to be set, and if we put this at thing level, then we'd need to find a way to group properties, or we would only be able to add a single set of data (ie a single endpoint).

Unless you have any ideas about how to group sets of properties I think it might be better to least this in the channel. Unfortunately both have downsides, but at least using the channel provides the functional flexibility.

WDYT?

@puzzle-star
Copy link
Contributor

Hi @cdjackson,

Just as an idea, when I added this at thing level in my own implementation, I used a configuration parameter called "zigbee_endpoints", with the following format: zigbee_endpoints="1:6;2:6;3:6"

  • A list of endpoint definitions separated by semicolons.
  • Each endpoint definition is the endpoint number, a colon, a list of input clusters, and optionally another colon followed by a list of output clusters
  • Each list of clusters is just a comma separated list of clusters.

So we could have something like zigbee_endpoints="1:6:6;2:6" for "Endpoint 1 with input and output cluster 6 and Endpoint 2 with input cluster 6"

After, I left the normal channel setup mechanism act on the manually added list of endpoints

BTW I did it as a configuration parameter to be able to set it manually, but I understand it could be done also as a property if putting this in configuration is a no go.

@cdjackson
Copy link
Contributor Author

cdjackson commented Nov 10, 2018 via email

@puzzle-star
Copy link
Contributor

puzzle-star commented Nov 10, 2018 •
edited
Loading

I agree this not the nicest approach, but with the close xml format I could not figure out a better way. I also setup other properties in my implementation by adding other config items, such as zigbee_[whatever_property]=value.

Doing it at channel level is another option, but it has other drawbacks, as you might need different overrides in different channels for the same endpoint, or you might need to override more than one cluster for the same channel (i.e. switchlevel channel using switch cluster, or color control using switchlevel an switch), which might make things even messier.

Between the two options, and considering the clean way would be at thing level, I would go in that direction. Maybe numbering the properties as "zigbee_endpoint_1", "zigbee_endpoint_2", "zigbee_devid", etc at thing level would be a better approach?

@cdjackson
Copy link
Contributor Author

cdjackson commented Nov 10, 2018 via email

@puzzle-star
Copy link
Contributor

puzzle-star commented Nov 10, 2018 •
edited
Loading

I would look then into a compromise in the amount of different properties.

  • I understand some must be unique accross endpoints: these do not need to be duplicated "zigbee_whatever=value"
  • Others may need to be setup in different endpoints: I would do just one per endpoint: zigbee_endpoint_1="input_clusters:1,2,3;output_clusters:1,2,3;whatever_property:value;..."

Would it be cleaner?

@cdjackson
Copy link
Contributor Author

cdjackson commented Nov 10, 2018 via email

@puzzle-star
Copy link
Contributor

puzzle-star commented Nov 10, 2018 •
edited
Loading

Even for basic cluster properties? Surprised about how many things can be different still being compliant with the standard... :-)
I was thinking anyway on being able to override power or node descriptor properties.

@cdjackson
Copy link
Contributor Author

cdjackson commented Nov 10, 2018 via email

@puzzle-star
Copy link
Contributor

Then I would go to a property per endpoint.
I have overriden node descriptor properties also, which may be used, for instance to force a device type for discovery participants. :-) But we can focus on overriding endpoints :-)

@puzzle-star
Copy link
Contributor

I thought you were thinking of a way to more generally overriding anything in the XML, if you are thinking only of overriding endpoints and clusters, then no need to discuss on that.

@cdjackson
Copy link
Contributor Author

cdjackson commented Nov 10, 2018 via email

@puzzle-star
Copy link
Contributor

puzzle-star commented Nov 10, 2018 •
edited
Loading

Well, in the end we will need to pick one choice

Oneliner:

  • zigbee_endpoints="json or list of properties in whatever format for all endpoints"

One property per endpoint:

  • zigbee_endpoint_1="json or list of properties in whatever format for endpoint 1"
  • zigbee_endpoint_2="json or list of properties in whatever format for endpoint 2"
  • ...

Several properties per endpoint:

  • zigbee_endpoint_1_input_clusters="1,2,3,..."
  • zigbee_endpoint_1_output_clusters="4,5,..."
  • zigbee_endpoint_1_whatever_property="value"
  • zigbee_endpoint_2_input_clusters=""
  • ...

Well, I have no preference, but looking at them the last one seems cleaner...

@hsudbrock
Copy link
Contributor

@cdjackson @puzzle-star I try to consolidate a little what was discussed so far, before giving my view:

  1. The goal of this PR is to support devices that report their endpoints and/or clusters incorrectly.
  2. To this end, the PR suggests to configure additional endpoints and/or additional clusters in the thing type XML file for the device.
  3. The schema for thing type XML files, however, does not provide structure that could be used for this purpose. The info about additional endpoints and clusters could be encoded in the following two ways in the thing type XML:
    3.1. Add the info as thing properties, by encoding the endpoint number in the property name.
    3.2. Add the info as channel properties; as each channel applies to exactly one endpoint, there is no need to encode the endpoint number in the property name.

First, I must say that I don't particularly like any of the two solutions, because they encode information in the thing type that is intended to be used by the thing handler, although (at least in my understanding) the thing type should be a description how a thing can be used (and not how it should be handled by the thing handler). The effect is, here, that pressing infos for the thing handler into the thing type causes problems, because thing types are not made for this. What do you think about a solution where the information is not encoded in the thing type XML, but rather in code, e.g., by extending the generic thing handler for those problematic devices?

In case you don't like a solution that avoids pressing the infos into the static thing type: I agree with you, @cdjackson, that leaving the info in the channel properties makes it easier to read it (both by a human, and by the thing handler). I don't see a way to group the properties on the thing level, other than introducing some JSON schema, and parsing the contents of a single property - but that makes the thing type definition hard to read.

One problem I see with this solution, though, is: How does one deal with thing types that have more than one channel for one endpoint. For which of those multiple channels does one add the cluster IDs as channel property? And what happens if cluster IDs are added to more than one channel for one endpoint?

What do you think about the following: Deal properly with specifying cluster IDs on multiple channels for the same endpoint, by adding all specified cluster IDs to that endpoint. This will permit to only specify those cluster IDs for a channel that are actually used by that channel. Example: One temperature channel for endpoint 1, adding the cluster ID for the temperature cluster, and one humidity channel for the same endpoint, adding the cluster ID for the humidity cluster. The thing handler would then add the endpoint 1 with both clusters on it.

@puzzle-star
Copy link
Contributor

@hsudbrock, as I commented, I have no preference, but this seems the right approach if the XML will not allow for a cleaner way to change the thing definition

Just one concern to take into account: some channels use more than one cluster (i.e. the dimmer channel uses switch and switchlevel clusters). Just a heads up to take this into account when bringing this into the channel description, and also some extra care needed on the order channels are loaded (if needed).

@puzzle-star
Copy link
Contributor

@cdjackson, any chance to merge this? As it is now it is indeed doing what is proposed at the channel level, and it is working. I recall you did not want to merge to put the config at thing level, but if it is at channel level, this seems to be working to start with.
What do you think?

@cdjackson
Copy link
Contributor Author

cdjackson commented Nov 24, 2018 via email

@cdjackson
Copy link
Contributor Author

@hsudbrock as @puzzle-star points out, we decided to leave this as it is - are you happy to merge this?

@hsudbrock
Copy link
Contributor

@cdjackson I think that it is ok to merge as is, but I will add issues for the two points I raised in my comments above, and did not get feedback so far, so that they don't get forgotten.

hsudbrock
hsudbrock approved these changes Nov 26, 2018
View reviewed changes
Copy link
Contributor

@hsudbrock hsudbrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@hsudbrock hsudbrock merged commit d54d832 into openhab:master Nov 26, 2018
@hsudbrock hsudbrock mentioned this pull request Nov 26, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hsudbrock hsudbrock hsudbrock approved these changes

@puzzle-star puzzle-star puzzle-star approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cdjackson @openhab-bot @hsudbrock @puzzle-star