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
Port LCD to python3 #120
Port LCD to python3 #120
Conversation
The LCD class does this automatically.
[https://docs.python.org/3/library/logging.html#logging.shutdown] When the logging module is imported, it registers this function as an exit handler (see atexit), so normally there’s no need to do that manually.
to solve conflict
- The logic to detect the address isn't suitable in an initialiser. Failure is messy. Move it up into main. - lcd_clear is used once but text is immediately displayed so it is pointless. Just remove it.
- Remove i2c_lib. We're only using one function.
We don't need the extra indirection of the LCD class in emonPiLCD, just move the useful functionality into lcddriver and use it directly. Just try to create the lcd object, it will throw an exception if it can't communicate with the LCD driver.
This allows using the modern MQTT topics, rather than the deprecated "packed" format. Add these to the config file too.
global is only required when assigning to a name within a function, to disambiguate between a local variable with the same name. These functions don't assign to these names, so global is misleading.
Re https://community.openenergymonitor.org/t/python3-for-emonhub/12670/79 Install process needs to be updated to accommodate this. openenergymonitor/EmonScripts#93
Once done, as part of the PR for Python3
Publishing the Python3 updated emonpi repo will then pull in the right install script and service file for Python3. |
OK what about this :
@TrystanLea @borpin : would you be happy with something like that ? |
No it will just confuse things and the emonpi install is dependednt on the right emonscripts repo version. If we move the install to the lcd folder and make the service file, the install & update scripts and the lcd python script all match in terms of python version it will just work as it has for emonhub (with a few glitches). The emonscript then just calls the install (or update) script in the lcd folder and the right script get executed. What we do need to handle is the update. In the existing python2.7 version, as well as moving the install script we create an update script that, for the 2.7 version, is empty. Also need to get the update process to call it. All that is part of the emonscripts update i.e. nothing actually changes, just the way things are called change. When the python3 version of the repo is created, the update script is created. You could also add a version.txt to the repo to be checked. It also has the advantage that, it you change branches of the emonpi repo, the install/update process will automatically pick up the right version of the lcd script. |
It should not need to check for the emonpi folder. If it isn't there, something is far wrong! Create log folder - do that in the main install script. All it needs is to check for the install script and then run it. It is just that both repos need to be updated at the same time (well emonpi first then emonscripts). Cheers |
This should be done by the service as an ExecStartPre=. /var/log might be a tmpfs, so the service should depend on var-log.mount as well. |
Just thinking that the way to know what version of Python to install would be by interrogating the hash bang of the script. |
That's a possibility. We should be moving everything to python 3 though. Python 2 is out of support as of (last!) January. Nobody keen to just package this as a deb which specifies its own dependencies and let apt handle it? |
Totally agree. For any release, if the install/update script matches the python script, then the right packages should be installed so a check isn't actually required. Oh and always add a |
Just to share something a little bit updated : python version catched by interrogating the shebang of emonPiLCD.py + addition of a 1) emonpilcd.sh file from the EmonScripts could become :
the test on the emonpi folder in the $openenergymonitor_dir folder is just there to initialize the process on a 'blank' OS.....if for some reason, the scripted install was stopped and relaunched, we just make a git pull..... 2) creation of an install.sh file in the lcd folder of the emonpi repo :
could be an interesting approach 👍
Sorry, but this is not clear for me : a new python3 branch in the repo emonpi (if so we do not need to track the python version) ? an update script in the update directory of the repo EmonScripts ? TO DO : define where the log folder is created |
Updating my proposition so that the service file can create the log...
|
There is no need to check for the python version. Create this method for current script, the install will use python. Update this repo. modify the emonscripts script and update emonscripts. When the python3 version of LCD is released, it should include a python3 version of the install. Note, the service file should be as per these service files https://github.com/emoncms/emoncms/blob/28fb790b1aba5445f0b6c984e555e5502e491af2/scripts/services/feedwriter/feedwriter.service#L46 The install of the service file also needs to account for non-raspberry installs. Suggest you do this as a PR for here (service file and install script) and a PR in emonscripts. The Python 3 PR then needs to include the changes needed to the install script. |
The emonpi repo is only for emonpi so only for raspberry platform, no? |
Ah yes of course 😁 |
Thanks @bwduncan for all of your work on this. Just working through testing now that I've merged @alexandrecuer's pull request. There is a conflict in the service file and I wondered what you both think should be present in the resolved version: @bwduncan's version:
@alexandrecuer's version:
|
I've merged this pull request into a branch called python3_lcd including an initial resolution of the service file conflict: https://github.com/openenergymonitor/emonpi/tree/python3_lcd https://github.com/openenergymonitor/emonpi/blob/python3_lcd/lcd/emonPiLCD.service |
Definitely Alex's version, but with the description from mine if you can be
bothered. Thanks!
…On Mon, 3 Aug 2020, 09:44 Trystan Lea, ***@***.***> wrote:
Thanks @bwduncan <https://github.com/bwduncan> for all of your work on
this. Just working through testing now that I've merged @alexandrecuer
<https://github.com/alexandrecuer>'s pull request.
There is a conflict in the service file and I wondered what you both think
should be present in the resolved version:
@bwduncan <https://github.com/bwduncan>'s version:
# Systemd unit file for emonPiLCD script
# INSTALL:
# sudo ln -s /usr/share/emonPiLCD/emonPiLCD.service /lib/systemd/system
# RUN AT STARTUP
# sudo systemctl daemon-reload
# sudo systemctl enable emonPiLCD.service
# START / STOP With:
# sudo systemctl start emonPiLCD
# sudo systemctl stop emonPiLCD
# VIEW STATUS
# systemctl status emonPiLCD
# VIEW LOG
# journalctl -f -u emonPiLCD
[Unit]
Description=emonPi LCD driver
Wants=mosquitto.service
After=mosquitto.service
# Needs to run as root so disable/enable ssh and shutdown emonpi
[Service]
Type=idle
ExecStart=/usr/bin/python3 /usr/share/emonPiLCD/emonPiLCD.py
User=root
# Restart script if stopped on a failure. Will not restart if not configured correctly
Restart=on-failure
# Wait 60s before restart
RestartSec=60
# Tag things in the log
# If you want to use the journal instead of the file above, uncomment SyslogIdentifier below
# View with: sudo journalctl -f -u emonPiLCD -o cat
SyslogIdentifier=emonPiLCD
[Install]
WantedBy=multi-user.target
@alexandrecuer <https://github.com/alexandrecuer>'s version:
# Systemd unit file for emonPiLCD script
# installation is done via the install.sh script in the same folder
# VIEW STATUS
# systemctl status emonPiLCD
# VIEW LOG
# journalctl -f -u emonPiLCD
[Unit]
Description=emonPiLCD service description
After=mosquitto.service
[Service]
User=pi
PIDFile=/var/run/emonpilcd.pid
Environment='LOG_PATH=/var/log/emonpilcd'
ExecStart=/opt/openenergymonitor/emonpi/lcd/emonPiLCD.py --logfile=${LOG_PATH}/emonpilcd.log
PermissionsStartOnly=true
ExecStartPre=/bin/mkdir -p ${LOG_PATH}/
ExecStartPre=/bin/chgrp -R pi ${LOG_PATH}/
ExecStartPre=/bin/chmod 775 ${LOG_PATH}/
Type=simple
Restart=always
[Install]
WantedBy=multi-user.target
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#120 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADSULHIS524ED65OR4WHCTR6Z2FXANCNFSM4N5VZKIA>
.
|
No need for a drop in as this service is only any use on a Pi! |
Thanks @bwduncan I've update the service description. For some reason testing this here, I dont get anything printed to the LCD, when I revert back to running a new script I've added with python2 it seems to print sometimes and not others. Never prints anything:
Works intermittently:
|
When I was testing earlier with python2 I didnt see any of these issues, so I dont think its the LCD hardware. |
I'm on a bus now heading off to my holidays so I'm afraid I won't be able
to spend much time on this this week. With any luck I'll have no phone
signal at all 😉
However, I did make some changes to the LCD libraries, to remove unused
code, remove a layer of indirection and add support for custom characters.
It's possible that the API has changed in a way your test script wasn't
expecting. Can you post your script?
Bruce
…On Mon, 3 Aug 2020, 10:15 Trystan Lea, ***@***.***> wrote:
When I was testing earlier with python2 I didnt see any of these issues,
so I dont think its the LCD hardware.
I reckon there is something in the updated libraries, but then that
shouldnt have affected running using python2.. strange..
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#120 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADSULASUBF4KAJU7N6SXTTR6Z53DANCNFSM4N5VZKIA>
.
|
Thanks @bwduncan, ah dont worry, have a nice holiday!! Are you going to the mountains somewhere? Here's the test script: the part that prints to the LCD is:
but this issue affects the main emonpiLCD.py script as well. Im just trying a fresh install here to see if the issue persists. |
Cannot test right now as i am far from office...in holidays :-)....with no emonpi nearby... previously I proceeded to some testings with both Bruce python3 emonpi version and the emonpilcd service file in the 'emonhub' mood (ie as recently merged by Trystan).... indeed, there were some problems : the lcd could remain blank as described by Trystan.... if i remember well, everything was going fine (without too much effort....) till Bruce last commit on his fork (15 of june) the last commit i could successfully test is this one I think : When I get back, I'll run some tests with Bruce last commits but you might find the solution before ;-) Best |
Seems there was a very slight timing related issue, introducing a small delay here seems to have sorted it: Should I introduce a delay after all writes? It seems to be working as it, perhaps we introduce a delay to the other places only if an issue is presented? |
That's really weird because there is already a delay in
lcd_write_four_bits. But hey if it works... I tried to find a data sheet
which gave the timings for these i²c transactions but I got lost in a
myriad of documents and gave up
…On Mon, 3 Aug 2020, 15:09 Trystan Lea, ***@***.***> wrote:
Seems there was a very slight timing related issue, introducing a small
delay here seems to have sorted it:
0c85f82
<0c85f82>
Should I introduce a delay after all writes? It seems to be working as it,
perhaps we introduce a delay to the other places only if an issue is
presented?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#120 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADSULHCLCGZA2CU2JK7EETR63AJHANCNFSM4N5VZKIA>
.
|
Thanks @bwduncan always hard to track these kinds of issues down, I wonder if there may be some variation in LCD hardware batch that may have resulted in it showing up this time, who knows! something to keep an eye on I guess |
This isn't ready for release yet, I should have used a different branch name 🤷 but this PR could be a handy way to consolidate comments.