home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
70.46k stars 29.4k forks source link

unfork python-openzwave so we can get access to new devices easily #817

Closed partofthething closed 8 years ago

partofthething commented 8 years ago

In #20, support was added for z-wave devices using the python-openzwave module, and it is glorious. However, after spending all day on small issues, I've realized that the fork of python-openzwave referred to in the home-assistant z-wave instructions is way behind its parent, and the folks over at OpenZWave have dutifully been adding new devices, including the new Multisensor 6 and the Gen5 Z-Stick.

Solutions:

  1. Merge OpenZWave/python-openzwave into balloob/python-openzwave now, and more frequently.
  2. Help me understand why the fork was needed in the first place so I can try to eliminate the need for it.

I prefer 2 so this issue goes away forever. What was the cython error that led to that fork? Is there a good reason it can't be pulled into OpenZWave/python-openzwave?

Landrash commented 8 years ago

In general I think I've read it was because of compile issues between python, openzwave and python-openzwave.

https://github.com/balloob/home-assistant/issues/382 has some of it but I'm sure I've read more. I to would love to se the Z-wave device support be enhanced.

joshughes commented 8 years ago

I was able to get master of python-openzwave to compile on python3 but found this issue...

https://github.com/OpenZWave/python-openzwave/issues/28

I used the following instructions to compile on my local machine...

https://github.com/OpenZWave/python-openzwave/blob/master/INSTALL_ARCH.txt

joshughes commented 8 years ago

I forked openzwave and got it to compile locally for python3 and work with homeassistant...

First make sure you uninstall all of python zwave. there is an uninstall or uninstall.sh in the directory that is checked out when you install following the docs.

  1. git clone https://github.com/joshughes/python-openzwave
  2. pip install Cython==0.22 ensure it is this version... not sure if it matters at this point but it is what i have on my machine
  3. cd python-openzwave
  4. make PYTHON_EXEC=/home/homeassistant/.pyenv/shims/python install

Ensure PYTHON_EXEC is the path to which python for your environment. I am still trying to get support going for my zwave smoke detectors. So at this point I at least have the latest open zwave. Though this might be useful to others experimenting with this project.

partofthething commented 8 years ago

Did you have to change any code in your fork? If not then we can update the build instructions and eliminate the need for the fork.

Landrash commented 8 years ago

@joshughes You mention python3, to be more precise, Which version are you using? 3.4 or 3.5?

danichispa commented 8 years ago

@joshughes Trying to follow your instructions, getting this error

Building OpenZWave Version 1.3-614-gadf3467-dirty
Building tinystr.o
Building tinyxmlerror.o
Building tinyxml.o
Building tinyxmlparser.o
Building hid.o
/home/pi/python-openzwave/openzwave/cpp/hidapi/linux/hid.c:44:21: fatal error: libudev.h: No such file or directory
 #include <libudev.h>
                     ^
compilation terminated.
/home/pi/python-openzwave/openzwave/cpp/build/support.mk:124: recipe for target '/home/pi/python-openzwave/openzwave/.lib/hid.o' failed
make[2]: *** [/home/pi/python-openzwave/openzwave/.lib/hid.o] Error 1
make[2]: Leaving directory '/home/pi/python-openzwave/openzwave/cpp/build'
Makefile:20: recipe for target 'all' failed
make[1]: *** [all] Error 2
make[1]: Leaving directory '/home/pi/python-openzwave/openzwave'
Makefile:266: recipe for target 'openzwave/.lib/' failed
make: *** [openzwave/.lib/] Error 2
joshughes commented 8 years ago

looks like your missing some compile dependencies... you can run sudo make PYTHON_EXEC=/home/homeassistant/.pyenv/shims/python common-deps sudo make PYTHON_EXEC=/home/homeassistant/.pyenv/shims/python pip-deps

partofthething commented 8 years ago

I just noticed that @balloob originally forked nechry/python-openzwave which was last updated in March but the ongoing development has been happening on OpenZwave/python-openzwave.

I'll try to build and send a PR updating the instructions if I can. That unicode issue in Python 3.5 seems to be the major impediment, but 3.4 should work fine I guess with the OpenZWave/python-openzwave python3 branch.

danichispa commented 8 years ago

Just installed installing the missing common-deps, had to insert the "make" after sudo ;) Now I'm using a Aeotec Z-Stick gen5 with HA :D

joshughes commented 8 years ago

Nice thanks for the correction! glad that helped you out!

danichispa commented 8 years ago

Thanks to you for the fork. In my case I had the python path set at /usr/bin/python3.4 (from memory, not in front of my Pi right now) Just for information to other people.

balloob commented 8 years ago

The fork was needed initially because the project did not support Python 3. I submitted patches for Python 3 support but they were never accepted. It would be awesome if we can start using the official version again.

joshughes commented 8 years ago

It seems like they are very close to python3 support. All I changed was string encoding with find and replace. My guess is they have a cPython configuration working against them... or a missing helper function.

I know very little about cpython, and just scraped enough together to be able to build master.

balloob commented 8 years ago

If you look at my fork, this was my initial commit to make Python 3 work: https://github.com/balloob/python-openzwave/commit/802ee416ea4068ed03c4cf686f95777399db8e90

Which is basically the same as you did now I guess :+1:

partofthething commented 8 years ago

Great! So those string conversions will get it to work as long as Cython <0.23 is used. Looks like @joshughes submitted his branch as a PR to python-openzwave, hopefully it will work for them.

Meanwhile, I'm trying to track down the issue with newer Cython over at the python-openzwave Issue 10

joshughes commented 8 years ago

My PR is not passing CI but I think I may be able to get it to pass... if anyone wants to take a look at the build results and give me some guidance that will be helpful. I am a python novice for sure.

balloob commented 8 years ago

The reason your PR is failing CI is because of the way strings work between Python 2 and 3. Calling str.encode is a fix that works for Python 3 but not for Python 2. If cython converted the strings correctly we shouldn't have to call any encoding methods.

partofthething commented 8 years ago

I think Cython 0.23 eliminates the need to have those string conversions in python-openzwave. I figured out how to get it to compile with Cython 0.23.3 and it compiles fine with Python 3.4 without the changes made by @balloob and @joshughes. I think some of the changes listed for Cython 0.23 dealt with the string conversion issue.

So, if we can get my simple change pulled in to python-openzwave, then we can update the zwave build instructions and close this. Cool.

partofthething commented 8 years ago

I was wrong above. Both changes are needed. Conveniently, python-openzwave has pulled these in to the master branch and it's compiling now. Thus, I believe this issue is nearly fixed.

partofthething commented 8 years ago

Uh oh, there are more problems. I just tried building today with all the latest of everything and started getting a bunch of TypeErrors about b'something' strings being not JSON serializable. The web page would not load at all. I reverted python-openzwave to the branch where it worked a few days ago and it still gave these errors.

Then I looked deeper and found this recent commit to openzwave (the actual library, not the python bindings). They changed a bunch of strings to uint16, which might make Cython return b'byte arrays' instead of 'regular strings'. I'm not sure though.

Anyway to unfork, we now have to update home-assistant to be convert the byte arrays to strings before JSONing them.

Traceback (most recent call last):
  File "/usr/lib/python3.4/socketserver.py", line 613, in process_request_thread
    self.finish_request(request, client_address)
  File "/usr/lib/python3.4/socketserver.py", line 344, in finish_request
    self.RequestHandlerClass(request, client_address, self)
  File "/usr/local/lib/python3.4/dist-packages/homeassistant/components/http.py", line 166, in __init__
    SimpleHTTPRequestHandler.__init__(self, req, client_addr, server)
  File "/usr/lib/python3.4/socketserver.py", line 669, in __init__
    self.handle()
  File "/usr/lib/python3.4/http/server.py", line 398, in handle
    self.handle_one_request()
  File "/usr/lib/python3.4/http/server.py", line 386, in handle_one_request
    method()
  File "/usr/local/lib/python3.4/dist-packages/homeassistant/components/http.py", line 261, in do_GET
    self._handle_request('GET')
  File "/usr/local/lib/python3.4/dist-packages/homeassistant/components/http.py", line 245, in _handle_request
    handle_request_method(self, path_match, data)
  File "/usr/local/lib/python3.4/dist-packages/homeassistant/components/api.py", line 191, in _handle_get_api_bootstrap
    'services': _services_json(hass),
  File "/usr/local/lib/python3.4/dist-packages/homeassistant/components/http.py", line 294, in write_json
    cls=rem.JSONEncoder).encode("UTF-8"))
  File "/usr/lib/python3.4/json/__init__.py", line 237, in dumps
    **kw).encode(obj)
  File "/usr/lib/python3.4/json/encoder.py", line 194, in encode
    chunks = list(chunks)
  File "/usr/lib/python3.4/json/encoder.py", line 422, in _iterencode
    yield from _iterencode_dict(o, _current_indent_level)
  File "/usr/lib/python3.4/json/encoder.py", line 396, in _iterencode_dict
    yield from chunks
  File "/usr/lib/python3.4/json/encoder.py", line 317, in _iterencode_list
    yield from chunks
  File "/usr/lib/python3.4/json/encoder.py", line 430, in _iterencode
    yield from _iterencode(o, _current_indent_level)
  File "/usr/lib/python3.4/json/encoder.py", line 422, in _iterencode
    yield from _iterencode_dict(o, _current_indent_level)
  File "/usr/lib/python3.4/json/encoder.py", line 396, in _iterencode_dict
    yield from chunks
  File "/usr/lib/python3.4/json/encoder.py", line 396, in _iterencode_dict
    yield from chunks
  File "/usr/lib/python3.4/json/encoder.py", line 429, in _iterencode
    o = _default(o)
  File "/usr/local/lib/python3.4/dist-packages/homeassistant/remote.py", line 284, in default
    return json.JSONEncoder.default(self, obj)
  File "/usr/lib/python3.4/json/encoder.py", line 173, in default
    raise TypeError(repr(o) + " is not JSON serializable")
TypeError: b'%' is not JSON serializable
balloob commented 8 years ago

We could have the zwave component/platforms convert it but I feel like we should be able to get this fixed within python-openzwave. I do not get however how they can convert strings to uint16 since one is a string, the other a number.

partofthething commented 8 years ago

Ok I think I've isolated the problem through bisection. It is introduced between this commit (works, gives strings) and this commit, where the previous commit was meant to be fixed for Python 2. This second commit does not work and returns bytes types for basically everything where as the first commit returns strings. This is the crux of the problem. I'm not sure how to fix it yet in a way that doesn't break python2.

balloob commented 8 years ago

Probabt the method cstr_to_str? although it looks fine to me.

On Thu, Jan 14, 2016, 00:22 Nick Touran notifications@github.com wrote:

Ok I think I've isolated the problem through bisection. It is introduced between this commit https://github.com/partofthething/python-openzwave/commit/5e4d68c03ebda965c16a627ba36de635528d026b (works, gives strings) and this commit, where the previous commit was meant to be fixed for Python 2 https://github.com/OpenZWave/python-openzwave/commit/d14f1a479c1f87875c67e65d046983692c4e307b. This second commit does not work and returns bytes types for basically everything where as the first commit returns strings. This is the crux of the problem. I'm not sure how to fix it yet in a way that doesn't break python2.

— Reply to this email directly or view it on GitHub https://github.com/balloob/home-assistant/issues/817#issuecomment-171569704 .

partofthething commented 8 years ago

I figured it out. Should be good now.

joshughes commented 8 years ago

Any idea why the build is failing?

partofthething commented 8 years ago

Travis log says

Using /usr/local/lib/python2.7/dist-packages
Finished processing dependencies for pyozwweb==0.3.0b8
Error in atexit._run_exitfuncs:
Traceback (most recent call last):
  File "/usr/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
    func(*targs, **kargs)
  File "/usr/lib/python2.7/multiprocessing/util.py", line 284, in _exit_function
    info('process shutting down')
TypeError: 'NoneType' object is not callable
Error in sys.exitfunc:
Traceback (most recent call last):
  File "/usr/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
    func(*targs, **kargs)
  File "/usr/lib/python2.7/multiprocessing/util.py", line 284, in _exit_function
    info('process shutting down')
TypeError: 'NoneType' object is not callable

which on searching appears to be a closed bug in Python itself that shows up when nose is used for testing. Notably, all the other recent builds have failed as well. I think it's just a problem with the CI setup. It's running on Ubuntu 12.04, which comes with Python 2.7.3. The aforementioned bug was closed between 2.7.3 and 2.7.4 for I think the solution here would be migrating the CI server from Precise to Trusty, as it appears Travis CI is already working on.

Once my PR is merged into python-openzwave, I will test a clean build. If it works (as I'm SURE it will ;), then I will update the instructions on home-assistant.io and close this.

Landrash commented 8 years ago

@partofthething bump Seems like it has been merged now. If you need another to test it then I volunteer. =)

luxus commented 8 years ago

i can test too if i got some basic instructions :D

partofthething commented 8 years ago

I saw that my PR got merged over there so yeah it should be all good to go. I will update the home-assistant instructions for zwave and then you guys can try them out and tell me if it works.

partofthething commented 8 years ago

Ok @luxus and @landrash, new instructions have been created in my branch. Try them out and let me know what happens.

luxus commented 8 years ago

shouldn't it be python3-sphinx ? python-sphinx will install python2

partofthething commented 8 years ago

Don't think so. What's that? I only see two branches at https://github.com/OpenZWave/python-openzwave/branches, master and python3.

luxus commented 8 years ago

sorry for the misunderstanding, i think you don't even alter this line.. apt-get install cython3 libudev-dev python-sphinx python3-setuptools will install python2 because of the python-sphinx

partofthething commented 8 years ago

Ah yes, good call. I didn't even see that in there. Will update now.

balloob commented 8 years ago

Could you also update the build script? https://github.com/balloob/home-assistant/blob/dev/script/build_python_openzwave

On Sun, Feb 28, 2016 at 11:22 AM, Nick Touran notifications@github.com wrote:

Ok @luxus https://github.com/luxus and @landrash https://github.com/landrash, new instructions have been created in my branch https://github.com/partofthething/home-assistant.io/blob/87dd6016a6852e0f460eb3229fb7d7e7a80983eb/source/_components/zwave.markdown. Try them out and let me know what happens.

— Reply to this email directly or view it on GitHub https://github.com/balloob/home-assistant/issues/817#issuecomment-189926799 .

PaulusSchoutsen.nl It's nice to be important but it's more important to be nice.

partofthething commented 8 years ago

@luxus, I had erroneous $s in my script. Updated in this link (which will update as I make additional changes).

@balloob, I updated it here.

partofthething commented 8 years ago

@luxus got it working with my updates so I will make a PR.

Landrash commented 8 years ago

@partofthething Tested and seems to work fine. Instructions should probably specify which distribution they are for and in some cases also for which release.

Would package requirements for Arch Linux be a interesting addition? Considering making a PKGBUILD script for Arch that would simplify the installation.

adamdobson commented 8 years ago

I just gave this a test with the intructions

pointed my config_path at the new location. But unfortunately got a error on startup:

ERROR:homeassistant.bootstrap:Error during setup of component zwave
Traceback (most recent call last):
  File "/usr/local/lib/python3.4/dist-packages/homeassistant/bootstrap.py", line 106, in _setup_component
    if not component.setup(hass, config):
  File "/usr/local/lib/python3.4/dist-packages/homeassistant/components/zwave.py", line 175, in setup
    DEFAULT_ZWAVE_CONFIG_PATH),)
  File "/usr/local/lib/python3.4/dist-packages/python_openzwave_api-0.2.6-py3.4.egg/openzwave/option.py", line 90, in __init__
    self._cmd_line.encode("UTF-8"))
TypeError: Argument 'a' has incorrect type (expected str, got bytes)
kmadac commented 8 years ago

I just experienced same error. My problem was, that I had still installed old vesion of python-openzwave. I uninstalled it manually:

pip3 uninstall python-openzwave-api
pip3 uninstall python-openzwave-lib

and then I did make install again

PYTHON_EXEC=`which python3` make install
fabaff commented 8 years ago

The unforking was done with 0.15.

Documentation: