genielabs / HomeGenie

HomeGenie, the programmable automation intelligence
https://homegenie.it
GNU General Public License v3.0
393 stars 155 forks source link

Allow node to have 1 to many device handler relationship instead of 1 to 1. #118

Closed hybridview closed 9 years ago

hybridview commented 9 years ago

This is more of a developer "feature" request to allow clean and full support of all generic devices.

I'm developing a new device handler to support meter message parsing for my AEON Labs HEM (g1) device. It has been difficult to create clean code because my HEM supports both METER and MULTILEVEL device classes. For example, If I have HEM inherit from Meter, I can get the proper meter message parsing support, but I have to duplicate code from Sensor.cs to support MULTILEVEL messages. I can just implement everything in my HEM handler class, but you can see that this will end up making the ZWave lib messy as more device support is added. The generic types will end up supporting each others command classes and more and more duplicate code will result.

MY REQUEST: At some point, it would be great to enable a NODE to support 1 or more device handlers. Each handler in the list checks if it can support the message class. First one to support the message handles it.

EXAMPLE: My HEM Node has 2 device handlers assigned. 1 for Meter and 1 for Sensor. A METER report comes in. The node calls the handler in for Sensor first. Sensor.cs does NOT support the METER command class, so it returns false. Next, the node calls the handler for Meter. Meter.cs DOES support the METER command class, so it parses the message and returns TRUE.

The METER generic is a great example for this request. Unlike multilevel, METER messages include 3 values in a single message frame: for "Meter Value", "Delta Time" (time between current value and previous), and "Previous Value".

genemars commented 9 years ago

You can make the Meter.cs inherit from Sensor.cs as a temporary work-around. Shall this work? I'm still stuck on finding a good solution for implementing 1to N relationship between device handlers. Any suggestion is welcome =)

hybridview commented 9 years ago

Your current device handling setup works very well with generic widgets. Implementing 1-N without over-complicating multiple areas of the ZWavLib is tricky indeed. :) I'll use your workaround for now and keep putting some thought into the 1-N subject. Thanks for the suggestion and quick response! This project has become a great side-hobby for me due to how actively it is developed.

hybridview commented 9 years ago

Gene, I found a great workaround for this issue that will have no impact on existing devices. I created a new generic device called "MeterSensorCombo" that inherits IZWaveDeviceHandler. Inside this class, I have 2 properties: MeterHandler and SensorHandler that inherit from Meter and Sensor. In the virtual "Handle" methods, I simply try handling with Meter, then Sensor if Meter cannot handle. Result is that both Meter and Sensor can be supported by 1 device. Not perfect, but it resolves much of the problem without major refactoring. I made a specific Aeon HEM device that inherits from this combo class and it works great.

I also fixed some parsing issues involving Multi-Instance reporting that caused some users to get incorrect "Binary" and "Generic" results along with the "Unhandled Message" error. I commited all of this to my forked instance under the branch HEM-Meter. Since I already have a Pull request pending, I'm afraid to submit another one so soon. I'm just starting to learn GIT. :)

All of these changes have been tested and confirmed on my local instance. Changes are heavily commented to reduce the risk of surprise issues. Your Statistics enhancements along with these changes have greatly improved the "wow factor" of this instance running on my home server. Many thanks for your hard work!

genemars commented 9 years ago

@hybridview I'm merging in few minutes, so you can create another pull request. At some point ZWaveLib will have a major refactoring that will deeply change the way commandclasses and device types are handled. Any suggestion about this is welcome :) In the meantime compatible and low impact enhancement are welcome. Do you have any thermostat device? Just pushed some new code and fixes for that.

hybridview commented 9 years ago

I've been doing a lot of research on ZWAVE and am starting to think that this 1 to many idea may not be the best final solution (but for short term, it requires no refactoring and should work great). Many of the device types have common commands, so parsing messages based solely on device type might not be the best way. For long term, we may want to handle messages based on command class instead. This will likely require some significant work, so I'm going to create a separate branch for "concept" work (mainly as a place to put ideas into). Since HG works so well using a much simpler model of ZWAVE (ie. not needing a FULL implementation of all zwave funcitonality), it might be a good idea to keep a future command class design as simple as possible (at least in the beginning). Definitely an interesting subject for future discussion.

genemars commented 9 years ago

Hi @hybridview, I'm thinking too of a solution right now. You're right. The best is to deprecate device handlers and create a static class for each z-wave command class. This way we will just modify ZWaveNode.cs MessageRequestHandler function to use the proper command class routine instead of calling "handlers" for different device types. At the very beginning z-wave protocol was not clear at all to me and I did proceed in an hackish way. Now I've got some more knowledge about how the protocol is structured and also getting help from you contributors, so it may be the good time for a big refactoring.

hybridview commented 9 years ago

Very good plan Gene! I had the same problem you did regarding lack of ZWAVE knowledge. Once I finally found good documentation, I just couldn't stop learning about it. Many of my previous assumptions about what various bytes represented were completely wrong. I even started a Microsoft OneNote notebook on my workstation to keep track of all of this great info. I only have a HEM device at the moment, but I plan on getting thermostats and switches soon. If I didn't have 2 little kids, I would be spending all of my free time playing with this stuff.

genemars commented 9 years ago

after the zwavelib rework this can now be closed.