Discover Z-Wave values by index#7853
Conversation
|
@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. |
| DISC_WRITEONLY = "writeonly" | ||
|
|
||
| METER_RESET_INDEX = 33 | ||
| INDEX_ALARM_TYPE = 0 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| DISC_WRITEONLY = "writeonly" | ||
|
|
||
| METER_RESET_INDEX = 33 | ||
| # https://github.com/OpenZWave/open-zwave/blob |
There was a problem hiding this comment.
I'm totally fine with making an exception for urls. Use noqa to disable flake8
There was a problem hiding this comment.
Probably should have thought of that. 😃
| DISC_WRITEONLY = "writeonly" | ||
|
|
||
| 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 |
There was a problem hiding this comment.
Can you move the pylint and flake8 directives to the line above? That way it's easy for people to copy paste the line.
There was a problem hiding this comment.
Looks like that also made pylint respect #noqa. All around win! 🎉
6 participants
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: