Adds better handling of command classes#134
Conversation
dynamically creates a node class based on what command classes the node has. There is a class that represents every single command class in Z-Wave. This is very helpful in keeping code organized but also being able to write methods/properties that are specific to a command class. I also added a mechanism to identify what command class a node has. we use simple equality testing. I have provided extremely detailed comments on how the whole process works. please be sure to read the src_api files.
Corrected a typo and added a needed import I also added .idea (PyCharm project files) to gitignore.
|
Does this update impact api interface ? |
|
I do not believe it does impact the current API at all. I submitted this PR a while ago I would have to check it to see if it does change anything. I am pretty diligent in making sure that API does not get broken when i make any changes to a project that I have not initially coded. Never want to pee in someone's cheerios if ya get my meaning. I also would have made special mention if I had to change any of the API. That would have been the first thing I would have stated. so that it was clear and a decision as to that specific API changing could be tolerated. and if it couldn't then it would save a whole bunch of time testing and checking. So with that being stated. It should extend it. and also adds some convenience methods for specific device classes. I do want to mention that mechanics that marry the device to to the proper python representation of that command class does function without a hitch (none that I have come across). The convenience methods on the other hand most of them are not tested. I created these methods working off of the ZWave specification as well as the openzwave source code. The dimming and binary switch classes have been tested and the rest has not. I do pretty good at reading API and source. But I am not perfect and mistakes do happen. I think the biggest feature of this is being able to compare class instances against command class constants. I am not a big fan of voodoo magic code. In this case it was the only way to achieve the desired results without breaking API. You should give this a test drive and make sure that API is not broken. you are going to know more about the API of python_openzwave then I. Plus it will give you a better understanding of the mechanics that make it work and the benefits that it will bring. I will be more then happy to help out with this in any way that I can. I do feel it is a really good addition to python_openzwave. If I remember correctly I commented large portions of the code describing step by step what is taking place and how it all works. If you need to to modify any documentation explaining the additions provided with some code examples I would be more then happy to help out with it. I cannot stress this enough. You need to know how it functions. I do not plan on going anywhere, but ultimately you are the maintainer of this library and you need to understand fully how the code works. There could also be a better way to go about what I did. The more people sharing ideas will always lead to better code. |
|
I am working on documenting this better and I am going to add it to sphinx to be compiled into HTML docs. |
|
I ned some help.. I want to expand upon the which ever one allows for multiple devices for a single zwave node. I cannot seem to locate any kind of information on how to use this specific zwave feature. There are enough devices out there that use this feature. examples would be secondary scene controllers (keypads). multi zone lighting controllers. I believe that possibly some sensors also use this. I am thinking maybe making the command class iterable and it would yield additional devices under that node. But without any clear documentation explaining how to obtain the devices leavs me in the dark. To be honest any help in adding functions/properties to any of the command classes would be a huge help. even if it is just information on what some of the command classes actually do (in layman's terms, so no geek speak) I really want these command classes well documented. If this PR gets added to the core code these classes will end up being the most used objects in the library. so we really need to provide clear and concise docs for them. and also as many convenience methods/properties to remove the complexity of use.. I did go over the openzwave code and I added as much as I could based on the information I was able to obtain from it. Unfortunately openzwave does not have a class definition for eeach command class that zwave supports. |
added some additional methods. remove a lot of repeat iteration code added __getitem__ to the command classes to obtain value(s) added __iter__ to return value.values() The __getitem__ works in a multitude of ways. * node[value.value_id] we can specify a single value_id and that is what will get returned if available otherwise IndexError is raised * node[(value.value_id, COMMAND_CLASS)] a tuple that contains the value id and the command class. this is going to search for a value id and the value must belong to the command class otherwise IndexError is raised * node[value.label] I tossed in checking the label of thee value as well KeyError is raised if not found * node[(value.label, COMMAND_CLASS)] tuple with the label and the command class. raises KeyError * node[1: 3] slicing. I organize all of the indexes for the values in ascending order then create a list of values in that same order and return the slice * node[(slice(1, 3), COMMAND_CLASS same as above except remove any values that are not in the command class * node[(None, None)] this is the same thing that __iter__ does * node[(None, COMMAND_CLASS)] this returns all values in a command class or an empty list * node[-1] this returns the lowest indexed value raises IndexError * node[(-1, COMMAND_CLASS)] returns the lowest indexed value in a command class raises IndexError
|
the latest commit is going to fail. I am in the process of doing some reorganizing. I split the command classes into separate files. It was starting to get congested in a single file once i started adding properties and methods to the command classes. I also fixed some potential problems and removed a lot of repeat code. |
I am not sure if this is going to work correctly. but it is worth a shot.
|
Hi, I want to start by saying Python is foreign to me!. From a language perspective this PR might be very necessary. IDK. The following is what I see as potential issues. Please educate me if I'm wrong. I have two concerns here. Mainly ideological...
Finally - Just want to point out (I've seen a few references here and there to this). In the Dev branch, you should not rely on the ValueID Labels to identify what a Value is in code, as we now allow localization in Dev branch. So if they are not using English, all of a sudden things break. The Index of a ValueID (in combination with the CC id) are always static, and will not change, so you can always count on index 5, command class 84 to always be index 5, command class 84! (just a random example). The Caveat there. Things like UserCode will expose potentially 100's of usercode slots as a incrementing index. But the starting index, and maximum range will always remain static, so you can write something to say |
|
ok great as far as the indices go. I was not sure if the indices were static or now. when I had first started with this PR I was not that familiar with openzwave it's self. I did not know if the labels were standardized by Zwave Alliance and or hard coded into the device it's self. I did later find out that they were added by openzwave and not the device. Tho almost all of the labels are identical to what is used in the ZWave specification. The fact that the indices are static is going to be the way to go. Is there a list or some kind of documentation anywhere that lists the command classes and what values are at what indexes? The python_openzwave library ni it's current state is really geared to the really knowledgeable developer. There are a tremendous number of DIY'ers out there that are picking up the programming books and using google and learning how to program on a small scale. This library is not the easiest to understand when getting started with it. The command class objects are also more for code organization. and only having methods and properties where they aught to be. and by having the various command classes represented it will be much easier to maintain the code and add new features. because all of the code specific to a command class will be in a single location. instead of a piece here and a part there. when you state that openzwave is designed to hide the protocol and that the user does not need to know about command classes. the way python_openzwave was written you do have to know them. def get_values_by_command_classes(self, genre='All', \
type='All', readonly='All', writeonly='All'):
values = dict()
for value in self.values:
if (genre == 'All' or self.values[value].genre == genre) and \
(type == 'All' or self.values[value].type == type) and \
(readonly == 'All' or self.values[value].is_read_only == readonly) and \
(writeonly == 'All' or self.values[value].is_write_only == writeonly):
if self.values[value].command_class not in values:
values[self.values[value].command_class] = dict()
values[self.values[value].command_class][value] = self.values[value]
return valuesNow I think that instead if passing a command class and having to perform a search over the values returned. it would be far easier to do the following. for node in network.nodes:
if node == COMMAND_CLASS_SWITCH_MULTILEVEL:
node.level = 30this makes it far easier to narrow your search and not have to provide all of the arguments that would be needed if using get_values_by_command_classes. It is by far easier to understand what is happening. the same mechanics as above can be used in determining what the command class is for a value also. the code below is an alternative way of isolating specific value(s). get all values from a specific command class node = network.node[10]
values = node.values[(None, COMMAND_CLASS_SWITCH_MULTILEVEL)]value at an index if the command class matches node = network.node[10]
value = node.values[(3, COMMAND_CLASS_SWITCH_MULTILEVEL)]label and command class node = network.node[10]
value = node.values[('Level', COMMAND_CLASS_SWITCH_MULTILEVEL)]just the label node = network.node[10]
value = node.values['Level']just the index node = network.node[10]
value = node.values[3]This is still very much a WIP and is not complete. the propose to it is to only expose methods/properties that would be relevant to a specific device type. there is going to be methods in the ZWaveNetwork class that will return say all dimmers and another for all switches and security sensors.. things of that nature that makes it easier to bundle devices types together. Here are a few thought/questions of things that would make life easier from the user standpoint as well as from the developer standpoint. I would love to have some feedback on them. or if it is even possible. People do not name their devices 1, 2, 3, 4, 5, 6 and so on and so forth. they give them names like "under cabinet lights" "south window sensor" "ceiling light" "track light" these seem pretty generic kind of names. But once you add a location or "room" to it then you know exactly what device it is. The grouping of devices by location should be something that is done by the library. This would make it easier for a developer adding the library to their code to be able to provide the user with a layout that would be familiar to them instead of just dumping all of the devices into a single page. (seen this done). make it easy to program and it will be used and the end result will be a much better experience for the user. I know that when I am going to add a dependency to a library one of the things that is top on my list of things i look for is ease of use. and also the code readability when using the library. The example below shows both the ease of use and the readability. for room in network.rooms:
print(room.name)
for device in room:
print(' ', device.name)this is how most users visualize their ZWave network. Here is something that would really make life easier if a hardware failure occurs. No one likes having to redo their whole HA implementation because something like a ZStick went bad. Hardware does fail that is a fact. But to have to delete your whole HA setup and start from scratch just for a simple device change is not something that makes me happy, I do not think that anyone would be for that matter. On the software side of things if a device had an ESN then that is what all of the settings would be married to. like the name of the device. and the location. so when a user adds the nodes to the new controller and goes and fires up their HA software everything is all happy and goes along it's business. With how it is now they would have to basically start from scratch and punch all of the labels in and the locations. and point their scripts/automations to the proper devices. This can be very daunting to have to do if you have a network of 100+ devices and a bunch of scripts that perform specific tasks based on the node id. And depending on the software the library is being used in it could make things even worse. |
its WIP in the Dev branch. I'm slowly converting the indexes over to ENUM's and will eventually move them all to its own header file.
We have a different target audience then. considering the library is C++, obviously I'm expecting a different level of programing knowledge.
This is my concern, and I don't agree its easier from a OZW (and wrapper maintainability perspective). With these proposed changes (assuming its the preferred way to use the wrapper from Bibi etc), Esentially, you have now 3 implementations of each commandclass - One in the Core C++ library. One in the Python Wrapper, and now, you have to implement knowledge of the command classes in the application. If the C++ library updates a CommandClass to the latest version, from a end user perspective, they have to wait till the python wrapper updates, then the application before they can take advantage of the new features the CommandClass offers. OZW currently has around 58+ commandclasses supported, with many more than are not. And it means that a developer has to decide which ones to implement, and does he even know that thing like SensorAlarm are depreciated for new devices, or that every release of the Z-Wave Spec, there are new Notificatation and Sensor types added to the Notification or SensorMultiLevel classes? The way it is now, if your writting a GUI application for example, and you see a ValueAdded Notification for a ValueBool type, you can just add it to the interface, with the label and Min/Max values etc exposed from the ValueID - and Bingo, your application now supports the latest/greatest commandclasess without you having to know a THING about how the Z-Wave Spec works etc. And for those cases, where a more refined interface might be required (say for instance the Color CommandClass you want a Color Wheel), then the information is exposed in the ValueID, and you can take a different code path if you want to layout the interface differently, but if you chose not to, then you dont loose functionality (but maybe end up having a rather clunky interface - at least it still works). With your PR, a application is going to have to write upto 58 different implementations to cover the full range of CommandClasses we support today. So maybe for the "newbie" developer, I can see your point. But the biggest developers (applications) of OZW (and the python library) tend to be well established HA software (both OpenSource and Commercial). I'm not against your PR in any way - I can understand your point of view, the way we do things is not necessarily easy to grasp for new developers, but I hope that the existing interface the python wrapper has continues to be maintained as well. Maybe you can structure your PR in such a way as its a additional "wrapper" around the python interface? Also - I'm assuming (with fingers and toes crossed) that you will continue to maintain the python CommandClasses as well, otherwise as thing get updated, the python implementation would get stale. (also - As I said in the earlier comment, please look into the Notification CC (Previously Alarm CC) in the Dev Branch (and we will also extend this to the SensorMultiLevel soon) and see how you are going to support "dynamic" notification types. The indexes will remain static for the NotificationTypes defined in NotificationTypes.xml, but adding new Types is just a matter of updating the XML file, and pushing it to the repository. The Dev Branch can pull down the latest copy of config files etc automatically thus there is no need to "release" a new version to support newer Notifications. )
(and this is also a demonstration of my point above. The application now has to know that the SwitchMultiLevel command class has a variable called "level" :)
I consider this a "policy" - How you want to visualize your network may be different from someone else, versus a 3rd person. All of a sudden OZW ends up having all these utility functions to support the different needs of a different users. Then you get into things like "sort them ascending/decending/numerical/alphabetical, or even english versus chinese!). Most GUI applications allow you to easily setup tables that handle that for you... so I consider it outside the scope of the core OZW library.
The ManufacturerSpecific CommandClass exposes a SerialNumber of the device, but its in a later version of the commandclass and a lot of manufacturers either don't support the latest version of the CommandClass (thus its not available), or its all 0's, or even in some cases, its the same across multiple devices... so while the feature is there, its going to be years before I think you can use it to identify individual devices. I know one HA software that stores a identifier string in the NodeLocation ValueID (but doesn't display it to the user) to overcome this, but its not reliable. Some devices will do a factory reset when excluded/included, thus wiping this information out. |
|
I'm only looking at this from and end-user perspective and thus cannot comment on the merits and intentions of this PR. I do however note that there are quite a few large downstream projects with dependencies on OZW and its python wrapper. The understandable slow pace of development is already causing some pain in downstream projects. This looks like it will cause considerable work for other projects for little additional gain. That's just my 2ct. |
I disagree with this. You're now forcing every user of the library to conform to your "view of the world" or they have to write code to adapt it to their model. Libraries like this should provide general functionality that allow clients to use the functionality as they see fit. The point of OZW is to support interaction with zwave devices and the python wrapper is for exposing that functionality to python projects. This would essentially create a quasi application model that has nothing to do with the intent of the library and therefore should be left as a concern of the applications that use the library. |
|
this is not directed at you @Fishwaldo. this is for anyone who decides to make a comment Let me make sure that everyone understands this. first. This PR is not complete. It is still being worked on. It has been here for 8 or 9 months. It is not anything new. Now why all of a sudden people are chiming in is only because @Fishwaldo made a comment. and for absolutely no other reason. If anyone would like to inject useful comments I am all for it. but in order to do that you will need to read each and every single comment that is posted here as well as all of the changes that are made. I do hate it when bits and pieces of a comment are taken out of context. If a comment is not going to help in any way with this PR then there is no need to make a comment at all. Second. This PR is nothing new. python_openzwave is already designed this way. all i did was expand it.
all this did was add support for ALL of the Z-Wave command classes and not just the few mentioned above. I am pretty sure that this was the original design idea it simply never got finished. @bibi21000 would be the person to ask if that is the case. the main portion of the communications between python_openzwave and openzwave is done through the use of the notifications. there are some methods/functions where a call is made directly made. The only way python_openzwave knows of the existence of a node on the network is from notifications. the data that has come in from openzwave would be just that data. not a C object. everything has to go through a conversion process. so passing an instance of a class is not easily done if it can even be done at all (I do not think it can be). What this means is that python_openzwave has no knowledge of what nodes the openzwave library holds reference to. It makes assumptions that a node still exists. you made a statement about hiding the protocol. and the use of the command classes. This MUST be done. you cannot hide this portion of the protocol. it coupled with the device type and sub type are the only mechanisms to be able to identify what a device is and what it supports. They have to be accessible by the user of the library. How would someone making a GUI interface be able to determine if they need to use a switch type of control or a slider control from just looking at the values alone? and how would they be able to determine what icon to use for the device by the values alone? the command classes are a necessity. and the device type and sub type are as well. and they need to be the core of how the program operates to be able to provide the correct methods and properties for a specific node. I don't think that you would provide thermostat related methods and properties for a node that is a dimmer switch. It does not appear that way in the code for openzwave, Now because this library has 0 knowledge of any references held in openzwave and there is no mechanism to be able to utilize any of the code contained within the command class structures in openzwave this is the way to go about doing it. There is another option. Completely remove Cython from being used. Write all of the necessary data conversions so python can compile the openwave library and create a pyd file (a dll that can be directly accessed by python). another option is to write a C connector that can be loaded by the ctypes library in python to be able to have access to the main program. Now if your issue is because i used the command class names for the symbols. well those can be changed to any name you would like. it does not change the need for using them and how it works. it is simply an identifier. |
5 participants
dynamically creates a node class based on what command classes the node has. There is a class that represents every single command class in Z-Wave.
This is very helpful in keeping code organized but also being able to write methods/properties that are specific to a command class.
I also added a mechanism to identify what command class a node has. we use simple equality testing.
I have provided extremely detailed comments on how the whole process works. please be sure to read the src_api files.
I have this implemented in a version of python openzwave which has been highly modified to be able to compile using Cython on Stackless python 2.7 x86 running in Windows 7 x64 compiled with Visual Studio 2017. I removed the biys specific to the command classes. I think I grabbed them all. if something does not function properly please let me know and I will fix it.
This is also to be used as an idea not necessarily directly added, as some of the things I did break API.
It does provide a much cleaner way to be able to add code for command classes that are specific to that command class., that is why I thought I would share it.