Closed dkerr64 closed 9 months ago
@donavanbecker @thenewwazoo Can you please review this PR. It adds good function to the web page including firmware update direct from GitHub and userid/password to make any "potentially bad" changes. Userid is "admin" default password is "password" but can be changed in settings page.
I reviewed the code and generally speaking, it looks pretty good. Really like the addition of pulling the update from GitHub and still having the ability to push a local image. I'll try to find some time to build it and test it locally.
Really want to get consensus on whether we want to include the ability to control the door with the web page. I know @thenewwazoo seemed to indicate he doesn't want it.
For now I would say, after I am able to verify the code locally, I'd be willing to merge this without the GDO controls. With them enabled, I definitely want more feedback.
Thanks for your comments. I considered adding a setting to enable/disable the GDO buttons and can probably add that in, making the default to hide them (into settings page, same place I have password change).
But... given how unstable the HomeKit binding is right now, I do think it is a good thing to have these controls so that there is a fall-back way to control the door. I think @thenewwazoo was concerned about adding complexity to the code, but it turned out to be rather simple to implement.
btw... I have been reading up on browser caching, and there is a change I need to add to make sure that a browser replaces the version of a file (javascript/image/css) in its cache whenever we make a change to one of these files. I should do that before merging so that we don't have to instruct people to hold down the shift key when reloading with a new firmware version.
I built this PR and loaded it and it looks pretty good. I left a comment on the python script though, the sed command is throwing an error.
I think the change I proposed is trivial enough, I'm just going to amend it and merge it.
I think the change I proposed is trivial enough, I'm just going to amend it and merge it.
I agree the -e flag is appropriate fix.
But before you merge, there is a change I need to make to the two HTML files which if not done will require us to tell folks to hold down the shift key to reload after next firmware update... because of browser caching of CSS and JS. Can you give me 24 hours to push that please.
Oh, sure, I forgot you wanted to make that change. agreed, that's a good value add.
I think the change I proposed is trivial enough, I'm just going to amend it and merge it.
I agree the -e flag is appropriate fix.
But before you merge, there is a change I need to make to the two HTML files which if not done will require us to tell folks to hold down the shift key to reload after next firmware update... because of browser caching of CSS and JS. Can you give me 24 hours to push that please.
Actually... you can merge now, as the required change will in itself cause the browser to reload first time without shift key. Basically I need to add a query string to the hrefs... ?v=<some version number>
for the images/css/js. Can be done later
Just to document what is going on with caching... we calculate a MD5 for each of the web page files.... html/css/js/image. That MD5 is shared with the browser. Browser, in coordination with server, uses that MD5 to determine if cached page needs updated. Right now I tell browser to cache for 1 hour, but best practice is to allow caching for weeks/months, after cache times out, browser asks server for page... server compares MD5 that the browser has with that of the page on the server and either tells browser that its cached version is good, or sends the new page.
The way to force a browser to get a new page is to add a query string ?v=<some version number>
that is in fact ignored, but it forces the browser to get new page because it notices something has changed in the URL if the version number is changed. We never cache the HTML pages (index and settings) so those are always updated, and then whether to update css, javascript, images is determined by method described here.
TODO... add the ?v=<xxx>
to the html files, and update the web page content build python script to insert a version number based on the MD5.
I've done the version before using the file's last modified time from disk.
function autoVer($file)
{
$ver = filemtime(getcwd() . "/$file");
return "$file?version=$ver";
}
...
<script src="<?= autoVer('js/scriptfile.js') ?>"></script>
But that only works if some engine is dynamically creating the HTML. Build variable injected into html pages would be a good solution.
Amended and merged in #107
But that only works if some engine is dynamically creating the HTML. Build variable injected into html pages would be a good solution.
Yes I will update the build python script to inject a checksum or something. MD5 hash is what I use for the cache updates but it is quite long, 32 characters, so will look for a shorter checksum/hash.
I don't think using last modified time of the file is reliable. That date gets set when someone clones the repository, so not a reliable indicator of whether the file contents have changed.
Regarding Password/Auth and control from web.
@iyerusad thanks for the comments. The update just merged has...
Unpairing from HomeKit also requires authentication. Reboot does not.
Now authentication is a bit weird, but I don't think we can do anything about it. What I mean by that is that if you are not yet authenticated and try something like open door, it prompts for password. But...
We can definitely improve that userid/password experience. I also think we could show the default userid/password on the main page so users don't have to go searching... as we have yet to update the README!! That's what you get with a project under active development.
Brand new user of this firmware as of two days ago, and I compliment you all because after installing and using it for a bit, I had a casual list of "if only" feature ideas in my head, most of which just showed up in version 0.11. I'm really encouraged that this is being so actively and sanely worked on. Hope to contribute to it!
@dkerr64 Awesome, not a worry about the rest, just ideas at the moment. I am not currently contributing code to this project but keeping tabs on the commits/merges and testing new builds.
Updated to this release, v0.11.0 from v0.9.0.
Updates for webpage as discussed in issue #87.
TODO... we have to rationalize version numbering...
_We have our versioning a little screwy. We are tagging everything "v0.0.0" so with a leading letter "v". However in the code auto_firmwareversion we are removing that letter "v'. This complicates things because it means I have to add back in the latter "v" to do the comparison. We should consider being consistent. Either the source code uses the complete version/tag string, or we remove letter "v" from tagging. Complicated is probably not the right word, but it exposes us to future bugs if tagging suddenly stops using the letter "v".