8000 Allow setting Z-Wave network key via options · Issue #7635 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Allow setting Z-Wave network key via options #7635

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.

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

Closed
balloob opened this issue May 17, 2017 · 23 comments · Fixed by #7637
Closed

Allow setting Z-Wave network key via options #7635

balloob opened this issue May 17, 2017 · 23 comments · Fixed by #7637
Milestone

Comments

@balloob
Copy link
Member
balloob commented May 17, 2017

We migrated Z-Wave to be based on pip. However, it looks like the network key used to be saved in the Python Z-Wave directory which is now part of the package. That means we'll lose the key on every upgrade causing people to have to pair their secure devices again.

To fix this, we need to add a config parameter to the Z-Wave component that we pass as the Open Z-Wave option NetworkKey to Python Open Z-Wave.

We pass options to Z-Wave network here.

@robbiet480 you were looking into this. Do you have a branch? If not, please note it here so someone else can pick this up.

This needs to be fixed before 0.45.

CC @armills @andrey-git @turbokongen

@balloob balloob added this to the 0.45 milestone May 17, 2017
@emlove
Copy link
Contributor
emlove commented May 17, 2017

Currently python-openzwave will read an options.xml file in the hass config directory, presumably from the user_path we set in the ZWaveOption object. Does this not work with the pip version?

We might just need to update the docs telling people to copy the file to their config directory before editing. This would be preferable since it allows anything else in options.xml to remain configurable.

@robbiet480
Copy link
Member

On a related note, someone submitted a PR to fix the PyOZW docs to note the network key can be passed in like I am doing here.

@andrey-git
Copy link
Contributor

There is code to copy options.xml from config path to user path if the file is missing from user path. I assume it is read from user path?

@balloob
Copy link
Member Author
balloob commented May 18, 2017

@armills is up to something. Maybe we should automatically copy options.xml to the Home Assistant config dir if it doesn't exist?

@robbiet480
Copy link
Member

options.xml needs to be inside the config dir

@balloob
Copy link
Member Author
balloob commented May 18, 2017

Okay, now I am confused. I was under the impression that somehow Python Open Z-Wave stored the key in the config files that are stored inside the Python package. Is this not the case ?

@robbiet480
Copy link
Member

PyOZW just clones the OZW config directory into its package directory. options.xml exists inside of that OZW config directory. Here it is.

@balloob
Copy link
Member Author
balloob commented May 18, 2017

I guess then we should do the same ? And use user_path to point PyOZW at that. We default now to the config dir btw but I think we should move that to a new z-wave folder.

@andrey-git
Copy link
Contributor

From reading ozw code I understand the following:

When locking options, ozwpy copies options.xml from config dir to user dir if it was missing in the user dir: https://github.com/OpenZWave/python-openzwave/blob/master/src-lib/libopenzwave/libopenzwave.pyx#L745 this was added April 26th

Then ozw cpp code first loads options from config dir, then user dir, then command line: https://github.com/OpenZWave/open-zwave/blob/master/cpp/src/Options.cpp#L371

Thus the only problem is for 8000 user without options.xml in user dir switching from version 0.3.x to 0.4.x

@Landrash
Copy link
Contributor

If possible wouldn't it be better if we could keep the key in the main HA configuration file?

@andrey-git
Copy link
Contributor

@Landrash It has advantages, but also has a downside of being unable to use secure devices with other programs, like ozwcp.

@emlove
Copy link
Contributor
emlove commented May 18, 2017

Agree with @andrey-git. Another downside to using configuration.yaml is that network key is only one of the options in options.xml, and it would be better to keep all of them accessible. options.xml was always accessible from the user path. That's how I've always used it since I run with docker.

Is it possible to add some transition logic that looks for an options.xml to copy in the old directory, if there is none in the user_path?

@balloob
Copy link
Member Author
balloob commented May 19, 2017

So if I get it correctly then, are we all good for this release and we don't need any changes?

(I'll be on PyCon from now till release so I need someone else to take the lead on this issue)

@balloob balloob mentioned this issue May 19, 2017
@andrey-git
Copy link
Contributor

We need to write in huge bold red letters that people should back up options.xml (once when switching to 0.45)

@emlove
Copy link
Contributor
emlove commented May 19, 2017

I don't like #7637 myself, as we're now giving users two different places to configure the network key. If we are going to require user action for the transition either way, I'd much more prefer that we just ask them to move options.xml into their config directory, where it really belonged in the first place.

It also sounds like going forward, python-openzwave is going to automatically copy a template there if one doesn't exist, so this will be the long-term expected practice.

@Landrash
Copy link
Contributor

Hopefully OZWCP wont be needed for the wast majority of users so the option in #7637 makes sense in a HA standalone way.
Having the user having to copy a file that may or may not be in a specific place is a bad work-around and one should always strive for the minimum amount of needed user interaction.

@emlove
Copy link
Contributor
emlove commented May 19, 2017

It's not a workaround though, it's the normal way zwave will work in the future. python-openzwave is already going to copy the file there automatically whether we like it or not, including a placeholder to set the network key. For a new install after 0.45, users will see options.xml right in their config directory next to zwcfg.xml, so it will be obvious where they should be editing it. No one should have been editing the file outside of the config directory to begin with.

Also, if we start pushing options.xml configuration parameters into the hass config, soon we'll end up with everything from options.xml in configuration.yaml. The network key is just the most currently visible option. I'm just of the opinion that duplicating upstream logic is just setting ourselves up for maintenance issues and breakages. We should be taking advantage of upstream code wherever possible.

And on a final note, users already are going to have to find that file that may or may not be in a specific place, because they're the ones who were editing it in that place. They'll need to copy the network key out either way, so they may as well just copy the file to where it belonged in the first place. Copying the file to the config directory is a much simpler request than asking them to extract the info from an xml file and add it to their configuration.yaml.

@Landrash
Copy link
Contributor

Ahh don't misunderstand me. I'm personally not a fan of of having external config files if they are not needed for the basic users since it only increases the support amount others will need to do.
If it possible to do both I don't see a problem with either.

@balloob
Copy link
Member Author
balloob commented May 20, 2017

@armills you're right, I like that more. Have them copy that file an we're all good. Let me wip up a quick PR.

@balloob
Copy link
Member Author
balloob commented May 20, 2017

Okay, don't have time to do a PR right now . Will leave the config option in for now.

We should also update the user path to be <config>/zwave. Should we copy the options.xml over when the user has none ? That would be the easiest. PR to fix this welcome 👍

@emlove
Copy link
Contributor
emlove commented May 23, 2017

I did some digging, since from the lines @andrey-git posted we would expect to see an options.xml file in the user_path we provide. It turns out there is an upstream bug that was preventing this from occurring. I've submitted a fix upstream. OpenZWave/python-openzwave#82

I think that after this is merged, and we can get the zwave user path in it's own subdirectory, it should be much clearer.

@andrey-git
Copy link
Contributor

@armills why does your fix help? In HA we set the user_path explicitly, so shouldn't the value in both ZWaveOption and libopenzwave.PyOptions be the same?

@emlove
Copy link
Contributor
emlove commented May 23, 2017

The user path was being reported correctly, but the calculated config_path was lost after this line executed. Once it was set as a python attribute, the cython variable became inaccessible. That means that this line was never able to execute successfully, even though it was properly loaded from the correct path. (This is at least how I interpreted the behavior I saw. I am by no means a Cython expert.)

HA was always able to load options.xml from the user_path, but python-openzwave didn't copy the template there, which caused a lot of confusion.

@home-assistant home-assistant locked and limited conversation to collaborators Sep 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants
0