Python3 #192

dadr · 2021-06-08T20:15:27Z

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.

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

mrworf · 2021-06-27T13:01:34Z

Awesome commit, if you could solve the conflict I'll pull it into the branch

mrworf · 2021-06-27T13:25:41Z

routes/maintenance.py

@@ -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)


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's also a risk that files change during export of configuration but I think it's less of a concern

mrworf · 2021-06-27T13:27:36Z

routes/maintenance.py

+            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)


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.

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.

mrworf

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?

mrworf · 2021-06-27T13:28:19Z

routes/maintenance.py

@@ -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's also a risk that files change during export of configuration but I think it's less of a concern

mrworf · 2021-06-27T13:30:15Z

routes/maintenance.py

+            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)


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.

mrworf · 2021-06-27T13:32:35Z

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.

mrworf · 2021-06-27T13:37:23Z

This PR now also includes #186 - However I have another question. After 5 tries, base.py calls an exception network.RequestNoNetwork. That exception just does a pass. It seems like there was something you wanted to do there and never got to? I'm thinking that the frame should not stop at this point. Rather, it should attempt to show photos from another service or another URL at the same service. If there are no others, then it should probably keep retrying until a network connection comes back. There may be a bigger discussion to have around this - it also seems that services are not given equitable time, and if random order is selected, it seems that pictures could be taken randomly among services. I find it especially difficult to mix the simple URL with either Google Photos or USB photos. The simple URL typically returns different pictures each time it's used (e.g. https://source.unsplash.com/random) and it will get starved for attention when USB photos has a few 100 photos to show.

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 network.py implementation, that's because the exception has no functionality, it's just tossed around. If you look at slideshow.py it catches it and will determine what to do about it until network is back.

mrworf · 2021-06-27T13:49:25Z

@dadr if you want a more direct comms, feel free to sign up for slack here https://dev.sensenet.nu/ 😄

Non-functional partial commit

dadr · 2021-07-05T23:03:22Z

@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

mrworf

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.

mrworf · 2022-01-17T23:23:07Z

modules/display.py

@@ -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()


Going to have to fix this later so it pulls that info from the settings 😄

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.

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 😄

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?

mrworf · 2022-01-17T23:24:12Z

modules/slideshow.py


    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()


Same here with port

mrworf · 2022-01-17T23:24:17Z

modules/slideshow.py

            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()


Same here with port

routes/maintenance.py

mrworf · 2022-01-17T23:29:17Z

routes/upload.py

+        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'):


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?

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.

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.

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!

routes/upload.py

Added error checks against tar. Also added sanity checks for config file restoration

Error checking on tar

Simple indent error

Indent error

Dadr and others added 30 commits February 24, 2021 11:35

Update debug.py

b6d2053

Fixes to make "Log Report" button work again

Update shutdown.py

0bf9013

Update helper.py

2b352e1

Update debug.py

4a2c00c

Per our discussion with previous changes

Update helper.py

ba3b6ab

Changes per our discussion

Update base.py

66eb329

Add same logic to python3 branch for issue mrworf#182

Update display.py

079d761

Fix for python3 branch for handling images set to Do Nothing or to Fill

Update servicemanager.py

2a2dc70

Update oauth.py

cd47273

Update oauthlink.py

6633fc8

added codecs library and modified code to translate bytes to string for json.load

Update README.md

4dc9064

Updated to describe this branch specifically.

Update svc_googlephotos.py

fa84132

Bring up to date with mrworf master/python3 version

Update README.md

aff4765

Updated description of alternate color module

Update helper.py

91bc4ec

Added Code to support HEIC and HEIF images.

Update README.md

e06d83e

Spelling and grammar

Update helper.py

6a36c79

fix syntax error

Update helper.py

b9c8f50

Indent error for Copy/paste

Update helper.py

cf5deeb

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.

Update colormatch.py

e23c136

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.

Update colormatch.py

ac4ed48

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.

Update colormatch.py

f1f2548

Added check to avoid going higher in temp than a monitor can support

Update colormatch.py

4a251b6

Syntax changes

Update colormatch.py

9617546

Syntax

Update colormatch.py

7f89b2d

More Syntax - I need to try an IDE!

Update colormatch.py

914b09e

temp and lux were not set when levels were zero

Update colormatch.py

91b767c

Update colormatch.py

1992a47

Update colormatch.py

c8b1864

Update colormatch.py

91c3e4e

Make scale adjustments

Tom Anschutz and others added 8 commits June 17, 2021 16:40

Merge pull request #5 from dadr/feature/backup

2f96e5c

Feature/backup

Update main.html

d532232

First try at new config page

Update main.html

61d09c3

tidy up

Config Page Formatting

fbf4653

Update main.html

020e6ef

Update main.html

c3e6f01

Update main.html

38b97d9

Merge pull request #6 from dadr/feature/Configuration

d33f8db

Feature/configuration

mrworf reviewed Jun 27, 2021

View reviewed changes

mrworf requested changes Jun 27, 2021

View reviewed changes

Initial partial commit

7a74b7f

Non-functional partial commit

Dadr and others added 8 commits July 8, 2021 17:38

Second set of work

2ac8c75

Still not functional.

Rework restore to run as separate process.

81d5977

Create Version File

96c993a

added function to create a json version file in photoframe_config.

Merge pull request #8 from dadr/feature/backup

a584370

Feature/backup

Final Bugfixes for backup feature

b215564

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.

Merge pull request #9 from dadr/feature/backup

ad2a43e

Final Bugfixes for backup feature

Last Bugfixes

42e5130

A few dumb errors copying from Pi to Github.

Merge pull request #10 from dadr/feature/backup

da233e9

Last Bugfixes

mrworf requested changes Jan 17, 2022

View reviewed changes

Dadr and others added 4 commits January 17, 2022 22:38

Error checking on tar

6e57772

Added error checks against tar. Also added sanity checks for config file restoration

Merge pull request #11 from dadr/feature/backup

8ca44ca

Error checking on tar

Indent error

471c4f8

Simple indent error

Merge pull request #12 from dadr/feature/backup

95ecaa1

Indent error

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python3 #192

Python3 #192

dadr commented Jun 8, 2021

mrworf commented Jun 27, 2021

mrworf Jun 27, 2021 •

edited

mrworf Jun 27, 2021

mrworf Jun 27, 2021

mrworf Jun 27, 2021

mrworf left a comment

mrworf Jun 27, 2021

mrworf Jun 27, 2021

mrworf commented Jun 27, 2021

mrworf commented Jun 27, 2021

mrworf commented Jun 27, 2021

dadr commented Jul 5, 2021

mrworf left a comment

mrworf Jan 17, 2022

dadr Jan 18, 2022

mrworf Jan 18, 2022

dadr Jan 18, 2022

mrworf Jan 17, 2022

mrworf Jan 17, 2022

mrworf Jan 17, 2022

dadr Jan 18, 2022

mrworf Jan 18, 2022

dadr Jan 18, 2022

Python3 #192

Are you sure you want to change the base?

Python3 #192

Conversation

dadr commented Jun 8, 2021

mrworf commented Jun 27, 2021

mrworf Jun 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrworf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrworf commented Jun 27, 2021

mrworf commented Jun 27, 2021

mrworf commented Jun 27, 2021

dadr commented Jul 5, 2021

mrworf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrworf Jun 27, 2021 •

edited