Closed mattgam closed 5 years ago
Thanks for the contribution!
There is a lot there to look over.
First off, can you run black
over the code base? There is a lot that it could fix in terms of following PEP8. I really need to put linter config in this repository, so that is my fault. Black: https://github.com/ambv/black (Run with line length of 80)
Second, variable names should be snake case, that is words_separated_with_underscores.
Codacy mentioned some other things, like trailing white space, an unused import (that might have been in the code already?), and an unused variable.
For the comments per section, I would prefer pound sign comments, rather than a multiline strings.
I've commented a bit more in-line in the review.
Where did you get the documentation for the options you are using, by the way?
Darn I had a pep8 linter running in my Vim config (I thought). I haven't used black before but I'll be happy to do all the linter suggested cleanup. Timing wise it probably won't be till this weekend but i'll post updates for your review. I'm glad to hear that snakecase is good, I hated those long variable names.
On Tue, Sep 4, 2018 at 7:18 PM Tom Faulkner notifications@github.com wrote:
Thanks for the contribution!
There is a lot there to look over.
First off, can you run black over the code base? There is a lot that it could fix in terms of following PEP8. I really need to put linter config in this repository, so that is my fault. Black: https://github.com/ambv/black (Run with line length of 80)
Second, variable names should be snake case, that is words_separated_with_underscores.
Codacy mentioned some other things, like trailing white space, an unused import (that might have been in the code already?), and an unused variable.
For the comments per section, I would prefer pound sign comments, rather than a multiline strings.
I've commented a bit more in-line in the review.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/TomFaulkner/SenseMe/pull/40#issuecomment-418577427, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3cUt4M6FOFBRoPZGq2z8ymARHCnH4Vks5uXzRfgaJpZM4WYLf9 .
No rush, I'm going to be crazy busy this week anyway.
It's been awhile, I've been busy. Is this something you are still working on?
Tom, Apologies for going dark on you. Work and life got in the way in a major way with my wife unexpectedly needing major surgery. This is for sure in the plans to finish I'm probably 2-3 weeks out from having it done but I am going to do so. thank you for checking in, Matt
On Tue, Oct 2, 2018 at 2:51 PM Tom Faulkner notifications@github.com wrote:
It's been awhile, I've been busy. Is this something you are still working on?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/TomFaulkner/SenseMe/pull/40#issuecomment-426442991, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3cUgb8m4Bd9pnepZarO4JPxIPXnkLuks5ug9_vgaJpZM4WYLf9 .
No problem. No rush from me. I hope things are going well for your family.
Tom, I've gone through and used pycharm's PEP8 linter to clean up the code as well as updated the code with Black for formatting. To answer your earlier question "Where did you get the documentation for the options you are using, by the way?" My process was a bit brute force. I ran your get_all.py script to see all of the attributes of the devices. I then took the Haiku IOS app and would make a change in it and then look at get_all.py's output to see what changed. This showed me what each item was in the protocol and then it was just a lot of trial and error to see what the proper setter syntax was. I used the senseme_test.py example I built as a manual test harness to verify each item and then see how a change in a setting was reflected in the app.
On the TODO list is to wire up support for the Wall controllers. I have 3 in my house and would like to expose the motion sensors in it. Those will be valuable in later home automation integration work I'm planning. Once this PR is deemed satisfactory I'll attack that.
I reached out to big ass fan support about their standalone light module and learned they were discontinued so I won't be including support for those.
Let me know what else you'd like to see changed Matt
Nice process. It is nice to have so much documented, and soon to be functional! I've been busy, but I'll take a look at this in a day or so.
It's a shame to hear about their lights, I liked the idea, but the price seemed unreasonable.
I look forward to the wall controller code. Are these controls to control the fans, or something else? I've not seen them if they are something else.
The wall controller is a 1-gang mains powered unit that has a dimmer switch for the light, a second dimmer for the fan and a motion sensor. They can be added to a wifi and controlled by the app as well. One cool feature is the fan and wall controller can host their own wireless AP and then you can connect them together totally without the house having a wifi. The data sheet is https://www.haikuhome.com/media/pdf/HaikuHome_CutSheet_HaikuWallControl.pdf
They also make a wall controller for the industrial units (my garage dream fan) which I suspect uses the same protocol: https://www.bigassfans.com/docs/smartsense/cutsheet-smartsense.pdf
From looking at the tags I suspect we can just use the exact same calls as to the fan for this and it will mostly be adding additional docstrings to the API to call out which devices are supported and CLI arguments to expose it right to the tool. We could get fancy later and make the API calls check the cached device info and error out if the device doesn't support the commands.
The output from get_all.py on a wall controller is: (Media Room Wall Control;DEVICE;INDICATORS;ON) (Media Room Wall Control;DEVICE;SERVER;PRODUCTION) (Media Room Wall Control;ERRORLOG;ENTRIES;NUM;10) (Media Room Wall Control;ERRORLOG;ENTRIES;MAX;10) (Media Room Wall Control;FW;NAME;FW000004) (Media Room Wall Control;FW;FW000004;2.5.0) (Media Room Wall Control;GROUP;LIST;Media Room) (Media Room Wall Control;GROUP;ROOM;TYPE;21) (Media Room Wall Control;NAME;VALUE;Media Room Wall Control) (Media Room Wall Control;NW;AP;STATUS;OFF) (Media Room Wall Control;NW;DHCP;OFF) (Media Room Wall Control;NW;PARAMS;ACTUAL;192.168.1.227;255.255.255.0;192.168.1.1) (Media Room Wall Control;NW;SSID;HOUSENET) (Media Room Wall Control;NW;TOKEN;3a47f6d7-af5a-451c-ace8-91676048a660) (Media Room Wall Control;TIME;VALUE;2018-10-10T02:16:57Z) (Media Room Wall Control;WALLCONTROL;CONFIGURE;FANSPEED;LIGHTLEVEL)
I was testing the motion detectors in both the fan and the wall controllers and they are really responsive. I'm looking forward to exposing them as events to home automation. I've made solid progress on porting HomeSeer's plugin framework to python and will use the senseme module as a first plugin.
That sounds great. I may have to buy a wall controller.
Does the motion sensor send out an event to the clients when it detects motion? That could be useful for other things.
Merged! Thanks!
This is a major expansion of the senseme protocol you original published. The only major open item is the events API at this point. Also, including a CLI tool used to test the protocol and for like home automation integration. The tool needs better command line validation, and better error handling but it has been tested to work against multiple haiku fans and against the IOS app.