-
Notifications
You must be signed in to change notification settings - Fork 18
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
Security 1.0 support, and web page updates by dkerr64 #117
Conversation
If we can undo the last commit, so reset back to 46eab47 (allow for web password to be optional) then I would like to see this merged. |
@dkerr64, i made no changes... it just told me i had to push something... no idea why... there where no changes |
@mitchjs ok, yes there are no changes. I had cleaned up the commit history and your last merge restored that and no more. Code/files are identical. @jgstroud I closed my PR #113 in favor of this one. It is good to merge whether as it stands or squashed. Let me know if you want any cleanup on the commit history. |
Commit 9aa4436 should just get squashed as well. |
You should rebase the whole branch on main to get rid of the extra merge commits. |
I did the rebase in #118 take a look at that and see what you think. |
Oh, I just realized while testing this that the README is going to need updating since the setup has changed. Need to update the part about the default login / setting password and the new protocol setting. |
Adding the protocol setting to the web page goes directly against @thenewwazoo's requirement of no configurables. But I am told that autodetecting the protocol isn't possible, which I take to actually mean, it's possible, but it's not worth the effort and all the extra code. I also think this is a much better option than having 2 binaries. |
src/utilities.cpp
Outdated
return secType; | ||
} | ||
|
||
void write_gdo_security_to_flash(const char *filename, uint8_t* secType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is identical to write_file_to_flash
. Can we use a common function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup.... added default value to the read code
src/utilities.cpp
Outdated
file.close(); | ||
} | ||
|
||
uint8_t read_gdo_security_from_flash(const char* filename) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is nearly identical to read_file_from_flash. With a different default file value. Can we use a common function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done tweaked read to allow a default value
src/homekit_decl.c
Outdated
@@ -89,12 +89,12 @@ homekit_accessory_t *accessories[] = { | |||
NULL | |||
}), | |||
HOMEKIT_SERVICE(LIGHTBULB, .primary=false, .characteristics=(homekit_characteristic_t*[]){ | |||
HOMEKIT_CHARACTERISTIC(NAME, "ratgdo"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reasoning behind this change? It seems unrelated to this changeset. Also, I had originally done it this way and something made me change it, but I honestly don't remember why. If the change needs to be there, lets break it out into a different commit with a comment describing why we made the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i changed this to have it make more sense in the homekit app on my iphone
it shows a icon of a light bulb so might as well call it lights, and that's what it is
same for motion
but if needs be we can make this separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I don't actually see this text anywhere in HomeKit. Are you suggesting it names the accessories differently based on this value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the device from your home kit, and re-add
it gets shown (at least on my iphone) when i add new device
i dont have appletv or homepod... i just use iphone
so not sure how things work in HK on those devices
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done that numerous times and the Light shows up as "Light" and the motion detector says "Motion Sensor". I just tried to pair on another device where I don't have a hub and see the same thing. I'm not opposed to the change, but we should move it to a different commit, and I'm still very curious about it. I had it this way to begin with. Not sure my reasoning for renaming all services the same way now, but this way seems more correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i know i wouldnt have changed it to light and motion, if it already said that :)
it showed a lightbulb and said ratgdo and that just looked wierd
since its of HK type lightbulb it prob should say that :)
maybe i can screen shot what it did
me temp change it back and flash, and try
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ha, well course i have 1.0 and no motion, but light is saying light and i flashed it with ratgdo in there :)
ok, reverting back to ratgdo... and keeping eye on it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to side with @jgstroud on this one.
There are actually two settings in HomeKit, which have evolved over the years. There is Characteristic.Name
and Characteristic.ConfiguredName
. Configured name is often empty/undefined and I think is a fairly recent add (like in the last 5 years). A value is written to that by Apple Home if a user enters a "name" for the device/accessory. In absence of ConfiguredName, the apps will use the Name value, but on change will save the change to ConfiguredName.
But what should the default be? Typically you do not name a light "light" or a switch "switch" as the device type is identified by what it is. Typically you name it something like "Kitchen Counter" or "Family Room Desk" (and note that in Apple Home, if the start of the name is identical to the room it is in, then that is removed from the displayed name.... so the app will only display "Desk" for the lamp that is named "Family Room Desk".
For newly installed devices normal practice seems to be to name the device something unique so that it can easily be identified when newly installed. Typically a combination of device manufacture and serial number or last 4 digits of MAC address, or something like that. It is therefore appropriate for us to set everything to "ratgdo" or better still, a combination of "ratgdo" + MAC address... similar to what we now have for the web page title.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might go back on this. For some reason starting today while trying to reproduce the pairing failures I am now seeing the door and light show up as ratgdo and ratgdo2
@@ -1,6 +1,7 @@ | |||
// Copyright 2023 Brandon Matthews <thenewwazoo@optimaltour.us> | |||
// All rights reserved. GPLv3 License | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/comms.cpp
Outdated
return; | ||
} | ||
|
||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
picky, but there are several formatting cleanups. remove this empty comment, and fixup the mixed indentations below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI that I use Visual Studio Code to edit, and it has a C/C++ extension which will format your code (shift-opt-F on Apple keyboard). Now I'm not a fan of some of its choices (it moves open curly onto a new line for/if/while/etc statements, that I would prefer on the same line) and it is over eager to split things onto multiple lines. But it does help with consistency of formatting.
src/comms.cpp
Outdated
if (key == 0x30) { RINFO("0x30 RX (door press)"); } | ||
// wall panel is sending out 0x31 when it starts up | ||
// but also on release of door button | ||
else if (key == 0x31) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use an ENUM for all the different KEY values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add them to include/ratgdo/Packet.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i put them in comms.cpp, Packet.h really is all about SEC2.0
lockState = !bitRead(val, 3); | ||
|
||
// light status | ||
static uint8_t lastLightState = 0xff; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need this static int. Can't you just use garage_door.light? It's essentially a static last light state variable already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment for the lock state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe, but lock state doesn't really have a bool available (in Garagedoor) to compare for change
so for consistency I want to keep it the same,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a garage_door.current_lock and a garage_door.target_lock. The sec+2 logic does the same thing you are doing, except it uses the variables from garage_door rather than declaring additional statics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the vars in garage_door arnt bools for the lock status... so its compare is more complicated
i would have to convert the bool from the opener in to the enum, then look for change
i think code is simpler now with the static var, we got plenty of ram :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The web page code uses and is dependent on garage_door.current_state
and current_lock
so this does need to be consistent between protocols. Yes they are not bools, but we should still use them, If the 1.0 protocol does not provide the same level of detail, then just use the values that match... possibly just 0 and 1.
Door state:
- Open
- Closed
- Opening
- Closing
- Stopped
Lock state:
- Unsecured
- Secured
- Jammed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its just 1 byte... i feel it should be like this, as the code is looking for change in the response FROM the GDO, then sets the state of HK based on that
uint8_t lightState; | ||
uint8_t lockState; | ||
|
||
//byte secplus1States[19] = {0x35,0x35,0x35,0x35,0x33,0x33,0x53,0x53,0x38,0x3A,0x3A,0x3A,0x39,0x38,0x3A, 0x38,0x3A,0x39,0x3A}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the commented out States. Either remove or add some comments around what this one is, or maybe an ifdef. Not sure what makes sense here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i want to keep this in case its needed(for now), i have limited testing with emulation of wall plate
for those who dont have one.... i shorten the states based on my wall plate power up sequence
i pushed changed to my fork... does it sync to the PR? |
i posted bunch of cleanup and such... i guess needs to be checked again |
I have cleaned up all the commit history... essentially merged all commits from @mitchjs into one, and all my commits into one, except for the update to build_web_content that I left as a separate commit so I could retain attribution to @mitchjs for his contribution to that. @mitchjs do not push into your branch without first doing a @jgstroud time for another code review please on this cleaned-up version. |
@mitchjs and @jgstroud I have completed updates to the webpage and added them into this PR. This includes updating the webpage images and some text in the README. Time for a solid test and code review please. Oh... forgot to mention, my system is not setup for dark mode, so the screenshots I included don't have the dark background. Not sure how strongly you feel about that. |
@dkerr64 , testing it now... web page looking good... your web programming way-way-way above my skills :) |
after a weekend far as i can tell its working perfectly |
I've been really busy so I haven't had a chance to review the new changes and merge, but I haven't forgotten about this. |
The changes in this PR are all goodness in my opinion. However I am away from home this week and just checked on my device… it has lost its pairing with HomeKit. Web interface is still live, but no response in Apple home. Web page shows QR code and up time of 12 hours, which means it ran for about 3 days and crashed. I think all our efforts now need to focus on that. Need to capture logs from the moment of crash. I’m suspecting a memory leak, whatever it is, is destroying record of HomeKit pairing. I’m wondering if an automatic reboot every 24 hours would be a nasty hack of a fix. |
The web page under-the-covers is greatly improved. It is essentially a single-page application (google it) which means no reloading when switching between pages, so the JavaScript remains always loaded, which is more goodness. |
…nts to update status
I have implemented the nasty hack... automatically reboot ratgdo every 24 hours (default) configurable between 1 and 72 hours, or disable by setting to zero. I believe that this will solve the un-pairing problem but should be considered a workaround rather than a proper fix. @jgstroud what are your plans to merge this? There are significant updates worthwhile merging and getting more testing of. |
@dkerr64 this hasn't happened to me... or my friends unit... we have not had any "unpairings" |
Enough people are reporting the problem to suggest it is fairly common. And it is certainly "reliably" crashing and unpairing for me. If folks are uncomfortable with automatically rebooting every 24 hours, then I can change the default to never. |
@dkerr64 so we can break down the subsystems and try to narrow down where its crashing |
I am sec 2.0+. I do not think it is anything to do with web server... I left for vacation and did not touch the device for 3 days, and it crashed. Being on vacation there was little to no activity (ie, no open/close requests from HomeKit or anything else), everything was idle -- and yet still it crashed. I am going to disable auto reboot and leave it running with debug terminal open in hope I can capture something. |
As another data point, my three sec 1.0 devices now have 13 days of uptime without issue. |
ok, mine must have rebooted! |
so it was good all night, went to web page to look at uptime...
mitch |
i just changed build_type to debug, with hope to get more info out of the stack trace |
I'd be interested in what additional information that provides. The LoadStoreAlignment error is disconcerting! The ESP requires that memory is addressed on 32-bit boundaries for some data types. This exception occurs when a program attempts to load or store a 32-bit value at a non 32-bit aligned memory address. There are keywords (PSTR, PROGMEM) and functions specifically designed to address this. It should be easy to fix... if we can find out where the exception occurred. I am at 26 hours and waiting for my ratgdo to crash... I expect it to take 3+ days. |
ok, now a new crash... and tons more data Soft WDT reset Exception (4): Level1Interrupt: Level-1 interrupt as indicated by set level-1 bits in the INTERRUPT register
ctx: cont 0x4020d50d in ge_double_scalarmult_vartime_lowmem at .pio\libdeps\ratgdo_esp8266_hV25\HomeKit-ESP8266\src\wolfcrypt\src/ge_low_mem.c:563 --------------- CUT HERE FOR EXCEPTION DECODER ---------------` |
ok, starting to reboot often now, same WDT Reset, got bad feeling its due to changing the build_type to debug confirmed back to release build... and no WDT resets |
Can we move discussion on crash/reboot/no-response over to issue #94 please. I really want this PR merged and any code changes to fix the HomeKit bugs is going to take time and should be pushed separately from this PR. @jstroud please review this PR and advise of any changes you want before merging. |
Change default to 0/disabled please. A production build can have debug option but enabled by default is going to potentially generate alot of other issues/complaints. I'm willing to run latest builds but (knock on wood) my v.11.0 build is at 30 days up time without reboot/crash. |
I don't consider this a debug option, rather a wise precaution to keep the board running smoothly... however, that said, we can change the default to disable the periodic reboot if that is consensus. Still waiting for @jgstroud to comment. |
…24). If zero, then never reboot Update web.cpp
I merged with the default to disabled. Given the instabilities, I think it's fine to have this debug option, but enabling by default seems wrong to me. If people are having issues, its easy enough to enable. |
Security 1.0 support, and web page updates by @dkerr64 (I believe same as PR #113)