python-openzwave
python-openzwave copied to clipboard
OpenZWave
1.6 Update - Handling Labels (python-openzwave)
Per the OZW 1.6 Release Notes, all labels or user visible strings are dynamic and can be localized.
Need to check no hard-coded string comparisons are made on things like ValueID's (eg, GetLabel() == "Switch" type calls). Instead you should be using the CommandClass, and Index values from a ValueID to determine what the ValueID represents.
Please close this if its not relevant.
quick question for ya. is there a list of index numbers available yet? an enum possibly that can be accessed?
In Progress on the Dev Branch: https://github.com/OpenZWave/open-zwave/blob/Dev/cpp/src/ValueIDIndexes.h
I should be able to merge this to master after this weekend.
OK great! when that is finished up I will see what I can do on our end to collect the information directly from openzwave. Instead of duplicating it and having to maintain 2 copies.
Which I did want to ask another question for you. But I am not sure where I should ask the question. It has to deal with that other PR I made fo the command classes But also it pertains to this one as well.
Currently Cython is used to generate code to be able to access the openzwave library. we really do not need to use Cython. I know enough C, CPP and C# to get me into trouble. But seeing as you are way more efficient with it then I, you will be better to say if it is possible or not.
If you have Python installed you will see 2 folders in the python installation directory. libs and include
In those 2 folders contain the API that can be used to be able to directly compile libopenzwave zwave into a dll (.pyd) that python is able to access directly. There would be no need for the "middle man". This would make it far easier to maintain when code changes are made to openzwave not to mention removes a huge amount of complexity. It would be cross platform without the need to do anything special or deal with platform specific compilers. This would all be handled internally in python. There are slight differences between Python 2 and Python 3, seeing as Python 2 is close to EOL it may not be worth worrying about.
I have some knowledge of the API nothing really bonkers. I do know that wrappers would have to be made because how the python API is you can only make function calls and not access classes or namespaces directly. So you would create a wrapper for initializing the connection that would make the call needed to create it, then set the manager into a global. create a function for each of the methods in the manager so python can use them and the code inside the functions would be able to make the calls to the proper methods. Because almost all of the data being used is integers (of some form) and strings the data conversion would not be terribly hard to do. I know this can be done with CPP I am unsure if it can be done to access C# code. Might be worth investigating.
I have attached the source code for a module used in a project I work on. It is coded in this fashion, It will be easier to understand what I am babbling about by looking at the code.
Sorry - I know nutz about the Python interpreter and it’s internals.
One of the original authors of the python wrapper has just reappeared and said he will have a look at things. He might be able to shed some light.
My only comment here is that lots of HA application are eagerly awaiting the wrappers to be updated for 1.6. Maybe get the existing wrapper running and start a new project to explore a new way to interface?
(I know one other tool that helps bridge the languages - swig. I guess it’s similar to cython but maybe it does it in a different way. Might be worth looking at as well)
I am familiar with swig. but again it is simply another layer. and swig just like Cython has it's own language so another thing to have to learn.
I do agree with you on updating it for 1.6 and that will be the goal at the moment. I am more then willing to work on the updates, I am going to need some guidance on the openzwave end of things and also how you would like to have everything done. The python coding part is pretty simple to do. I have some experience with Cython so small modifications are usually not a problem, If something larger needs to be done it will take me longer (learning curve).
I have looked at all of the issues you have posted so there are quite a few updates that have to be made. In order to update the code in a manner that you will be happy with I am going to need to be in contact with someone that has a really good knowledge of openzwave. If you are available to be that person great! If not point me to the person that is. Also need to figure out the best way to communicate I don't think that github issues are the best mechanism to use for development chatter.
I think we should keep the discussions here on github. There are a few people that have indicated they want to help out.
Ultimately @bibi21000 has the final say on the python wrapper as this is his repository as well.
yes he does.
From looking at openzwave and also python-openzwave there are 3 ways to go about doing this.
1.) I am going to have to create a whole second set of enumerations either in the Cython portion of python-openzwave or in python side of it. There is no way of getting the enumerations with their names to "transfer" over to python doing what I have explained in 3
For the sake of readability I would imagine that having the entry names would be important to have.
2.) hard code every index. doesn't lend to easy debugging of a problem and also makes the code more difficult to understand what is happening.
3.) there would need to be a method added to the Manager class in openzwave that would create an array of [entry name, enum value] pairs that can be obtained by passing a command class constant.
you can see here from the existing code for the Cython portion the enums are manually entered
cdef extern from "ValueID.h" namespace "OpenZWave::ValueID":
cdef enum ValueGenre:
ValueGenre_Basic = 0 # The 'level' as controlled by basic commands. Usually duplicated by another command class.
ValueGenre_User = 1 # Basic values an ordinary user would be interested in.
ValueGenre_Config = 2 # Device-specific configuration parameters. These cannot be automatically discovered via Z-Wave, and are usually described in the user manual instead.
ValueGenre_System = 3 # Values of significance only to users who understand the Z-Wave protocol.
ValueGenre_Count = 4 # A count of the number of genres defined. Not to be used as a genre itself.
cdef enum ValueType:
ValueType_Bool = 0 # Boolean, true or false
ValueType_Byte = 1 # 8-bit unsigned value
ValueType_Decimal = 2 # Represents a non-integer value as a string, to avoid floating point accuracy issues.
ValueType_Int = 3 # 32-bit signed value
ValueType_List = 4 # List from which one item can be selected
ValueType_Schedule = 5 # Complex type used with the Climate Control Schedule command class
ValueType_Short = 6 # 16-bit signed value
ValueType_String = 7 # Text string
ValueType_Button = 8 # A write-only value that is the equivalent of pressing a button to send a command to a device
ValueType_Raw = 9 # Used as a list of Bytes
ValueType_BitSet = 10 # Setting individual bits
ValueType_Max = ValueType_BitSet # The highest-number type defined. Not to be used as a type itself.
cdef extern from "ValueID.h" namespace "OpenZWave":
cdef cppclass ValueID:
uint32_t GetHomeId()
uint8_t GetNodeId()
ValueGenre GetGenre()
uint8_t GetCommandClassId()
uint8_t GetInstance()
uint8_t GetIndex()
ValueType GetType()
uint64_t GetId()
what would be your thoughts?
found a typo in Value_IDIndexes.h
ENUM(ValueID_Index_ClimateControlSchedule,
DOW_Monday = 1,
DOW_Tuesday = 2,
DOW_Wednsday = 3,
DOW_Thursday = 4,
DOW_Friday = 5,
DOW_Saturday = 6,
DOW_Sunday = 7,
OverrideState = 8,
OverrideSetback = 9
);
DOW_Wednsday should be DOW_Wednesday
There is also some inconsistency with the naming conventions of the enum values. specifically the camel-case and the use of _ instead of a space. I am not sure if this in intentional or not or if there is some kind of significance with when an _ is used and when one is not.
example with the _ being used
ENUM(ValueID_Index_WakeUp,
Interval = 0,
Min_Interval = 1,
Max_Interval = 2,
Default_Interval = 3,
Interval_Step = 4
);
example without it being used
ENUM(ValueID_Index_UserCode,
Start = 1,
End = 254,
Refresh = 255,
RemoveCode = 256,
Count = 257,
RawValue = 258,
RawValueIndex = 259
);
With "regards" to transfering the ENUM's over... you may not need to. (I'm not sure about how python handles enums though). All that fancy pre-processor magic turns each ENUM block into:
struct ValueID_Index_ThermostatOperatingState {
enum _enumerated
{ OperatingState = 0 };
_enumerated _value;
ValueID_Index_ThermostatOperatingState(_enumerated value)
: _value(value)
{
}
operator _enumerated() const
{
return _value;
}
const char* _to_string() const
{
for (size_t index = 0; index < _count; ++index)
{
if (_values()[index] == _value) return _names()[index];
}
return 0;
}
static const size_t _count = 1;
static const int* _values()
{
static const int values[] = { (ignore_assign)OperatingState = 0, };
return values;
}
static const char* const* _names()
{
static const char* const raw_names[] = { "OperatingState = 0", };
static char* processed_names[_count];
static bool initialized = false;
if (!initialized) {
for (size_t index = 0; index < _count; ++index) {
size_t length = std::strcspn(raw_names[index], " =\t\n\r");
processed_names[index] = new char[length + 1];
strncpy( processed_names[index], raw_names[index], length);
processed_names[index][length] = '\0';
}
}
return processed_names;
}
};
So you can do things like:
ValueID_Index_ThermostatOperatingState variable = ValueID_Index_ThermostatOperatingState::OperatingState;
std::cout << variable._to_string() << std::endl;
or to get all the Enums and their values:
size_t count = ValueID_Index_ThermostatOperatingState._count();
char *names[count] = ValueID_Index_ThermostatOperatingState._names();
uint32_t values[count] = ValueID_Index_ThermostatOperatingState._values();
for (size_t i = 0; i < count; i++) {
add_to_python(names[i], values[i]);
}
As for your other comments - thats why its in Dev Branch. Its WIP.
I plan to add something like a array to map the ValueID "ENUMS" to CommandClasses. Something like (via MACRO's again):
enumstruct = GET_CC_VID_ENUM(ThermostatOperatingState::GetStaticCommandClassId());
index = enumstruct::OperatingState
/* index now = 0 (OperatingState), so you can pass it to whatever */
Does that help?
that does help. I can see it is in the dev branch. Just looking for information on what the plans are when it is fully implemented.
I do know that nothing is etched in stone either. and plans change. I decided that for ease of readability to make the enums on the Python side and do a very similar setup with the checking of the command class in a value and have it hand back the correct enumeration.
python_openzwave has been set up in a manner that exposes methods to devices the methods would actually not work on. because of this we have to do quite a bit of additional checking to ensure that the node is of the correct command class.
# first check to make sure the node has COMMAND_CLASS_THERMOSTAT_SETPOINT
# if not return False indicating the node does not have the command class
if 0x43 not in self.command_classes:
return False
# iterate over all of the values the node contains.
for value in self.values.values():
# second check. see if the value is from COMMAND_CLASS_THERMOSTAT_SETPOINT
# if not continue the iteration loop
if value.command_class != 0x43:
continue
if value.index = value.indices.cooling:
value.data = new_setpoint
# return True indicating the new data was put into place
return True
# return false indicating that the index was not located
return False
This is the easiest solution I could come up with. what are you thoughts?
also the issue with any compiled code is the use of variable names. as you know they are only for our benefit the compiler mangles them. so I am not going to be able to access an enumeration by "name" even if it is assigned to the correct value. Not unless I make a replica of the code you have above in Cython. (which is essentially what I have done in Python).
In Python you have some magic methods where string constants can be used access any object in python does not matter if the object is a child of a module, a class a function. I have never read up on the python compiler. I am assuming because of the ability to access the object by string constant that the compiler does not change the names. Where as in C, CPP code it does. I am not sure about C# because it is more of an object orientated language.
The only way to allow this to work properly without the need to duplicate the code in Python is you would need to create an array and store the string constant of the variable name along with the enumeration value. and through the use of some voodoo magic code in Cython we can have a lookup performed. There are a slew of reasons not to go about it this way. First, the resource use. Second, the voodoo code that causes IDE's to not be able to follow the data path. Third, maintainability. etc....
I know that making a second set of enumerations creates 2 spots to have to update code when maintaining the indexes. But it is not that difficult to add them. there is going to be some additional memory use because of the second set, the use is negligible.
In the example above the lookup of the index set that belongs to the value is only going to be done a single time. when it gets used the first time. then it will cache the indices so it will not have to be looked up again. I can have it do it when the value is initially added. But this is going to slow down the startup and it will cache indexes that may not ever get used. I thought it best for the reasons above to do it the first time it gets accessed. One of the complaints I have seen from users is the startup time being slow. We do not want to add any additional work and slow it down even more. the small impact from the initial lookup done on a per use basis would not be noticeable, but 100's of them at once would make a very noticeable impact on the startup.
These next few lines are directed at people that make a comment that adds no value to the conversation. These are simply suggestions. It is not in any way something that is going to happen. or will happen. If there is a better solution then say there is and post your ideas about it. saying "It's a bad idea". or "No you can't do that" and nothing else is not constructive and in no way adds to the conversation. Also please read the WHOLE post not bits and pieces just skimming over it.
I will make any changes that need to be made in whatever manner that is deemed. If i did not supply the information below I would not be helping to make things better and that is the sole purpose of updating. I hope that everyone can understand that and appreciate that, because that is the only purpose to this post. It's information that should be used (or not be) to make a decision on what should be changed and how it should be changed. It does not take that long to put together a simple outline/plan for the changes that need to be made. I am pretty sure that everyone can agree that slapping in modifications to just "make it work" is not the ideal thing to do.
The more I dive into making any changes to the library the more I am really looking at how the code operates. I am also seeing ways that the performance of the library can be improved upon. Sorry for the book below. it outlines my concerns with the current code and there is also some of my thoughts and opinions on how we should proceed from here. These are simply my thoughts nothing more. objections are good. but you need to supply a really thorough reason as to why. not "I don't like it" Any code changes that are suggested should only need to be done in pyhon_openzwave and not in open_zwave. I provide examples of what would need to be done in openzwave if a specific code setup is done in this library. They are only there for purposes of example not something that should or will be done. They shouldn't be done I am providing the reason why they shouldn't hence why you see them in this post. I am human I do make mistakes. I am a 100% self taught programmer, so my interpretation of how bits and pieces operate may not be correct. If not tell me that they are incorrect and explain to me the correct way.
From looking at the code and using the knowledge I have this is a summation of where issues are and suggestions on how to solve them as well as making the necessary modifications to make the library work properly with openzwave 1.6.
The largest stumbling block is that Python is slow. it is really crazy how slow it is.
This blog post has some good information on the speed. differences between c code and python
https://medium.com/practo-engineering/execute-python-code-at-the-speed-of-c-extending-python-93e081b53f04
currently python open_zwave stores nodes based on the node id and the value gets stored in the same manner so there is no way to perform a lookup using c code unless the value id or node id is used specifically. The code in the library makes no use of either of those directly. It performs an iteration each and every single time either a value or a node needs to be accessed. because the iteration is taking place in python on not in c code is is really slow. here is a code example of one being performed in c code and one being performed in python.
iteration over an array in python
class Value:
# constructor
def __init__(self, id):
# creating instance attribute
self.id = id
# an array that has key, value pairs. This holds the value instances
values = {
1: Value(1),
2: Value(2),
3: Value(3)
}
for value in value.values():
if value.id == 3:
print('found value')
iteration over array using the core of python which is written in c code
if 3 in values:
'print('found value')
this exact example is small scale so execution times are not going to be noticeable. with larger arrays the time to execute becomes more apparent.
I personally hate having to loop over an array time and time again try to locate a single entry. This is not very optimized code and is not good in terms of performance. Having to do this a single time is not that big of a deal. but having to do it over and over again is not the best idea.
solution # 1 If a search is performed I can cache the results so the lookup would not have to be performed again. this would increase the speed at the cost of higher memory use. The memory use would not be to much higher I think because it would only be storing memory pointers, there is no duplication of the actual object.
how the Z-Wave protocol makes fast look ups difficult. The Z-Wave protocol allows overlapping indexes based on command classes. so if you take for example a node that is a thermostat and uses these command classes
* thermostat operating state
* thermostat mode
* thermostat fan mode
* thermostat fan state
you will have 4 values that all share the same index number of 0. so an iteration over the values would be needed in order to check what command class the value belongs to and also to check the index.
solution # 2
class Value:
# constructor
def __init__(self, id):
# creating instance attribute
self.id = id
# an array that has key, value pairs. This holds the value instances
# in python the "key" can also be a tuple. in that tuple we can place
# the command class identifier as well as the specific index.
val = Value(1)
values = {
(COMMAND_CLASS_THERMOSTAT_MODE, 0): val,
(COMMAND_CLASS_THERMOSTAT_OPERATING_STATE, 0): val,
(COMMAND_CLASS_THERMOSTAT_FAN_STATE, 0): val,
(COMMAND_CLASS_THERMOSTAT_FAN_MODE, 0): val,
}
if (COMMAND_CLASS_THERMOSTAT_OPERATING_STATE, 0) in values:
print('found value')
This would be faster code. There would be a higher memory use, but the performance gain would outweigh it.
I am looking at this from the internal code in the library and not from the application standpoint. I do not know currently how software that implements this library accesses the values or the nodes. The preferred way may be to hold a reference to the node id and the value id and access them that way. or it may be to use the methods provided to get the object for them. I would need some information from the people using the library in order to know what the correct approach would be.
The old style array can still be left in place and the addition of an array based on what you see above can be added. so no API breaking changes would need to be made.
Now I can make everything work as the library stands. But with some minimal changes I can help the performance which is an issue that seems to come up quite a bit. It would be nice to kill 2 birds with 1 stone. I know there have been significant changes to openzwave to help with regards to this. But if the python side of it is not optimized as well I do not know if the true benefit of those changes is going to translate over to the user.
This is also another thing I am sure that every programmer out there has come across this happening. and does not really like it when it does. Having a plan can avoid it.
I really hate having to write code 2 times. as I am sure you do as well. So the choices are this:
- Quick and dirty: Write the code to just make it work.. easy to do. But would require a rewrite later on.
- A better way: Spend a small bit of time coming up with a plan or a path to improve the program, Write the new code to work in the existing code but it gets written in a manner that would allow for a change to be made later on without having to do a rewrite.
- Best but also going to take a while: Fix the problems. Add the new code.
I am not one to break API. I personally hate it when when it happens. I try my absolute hardest to not break API.
I need to be told what direction to go. I am sure that before the last set of changes to openzwave took place the developers has some kind of a conversation and came up with a plan to make the changes. No less should be done here as well. We do not want to glue, tape and band-aide code in to "just make it work" may as well nickname the library "Windows" at that point.
Some of the changes to make the library compliant with 1.6 is not a big deal. while others are more invasive. a plan has to be formulated in order to improve the library, that's what it is all about. making improvements, not making it just work even if it doesn't work better. without a plan we fall on our faces.
If there are changes to be made then why not also try to correct some issues at the same time. I am not saying to do a complete rewrite. Or to get rid of Cython. nothing like that. changes to data access and storage to help improve performance can and really should be done. since the methods need to have a once over to correct the use of the labels changing that storage to improve the performance at the same time I think would be a good idea and not terribly hard to do. Hearing from developers that are using python_openzwave is really important. The slow nature of Python demands the highest level in terms of code optimization in order to provide the users with a superior Z-Wave control system. That is all I want to see. I am sure that is what most also want to see.
If i didn't type this whole post up I would be doing the library and it's users an injustice. The changes above if run a single time would not show any kind of a substantial benefit. you have to remember how many times does a value get accessed??. I would say almost every single time the program wants an update of the device states. in some cases once every 10 seconds, other could be shorter or longer. But remember it has to do this for each and every single value on the whole network. This has nothing to do with openzwave or the zwave network it's self. This only has to do with how fast the library is able to locate the value object and nothing else. the same holds true for how nodes are stored as well.
You can also mark this as done. Since there was no input on this. I went ahead and changed up the storage for the values. The old storage is still in place so that end of the API is still good.
There are some changes to node specific methods. they are for the better I think.
There was some strange code for changing say the thermostat setpoint. I reworked those and they are now properties that access the value by index. i created properties for each of the indexes. I also created properties for things like dimmer level. and switch state. The old mechanism required passing a value id in order to get the values value and also to set it. Now that is no longer needed and the reason why I can use properties.
I am going to leave the original methods in place and they will just return the information from the properties.