Jack477 / CommanderPi

Easy RaspberryPi4 GUI system managment
MIT License
189 stars 33 forks source link

crash with interface connection but without internet #29

Open paulwratt opened 4 years ago

paulwratt commented 4 years ago

(EDIT: I actually think this is a python bug, but the PR work-around avoids the crash) I use a portal often, that means I have an interface connection, but until I login through the portal, I dont actually have an internet connection. The auto check fails at startup, way down deep inside a python library. NOTE: this does not happen when I dont have an interface connection (eg if I disconnect wifi or ethernet).

Traceback (most recent call last):
  File "/usr/lib/python3.7/urllib/request.py", line 1324, in do_open
    encode_chunked=req.has_header('Transfer-encoding'))
  File "/usr/lib/python3.7/http/client.py", line 1244, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1290, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1239, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1026, in _send_output
    self.send(msg)
  File "/usr/lib/python3.7/http/client.py", line 966, in send
    self.connect()
  File "/usr/lib/python3.7/http/client.py", line 1407, in connect
    server_hostname=server_hostname)
  File "/usr/lib/python3.7/ssl.py", line 412, in wrap_socket
    session=session
  File "/usr/lib/python3.7/ssl.py", line 853, in _create
    self.do_handshake()
  File "/usr/lib/python3.7/ssl.py", line 1117, in do_handshake
    self._sslobj.do_handshake()
OSError: [Errno 0] Error

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/pi/CommanderPi/src/main.py", line 22, in <module>
    main()
  File "/home/pi/CommanderPi/src/main.py", line 18, in main
    start = g.Window()
  File "/home/pi/CommanderPi/src/gui.py", line 710, in __init__
    up.check_update()
  File "/home/pi/CommanderPi/src/update.py", line 50, in check_update
    with urllib.request.urlopen(url) as f:
  File "/usr/lib/python3.7/urllib/request.py", line 222, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/lib/python3.7/urllib/request.py", line 525, in open
    response = self._open(req, data)
  File "/usr/lib/python3.7/urllib/request.py", line 543, in _open
    '_open', req)
  File "/usr/lib/python3.7/urllib/request.py", line 503, in _call_chain
    result = func(*args)
  File "/usr/lib/python3.7/urllib/request.py", line 1367, in https_open
    context=self._context, check_hostname=self._check_hostname)
  File "/usr/lib/python3.7/urllib/request.py", line 1326, in do_open
    raise URLError(err)
urllib.error.URLError: <urlopen error [Errno 0] Error>

I have patched the startup to accept a auto_check = false setting in the cpi.config file as a work-around. I believe this to be a useful setting on some systems/situations. However, looking at the code, and the TODO lists in the code, I would prefer to move all config stuff to a seperate config_cpi.py and use import config_cpi as cf (if thats acceptable). I will submit a PR so you can see why I think it should be split off into a seperate file. (Also there is a issue with version in the cpi.config)

Jack477 commented 4 years ago

Config file was created to store some resources.py stuff but in my opinion there was not too much stuff to push there so it was created for future, yes maybe it was bad practice to left it empty.

About change config file to python file, it's not bad idea and it would it will be easier to work with it just import

Hmm this internet crash looks like python libary crash, I hope they will fix it.

paulwratt commented 4 years ago

if you did change cpi.config to config.py then there is no need for the cpi.config (if I understand what you said correctly?). I was not thinking of that specifically, but I dont see why that should not work, and you could eliminate the idiot proofing (option checking)

The problem I see is that there are 2 different switches that are recorded into cpi.config from the user interface (force & dark-theme).

BUT, it just occurred to me that any config file should be in ./CommanderPi/ folder, not lower down in the src directory, just to make it easy to find/use. I think then it would be easier if the python script just accesses ../cpi.config, but it still means keeping all the idiot proofing (option checking).

re: version #30, just change the version number, and have it automatically inserted into the string. Then you can explicitly match the version number when you do an update check.

But like I mentioned, there are other TODO in the code as well. I dont mind providing a "good" (clean) PR of what I have done, with or without the seperated config_cpi.py. But I am not 100% sure if I could make a ../config.py work, especially recoding "force" and "dark-theme", so a seperate "../cpi.config" still makes the most sense.

When do you think you might get around to the TODO lists in the code? I can probably do that too, but I dont want to make decisions for you if you already have any ideas. CommanderPi is a small (code) project, and not to complex (for me), so I have the time to get things working/changed/fixed pretty quickly (from my point of view - it still took 4 hours to make changes and create issue posts)

Jack477 commented 4 years ago

Thanks for your commitment at this project. Honestly CommanderPI was my first big (big for me) project which I start coding while pandemic stuff and holiday started.
I'm at last year in my school, I have e-learning but I need to learn to very important exams so I don't care much now about other things.

Back to the topic, you are right that cpi.config should be in /CommanderPi/ folder beacuse config file should be separate from source files and easy to find. A lot of TODO in code are little mesh and out of date. For now in my mind TODO was to relate more stuff from resources.py with cpi.config and make this config more usefull and make it with Pi 3b.

At now I don't have many ideas to improve CommanderPi, personally I was coding some stuff to controll fan with 3v3 and make depend it on PWMOutputDevicefrom gpiozero libary but don't have time and need to mod my colling beacuse my +- fan pins are connected together.

paulwratt commented 4 years ago

I kinda thought that was the case.

I will retract that PR and put something together in the next few days.

Apart from the changes mentioned here, there are only aesthetic things (eg the network dialog, & change [ESC] text to button on all dialogs) - ie how things look (like the CPU output I fixed originally). Maybe get it to work on PC version of RaspberryPi OS (but I dont have a PC atm - could always use qemu I guess)

back soon ..

paulwratt commented 3 years ago

I did the new #37 configcpi.py (plus others) last week, but I was unhappy with some things (needed to think about it more). Spent today testing the config, may have found a bug in the configparser (it drops comment lines when re-writing), but the new file gets around that by re-writing a full array variable (with comments). Apart from that workaround, all future code will drop duplications and only reference true variables (that can be tested) from the config file, incl. (and especially) path, so that it does not include any reference to either $HOME or /CommanderPi/, which is needed if you are trying to develop CommanderPi, as a side affect it no longer needs to be in either location

Jack477 commented 3 years ago

Ok, I did some changes in this already but I don't know, is version in config is necessary. I don't think so that user can change version by config file so in my mind it should be still at resources.py. Added function setCPiValue(section, value, CPiKey='DEFAULT') for set each config, function updateCPiConfig() in my opinion should be only used when update arrives for future compatibility.

I have re-writted comments in config as separate values started with ';' so now I don't see any bugs with writing/reading config and will drop auto_update functionality because it's annoying and you have auto_check for notification when update comes so do it manually (manually I mean by CommanderPi button click)

paulwratt commented 3 years ago

I added auto_update for a remote system, or an automated system. It was to be run from main.py so it can update the files before it started the GUI. It was just an idea, and I personally probably would not use it (except maybe on a remote server that also had X-windows installed, which I now have) (EDIT: and I just noticed that update.py is already setup to run as a shell script).

auto_check was from the original patch/PR that allows a workaround for the networking issue.

one reason to have version in ../cpi.config is that you can force an update by changing it.

yes, updateCPiConfig() is used for color_mode changes, at the beginning of change_theme() (and after update_cpi(), because the version has changed). so the update is done from one function in a single file, that would be imported into each other file. As opposed to separate config write functions in multiple files, so they config cant get out of sync.

I was going to leave a '0.0.0' in a line with app_version in resources.py, so no matter how old any version currently installed, the update check would still work for old versions.

DEFAULT is a section header, the rest are key/value pairs (as described in dictionary and keyed array lists), ie its a parent object whose value is a list of other objects.

According to python docs, section does not have a value (INI style sections). I did think about changing DEFAULT to CommanderPi just incase the config is lying around without associated files, but either way its just human readable.

I used # cos thats what is used in config.txt and .py files (and shell scripts), ; is most common in microsoft INI format files (eg windows device drivers, and window setup file lists) - its a way of teaching people "this is how you remark/comment on linux" by example.

One good reason not to keep version= in resource.py is that configcpi.py is only 4kb, so the update check will be extremely quick (which also happens to be the smallest disk block size on linux, mac and windows - yes 512 bytes is emulated for almost 10 years now) - ie resources.py is just over 11k. There should not be any need for anything else in configcpi.py except more configuration settings (some for not RPI4 and not RPi maybe).

EDIT: another reason to keep everything in configcpi.py is that it could be added to c_desktop.py to write the config at installation time

the changes to update_cpi() would include removing ::-1 - the first time I ever installed CommanderPi I could not get it to update because of the way it does the string matching. As a by-product of not checking app_version, the line can be evaluated in Python using exec, so the code knows what the new version is when it is changed, which is why it can be written back to the config. Actually, unless version is ever going to be a tuple (number value as opposed to current string value) its name can be anything.

one final note (I hope you tested the creating and rewriting properly, I only found the write problem by accident) - the way I constructed that read+create & update config sections, it will work if the filesystem is read-only (cant create) and/or the config file is read-only (cant update)

EDIT: BTW if its specific to the configcpi.py code, we can deal with details over on that PR (GH does save all even after me removing fork repo)