-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
Python3 #192
base: python3
Are you sure you want to change the base?
Python3 #192
Conversation
Fixes to make "Log Report" button work again
Per our discussion with previous changes
Changes per our discussion
Add same logic to python3 branch for issue mrworf#182
Fix for python3 branch for handling images set to Do Nothing or to Fill
added codecs library and modified code to translate bytes to string for json.load
Updated to describe this branch specifically.
Bring up to date with mrworf master/python3 version
Added Ability to make use of alternate TCS34727 color sensor on a different module. This module has a better physical design than the Adafruit module, and works the same, except it has a version ID of 77 (0x4D) instead of 68 (0x44). It was bought on eBay: https://www.ebay.com/itm/133600154256
Updated description of alternate color module
Added Code to support HEIC and HEIF images.
Spelling and grammar
fix syntax error
Indent error for Copy/paste
Moved HEIC to JPG conversion from makeFullframe to Autorotate in order to catch a missing case where a HEIC was set to do nothing but changed colors when cropped to fit the screen.
Colormatch had some questionable logic at low lux levels. The changes allow temperature to be calculated as long as all the colors are not 0. And also, when there is NO light, zero temperature is probably not correct, rather make a middle-of-the-road assumption.
Modified to detect and control monitors that can be adjusted using ddc channel. These will change brightness and temperature, and not require the colormatch script.
Added check to avoid going higher in temp than a monitor can support
Syntax changes
Syntax
More Syntax - I need to try an IDE!
temp and lux were not set when levels were zero
Make scale adjustments
Feature/backup
First try at new config page
tidy up
Feature/configuration
Awesome commit, if you could solve the conflict I'll pull it into the branch |
routes/maintenance.py
Outdated
@@ -85,6 +85,16 @@ def handle(self, app, cmd): | |||
elif cmd == 'ssh': | |||
subprocess.call(['systemctl', 'restart', 'ssh'], stderr=self.void) | |||
return self.jsonify({'ssh': True}) | |||
elif cmd == 'backup': | |||
subprocess.call(['tar', '-czf', '/boot/settings.tar.gz', '/root/photoframe_config'], stderr=self.void) |
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.
Some thoughts here. The backup should include some meta info, preferably a JSON file with the following details:
- Last commit from running branch
- Which branch
- What version of the export tool was used (in case we need to handle different scenarios on restore). For the initial version, it should simply be 1 :-)
Complete folder path should also be avoided since we cannot assume /root/ will always be the destination (in the restore).
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's also a risk that files change during export of configuration but I think it's less of a concern
routes/maintenance.py
Outdated
return 'Backup Successful', 200 | ||
elif cmd == 'restore': | ||
if os.path.isfile("/boot/settings.tar.gz"): | ||
subprocess.call(['tar', '-xzf', '/boot/settings.tar.gz', '-C', '/'], stderr=self.void) |
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 should not be done within the frame, since we're multi-threaded, instead it's better to have a tool that you kick off which will shutdown the frame service, update it and then restart it. This also gives us the option to add extra steps during import of older configurations.
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.
Also, the restore here does not remove the old configuration, so you run a big risk of getting a really jumbled setup after doing restore.
One way to do restore without having downtime is to restore into /root/photoframe_config_restore
and once that is done, validate configuration and then finally shutdown, swap folders and start again. Once it successfully starts, it should delete the old setup. This also gives a safety net should something fail, allowing rollback.
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.
Sorry about the late review, but hopefully you see the reason for the comments 😄 also, if possible, can you backport this particular feature as a PR against master?
routes/maintenance.py
Outdated
@@ -85,6 +85,16 @@ def handle(self, app, cmd): | |||
elif cmd == 'ssh': | |||
subprocess.call(['systemctl', 'restart', 'ssh'], stderr=self.void) | |||
return self.jsonify({'ssh': True}) | |||
elif cmd == 'backup': | |||
subprocess.call(['tar', '-czf', '/boot/settings.tar.gz', '/root/photoframe_config'], stderr=self.void) |
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's also a risk that files change during export of configuration but I think it's less of a concern
routes/maintenance.py
Outdated
return 'Backup Successful', 200 | ||
elif cmd == 'restore': | ||
if os.path.isfile("/boot/settings.tar.gz"): | ||
subprocess.call(['tar', '-xzf', '/boot/settings.tar.gz', '-C', '/'], stderr=self.void) |
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.
Also, the restore here does not remove the old configuration, so you run a big risk of getting a really jumbled setup after doing restore.
One way to do restore without having downtime is to restore into /root/photoframe_config_restore
and once that is done, validate configuration and then finally shutdown, swap folders and start again. Once it successfully starts, it should delete the old setup. This also gives a safety net should something fail, allowing rollback.
Btw, as a general comment, use the first line in your commit to give a quick summary, don't use it to say what file you changed, since it makes it very hard to see at a glance what commits did what. |
I believe it does? The UX has an option on what to do. Problem is that services cannot hint to the system if they depend on network or not, so instead it will use cached images to continue working when no network is available. If you're referring to the |
@dadr if you want a more direct comms, feel free to sign up for slack here https://dev.sensenet.nu/ 😄 |
Non-functional partial commit
@mrworf, Thanks for the comments and inputs. I'm working on the changes. I'll let you know when they are done. |
Still not functional.
added function to create a json version file in photoframe_config.
Feature/backup
Provides running (if rough) code to restore settings from /boot and from the web browser. Also has a small improvement in debug web page to show system info.
Final Bugfixes for backup feature
A few dumb errors copying from Pi to Github.
Last Bugfixes
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.
Overall good, the port issue isn't blocking, but you need to check return codes from tar, otherwise people may assume all is good but in reality no backup was made.
@@ -184,7 +184,7 @@ def message(self, message, showConfig=True): | |||
|
|||
url = 'caption:' | |||
if helper.getDeviceIp() is not None and showConfig: | |||
url = 'caption:Configuration available at http://%s:7777' % helper.getDeviceIp() | |||
url = 'caption:Configuration available at http://%s' % helper.getDeviceIp() |
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.
Going to have to fix this later so it pulls that info from the settings 😄
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 was looking at this, and thought it might be a simple fix (copying the server port from the code at line 175). But that code crashes when I use it here. I get a "server" is not defined when I try to use server.get_server_port(). Seems there may be a bug at 175 as well.
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 been thinking about this too, server will fail since it's not exposed to this namespace, an option is to have the server expose a hook that the helper registers to. But like I mentioned, this doesn't have to be solved at this time since the default is 80 and anyone changing will hopefully also know what they're doing 😄
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 about frame.py? It parses the cmd line args and sets the port to start the server. So, instead of asking the server what port it is at, we collect the information at the start of the program and make it available with all the other base configuration variables?
|
||
def handleErrors(self, result): | ||
if result is None: | ||
serviceStates = self.services.getAllServiceStates() | ||
if len(serviceStates) == 0: | ||
msg = 'Photoframe isn\'t ready yet\n\nPlease direct your webbrowser to\n\n' | ||
msg += 'http://%s:7777/\n\nand add one or more photo providers' % helper.getDeviceIp() | ||
msg += 'http://%s/\n\nand add one or more photo providers' % helper.getDeviceIp() |
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 here with port
else: | ||
msg = 'Please direct your webbrowser to\n\n' | ||
msg += 'http://%s:7777/\n\nto complete the setup process' % helper.getDeviceIp() | ||
msg += 'http://%s/\n\nto complete the setup process' % helper.getDeviceIp() |
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 here with port
elif item == 'config': | ||
# if user does not select file, browser also | ||
# submit an empty part without filename | ||
if file.filename == '' or not file.filename.lower().endswith('.tar.gz'): |
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.
Probably should use magic module here if possible since file extensions can be ... unreliable ;-) and since this is sent on to possibly tar
with minimal checks, we could create a bigger issue. Should also ideally validate that the .tar.gz file contains a backup, but perhaps the restore tool does that?
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.
Yes, the restore tool does that. It could also check a restored backup to see if the typical files and directories are there. Is that what you had in mind? I started going down that path, and then it seemed I was re-writing the whole load config section of photoframe. I thought it would be better just to try to load the config.
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 think just checking that the bare minimal files are there including the version number (I think we spoke about this a while ago 😉 ) should be enough. Just so people don't load a non-relevant TGZ file.
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 checked against the 2 directories and 2 top-level json files - not the services/services.json file. I think we're protected from random TGZ files. However, I'm wondering what a valid configuration upload is or should be? At this point it seems that configurations are pretty mobile across all releases and devices - meaning I can see trying to move a config across all sorts of systems - especially rather than trying to re-do the auth credentials for google. ;-) I have used the same config file to jumpstart systems on various branches and hardware.
I have added code to create a version.json file in settings. It includes the linux and python versions, as well as git origin branch and commit. That says exactly what saved the settings. At some point I think it may become the case that some settings cannot be restored to some other versions, and I wonder if strict checking against the same origin/branch/commit is good. For example, if I were concerned about trying some settings in a new system, I'd likely backup the existing setting to /boot/ and then try to upload the new ones. If they should fail, I could restore from /boot/.
I realize that this does not include a separate version number (i.e. "1"), it would be dead-easy to add, but I'm wondering what that would provide that the origin/branch/commit do not? Thanks!
Added error checks against tar. Also added sanity checks for config file restoration
Error checking on tar
Simple indent error
Indent error
I think this is just a minor change to the Readme.md - to change the install instructions to point to mrworf/python3 instead of my fork. But I also think that GitHub has lost sync of the changes, as it thinks that there are 46 commits I made since the last PR that still must be applied. However, when I look at the code on your python3 branch, it seems up to date with these commits. If I'm doing something wrong with using GitHub, please feel free to share a clue.