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.

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

Discover Z-Wave values by index #7853

Merged
merged 4 commits into from Jun 14, 2017

Conversation

Projects
None yet
6 participants
@armills
Copy link
Contributor

commented Jun 1, 2017

Description:

This PR adjusts the discovery schemas to migrate away from using the label as a classifier. Openzwave provides the index for this purpose, so it makes sense to use it. It also allows us to clean up some of the other classifiers that could use improvement. See also #7780 (comment)

While these indices come from the openzwave source, it would be best if we could get some further testing from devs to ensure that devices are still discovered correctly. The following schemas should be tested:

  • Node power
  • Climate
  • Light
  • Lock
@mention-bot

This comment has been minimized.

Copy link

commented Jun 1, 2017

@armills, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @andrey-git and @turbokongen to be potential reviewers.


METER_RESET_INDEX = 33
INDEX_ALARM_TYPE = 0

This comment has been minimized.

Copy link
@andrey-git

andrey-git Jun 6, 2017

Contributor

Could you add a comment with a link to OZW source where those are defined?
This is very brittle. What if OZW change indexes? Aren't those internal implementation details?

This comment has been minimized.

Copy link
@armills

armills Jun 6, 2017

Author Contributor

I'll add links in a bit. I debated myself a bit on whether we should add links, since the links could become broken in the future, but I guess it's better than nothing at all.

As for the indexes, they are actually explicitly defined for this purpose by open-zwave. I couldn't find an answer in the documentation, but there is an answer in the thread linked below, and they are defined explicitly as enums in the source:

https://groups.google.com/d/msg/openzwave/tqxQr5XbG2Q/3rbUwORKB5UJ
https://github.com/OpenZWave/open-zwave/blob/master/cpp/src/command_classes/SensorMultilevel.cpp#L50

This comment has been minimized.

Copy link
@andrey-git

andrey-git Jun 6, 2017

Contributor

Thanks!
It would be great if we could directly use those instead of putting explicit numbers and maybe making a mistake, but I guess it is not possible.
I think a per-cpp-file line to each group of enums (to set version, not to head) would be helpful.

This comment has been minimized.

Copy link
@armills

armills Jun 6, 2017

Author Contributor

Yeah, I agree. If there is a way to directly import the enums let me know!

I'll add links like that. I like the idea of linking to the current version, to prevent a broken link.

This comment has been minimized.

Copy link
@armills

armills Jun 7, 2017

Author Contributor

So, I added the links, but unfortunately they have to be pretty heavily wrapped. It's pretty ugly but I think it's probably better to have the reference than not. It's not like people are going to be working in const.py all the time, so it's not a huge inconvenience.

@balloobbot balloobbot added the platform label Jun 7, 2017


METER_RESET_INDEX = 33
# https://github.com/OpenZWave/open-zwave/blob

This comment has been minimized.

Copy link
@balloob

balloob Jun 9, 2017

Member

I'm totally fine with making an exception for urls. Use noqa to disable flake8

This comment has been minimized.

Copy link
@armills

armills Jun 9, 2017

Author Contributor

Probably should have thought of that. 😃


METER_RESET_INDEX = 33
# https://github.com/OpenZWave/open-zwave/blob/67f180eb565f0054f517ff395c71ecd706f6a837/cpp/src/command_classes/Alarm.cpp#L49 # noqa # pylint:disable=line-too-long

This comment has been minimized.

Copy link
@balloob

balloob Jun 12, 2017

Member

Can you move the pylint and flake8 directives to the line above? That way it's easy for people to copy paste the line.

This comment has been minimized.

Copy link
@armills

armills Jun 13, 2017

Author Contributor

Looks like that also made pylint respect #noqa. All around win! 🎉

@armills armills merged commit ae39731 into home-assistant:dev Jun 14, 2017

4 checks passed

cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.002%) to 93.565%
Details
hound No violations found. Woof!

@armills armills deleted the armills:zwave-index-discovery branch Jun 14, 2017

@balloob balloob referenced this pull request Jun 16, 2017

Merged

0.47 #8055

@andrey-git andrey-git referenced this pull request Aug 13, 2017

Merged

Fix zwave power_consumption attribute #8968

2 of 2 tasks complete

@home-assistant home-assistant locked and limited conversation to collaborators Oct 20, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.