Eventwatcher

[markrad]

This is a fairly large commit. I don't know if you have any unit tests for your code but I will warn you I have only tested the RPi components. I may have accidentally made breaking changes to other components.

This adds event detection to the RPi sensor and also adds the option to load and call a custom script when an event is detected. The real world usage scenario for this is I have a RPi that is in an area that sends a message to OpenHab via MQTT when it detects motion. OpenHab will turn on the lights if it is after dark. However, I wanted to see some local feedback so the custom script is invoked to turn off the red LED (no motion) and turn on a blue LED (motion detected) and vice-versa. I know I could have done this by sending a message back from OpenHab but having local feedback provides me with more useful information.

I also added an initial state option to the sensor so one could have it set to high or low at start up. I think you have a GitHub issue asking for that. I needed it too so I put it in since it's only a few lines of code.

My big concern is that in sensorReporter.py you call checkState followed by publishState. In the RPi sensor code checkState calls publishState so one ends up publishing twice. This caused me some problems so I modified sensorReporter.py to assume that checkState would call publishState. If this is not what you want I'm prepared to switch it back and make any other necessary changes to keep it consistent across sensors.

[rkoshak]

I've had the implementation of event handling on my mental roadmap for a long time now. Thanks for taking the initiative.

I don't know if you have any unit tests for your code

Sounds like a fantastic idea. It is probably obvious from my code quality that I basically used this project to learn me some Python. Had I written it in Java I'd have unit tests all over the place. I've not dug into unit tests in Python yet. :-(

It might take me a little extra time to run the changes through to make sure nothing is broken. I'm more concerned about an unpublished RFM69 extension I have that uses a GPL v2 library so I can't release it here and I'm not sure if I'd be allowed to release it as a separately licensed extension.

The changes appear to be just the addition of some arguments to __init which should be benign so I'm not too worried about the extensions. I'll have to test them though given the changes to the main file.

StateCallback

I am a little ambivalent on this. Clearly, I like to use plug-in type architectures where you can wire together a bunch of objects together through config files. And I've even had the idea of implementing something like this myself. But I worry about applying this approach too much and resulting in too much complexity for non-technical users to understand.

Does it make sense to implement in this way or would it make sense to create a copy of the rpiGPIOsensor and implement the StateCallback natively in the extension?

Clearly, I would probably have implemented it just like you did. When all you have is a hammer... But I just want to stop and think and talk about if this is the way we should go or if there is some better approach.

For example, we could add another Connection and update so extensions can support multiple Connection. Then the StateCallback would just be a second Communicator, it just so happens that the messages don't go anywhere and just turn on/off the right LEDs. Another approach can be to borrow from the ESH architecture and bring in an event bus (probably a little heavy-weight for this). I'm starting to wish I had built this a little more OO.

Thoughts?

I also added an initial state option to the sensor so one could have it set to high or low at start up. I think you have a GitHub issue asking for that.

Yes, it's #42

My big concern is that in sensorReporter.py you call checkState followed by publishState. In the RPi sensor code checkState calls publishState so one ends up publishing twice. This caused me some problems so I modified sensorReporter.py to assume that checkState would call publishState. If this is not what you want I'm prepared to switch it back and make any other necessary changes to keep it consistent across sensors.

I have to think back on that. I did it that way on purpose, I remember that much, but I can't remember why. I know I separated checkState and publishState so it would only publish when the state changes but I can't remember why I call both checkState and publishState from the main polling loop. After looking at the code, I think that may have been something I was doing as a debug and just never took it back out. I reviewed all the sensors and they all publish from checkState when the state is the same. So the publishState should not be there in sensorReporter.

I really need to deprecate and get rid of gpioSensor.py. WebIOPi has long since gone defunct, we don't need to carry this around any more.

The contents of on_message and on_direct_message should be merged into a single method which gets called from on_message and on_direct_message. DRY

Probably should make the logging level be a parameter in the ini file.

Beyond running some tests I see nothing that stands out beyond the DRY comment above. I'll open new issues for some of the other random stuff I saw above list deprecating gpioSensor, and adding the logging level to the ini file.

[rkoshak]

After some thought, do we even need on_direct_message? Do we need to or should we make on_message a little more generic and able to handle calls outside of MQTT messages.

[markrad]

How about on_message simply calls on_direct_message? The other two parameters are not used.

[rkoshak]

That is reasonable. It is possible to just have the StateCallback just call on_message and do away with the need for two similarly named methods that essentially need to do the same thing? Or perhaps change the name of on_direct_message to something like activate or execute.

My main concern is one of confusion. The name on_direct_message is not very clear what makes it different from on_message. It's not a big deal, just something that came to mind.

[markrad]

I was thinking about your comment above about making the LEDs just another connection class. Initially I was not enamored with the idea but the more I thought about it the more I came to realize that one is not likely to do much more than flip an LED. Yes my approach gives one ultimate flexibility but is anybody actually going to use it for anything other than for what I did? I'm going to throw together a test using your idea and see if it makes sense. I'm still tempted to argue to keep the script loader in there too though for those that really want that flexibility. I have a feeling that if I take it out I'll probably find a need for it. But a connection class would be a more approachable solution for the non-technical and ultimately it's your project so I'll abide by whatever you think is best.

I'll try and play with the idea tomorrow and, if I get it working satisfactorily, I'll send you a link to a branch in my fork so you can take a look at it before I send you a PR.

My main concern is one of confusion. The name on_direct_message is not very clear what makes it different from on_message. It's not a big deal, just something that came to mind.

I can do that.

[markrad]

Here is the problem. The way the code works now a sensor is passed the connection on which to publish. In order to use an LED as a connection you end up needing to publish to two connections which the current design does not accommodate. Perhaps an array of connections?

[rkoshak]

Yes my approach gives one ultimate flexibility but is anybody actually going to use it for anything other than for what I did?

I can't answer that. Having a local callback like that is not something that ever came up for me so I can't really say. I like the idea a lot but I'm cautious.

Perhaps an array of connections?

That is exactly what I was thinking. We definitely need to be able to support multiple connections to support implementing this as a connection and I've always had a plan of supporting something like that at some point. I just wish that I had built the extensions a little more OO. Then some of the code like processing the handling of the array of Connections could have been centralized in a parent class. Some day...

[markrad]

I've been experimenting but I don't have the code ready yet. I've modified the first parameter to be an array and publishstate does a for conn in connections: publish(...) Yes a base class that implemented the publishState function would have been nice. Maybe v2.

[markrad]

I pushed my latest changes. I added a connection class for GPIO pins on RPi and I converted the connections to an array so that sensors could write to multiple connections. I haven't tested it too much yet but it works for my simple LED flip. It was more code than I expected. I also couldn't stand the lack of tab/space consistency in the DHT22 module so it shows as having massive changes when I converted it all to spaces. One nitpick, I would contend that the execActuator should be a sensor not an actuator. It doesn't really set things, it returns state from a command. Anyway, have a look and tell me what you think. I still left in the custom code loader for now. By the way, it's late and I may have missed stuff. M

[rkoshak]

One nitpick, I would contend that the execActuator should be a sensor not an actuator. It doesn't really set things, it returns state from a command.

The rubric I used as the distinction between a sensor and an actuator is whether it is a push or you have to tell it to run.

The rpiGPIOSensor pushes data independently so it is a sensor. It has a polling period and when it detects a change it pushes the new value. While Dash doesn't have a polling period, it operates independently and pushes sensor readings when detected.

The execActuator only does something in response to a direct request. You must command it to run, i.e. you must actuate it, so by the rubric above it is an actuator.

I can certainly see the addition of an execSensor that executes the script based on a polling period like the GPIO sensor and pushing the results. In fact, one of the ways I'm using the execSensor would probably be most appropriately implemented that way (note to self).

What the script actually does is not defined but when I wrote it I anticipated the scripts being executed to actually change something. My first use of it was to interact with the Roku API to cause my Roku to go home after everyone goes to bed. The return was primarily used as a confirmation the script ran without error.

I'll create an issue to create an execSensor that will work with a polling period, or modify the execActuator to make it work both ways, in which case, by the above rubric, it would become a sensor.

I also couldn't stand the lack of tab/space consistency in the DHT22 module so it shows as having massive changes when I converted it all to spaces.

No problem, the review page makes it pretty clear what the changes are. This needed to be done anyway. I don't think Python 3 allows a mix of tabs and spaces anyway. I assume you have a DHT22 set up and can test it for me as I do not and will not be able to test that extension independently.

The code looks good to me so far. Once you confirm the multiple connections work and update the other extensions to use them I'd say this one is done.

[markrad]

I do have a DHT22 and I have tested the code successfully. I need to do a little more testing before I declare it ready and probably some code clean up. Thanks for the feedback. I'll try and work on it some more later this week.

[markrad]

I've tested the multiple connections. My motion sensor sends 'OPEN' and 'CLOSE' to its MQTT connection and the RPi GPIO connection. OpenHab responds to the motion and puts on the lights and the LED goes from red to blue. Proves beyond any doubt that it works (famous last words). I fixed up the other extensions but I can't/haven't tested them. Do you have something set up to smoke test the others? I've tested all the RPi classes, sensor, actuator and the connection, the MQTT connector, and the DHT22 class.

[rkoshak]

Fantastic. I'll try to test the other extensions later today or sometime tomorrow. I'm actively using exec and can easily bring back up the Bluetooth and Dash configs in my environment. Shouldn't take too long.

It looks great! Thanks for the contributions!

[rkoshak]

I did some preliminary testing over the weekend and everything seem to work, at least the parts I'm actively using are working. If you are comfortable with the changes right now I'll merge them. If not, let me know when you are ready.

[markrad]

I've been running the code since I sent you the PR without any issues. I'm ready when you are.