Eink status #719

mjustian · 2024-05-15T14:55:05Z

What does this change intend to accomplish?

This adds two new pieces of text to the e-ink display: Serial Number and Status. The serial number is taken off of the EEPROM. The status defaults to showing whether the AmpliPi is playing or not, but a custom status can be sent using a function as well. A numerical status is interpreted as an error code.

Checklist

Have you tested your changes and ensured they work?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?
If applicable, have you updated the documentation/manual?
If applicable, have you updated the CHANGELOG?
Does your submission pass linting & tests? You can test on localhost using ./scripts/test

rtertiaer

I think my requests are entirely nitpicks, besides the early-return if statement stuff I'd like to see change into an elif structure in ctrl.py as we talked about in person. Also tbf my eyes glaze over a little bit with the eeprom changes, but they looked fine; I also tend to trust these changes if @Lohrer had a hand in it.

amplipi/display/einkdisplay.py

amplipi/display/einkinterface.py

Lohrer · 2024-05-21T16:00:54Z

Before I review this, can you confirm that:

The TFT display on older units still functions properly.
An AmpliPro/Streamer with an unprogrammed EEPROM (all 1's) shows an error
The Streamer EEPROM can still be programmed with hw/tests/config_streamer.bash
Tests for any API changes are added (I believe tests/test_rest.py is the correct spot)

linknum23 · 2024-05-21T20:49:34Z

amplipi/ctrl.py

  def get_state(self) -> models.Status:
    """ get the system state """
    self._update_sys_info()
+    self._update_serial()


This looks like it is called every get(). I realize that internally the serial number is cached so that isn't actually what happens. Why isn't this called in reinit instead? That is where we read a similar property set, the firmware versions.

It didn't work in reinit since other things were trying to reach the EEPROM at the same time

That's not true, it didn't work in reinit at first because you put it before the preamp's micro was assigned an I2C address, so it couldn't communicate. We didn't realize that until after you had already moved the serial reading to here. There's no reason you couldn't move it back, it just needs to be after the line that calls rt.Rpi() to setup the micro/firmware.

I tried moving it back to reinit where the firmware version is and it stopped working on the regular sized amplipi

linknum23 · 2024-05-21T20:57:31Z

amplipi/display/einkdisplay.py


-SysInfo = namedtuple('SysInfo', ['hostname', 'password', 'ip'])
+
+SysInfo = namedtuple('SysInfo', ['hostname', 'password', 'ip', 'status_code', "serial_number", "ext_count"])


Yiou could turn the SysInfo(None, None, None...) into SysInfo() if you use this initialization:

Suggested change

SysInfo = namedtuple('SysInfo', ['hostname', 'password', 'ip', 'status_code', "serial_number", "ext_count"])

_info_fields = ['hostname', 'password', 'ip', 'status_code', "serial_number", "ext_count"]

SysInfo = namedtuple('SysInfo', _info_fields, (None,) * len(_info_fields))

mypy does not like this for whatever reason

amplipi/eeprom.py

linknum23 · 2024-05-22T13:43:13Z

amplipi/eeprom.py

    """Read data from address in EEPROM."""
    try:
-      return self._i2c.read_i2c_block_data(self._address, address, length)
+      if self._bus() == 1:


Should we add an enumeration for the different i2c bus #'s?

linknum23 · 2024-05-22T13:45:17Z

amplipi/eeprom.py

-                     self.get_board_type(),
-                     self.get_board_rev())
-
+  # TODO: Do we need this? If so implement like the _read() function above. If not remove.


If we remove this, it probably makes sense to keep this pretty comment somewhere

linknum23 · 2024-05-22T14:34:04Z

As discussed: In the future we may want to add a slim barcode that can be scanned to quickly link to our troubleshooting guide when there is an error.

codecov-commenter · 2024-05-22T14:47:28Z

Codecov Report

Attention: Patch coverage is 21.95846% with 263 lines in your changes are missing coverage. Please review.

Project coverage is 49.81%. Comparing base (b516bb7) to head (9230ff9).
Report is 45 commits behind head on main.

Files	Patch %	Lines
amplipi/display/einkdisplay.py	0.00%	168 Missing ⚠️
amplipi/eeprom.py	45.29%	64 Missing ⚠️
amplipi/ctrl.py	44.11%	19 Missing ⚠️
amplipi/display/einkinterface.py	0.00%	11 Missing ⚠️
amplipi/display/__init__.py	0.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #719      +/-   ##
==========================================
- Coverage   50.90%   49.81%   -1.10%     
==========================================
  Files          25       26       +1     
  Lines        5838     6299     +461     
==========================================
+ Hits         2972     3138     +166     
- Misses       2866     3161     +295

Flag	Coverage Δ
unittests	`49.81% <21.95%> (-1.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

amplipi/display/einkinterface.py

amplipi/display/einkdisplay.py

docs/manual/EINK_ERROR_CODES.md

amplipi/ctrl.py

amplipi/display/einkdisplay.py

amplipi/eeprom.py

Lohrer · 2024-05-23T20:33:58Z

amplipi/display/einkdisplay.py

          return font
    except Exception as exc:
      raise Exception(f'Failed to load {self.fontname} font') from exc
    return ImageFont.truetype(self.fontname, 10)


-def get_info(iface, default_pass) -> SysInfo:
+def update_logs(status) -> None:


What is the purpose of this? It looks like you're reinventing logging here. All the rest of our logs are captured by journald (it's just capturing stdout and stderr, nothing fancy) and are viewable with journalctl -f or by going to Settings->About->Logs in the AmpliPi web UI which pulls from the same journal service.

Also I tested it and the only log message I'm seeing in display-logs.json is "Starting Up". I tried changing the status and no other statuses show in that log file.

This was one possible path I mentioned on day 2 as a persistent store of statuses to display. It may be vestigial now, particularly if we don't have external things reading or writing status, to this file or elsewhere - everything is simply represented as a state from the API.

It would make a lot more sense to move the log file's information to the actual logs. I'm making that change now.

rtertiaer · 2024-05-23T20:44:50Z

One ask here would be to surface these same statuses on the TFT display; you can borrow the unit on my desk for this if you'd like.

Lohrer

Much better! One last request would be to add display-status.txt to the .gitignore file so that git doesn't try to track it.

I think it would be awesome in the future to define these error codes as a {Short Name, Description} instead of just a number. That way we can display the short name on the display and the description in the logs and maybe webUI?

Lohrer · 2024-05-28T13:51:46Z

docs/manual/EINK_ERROR_CODES.md

@@ -0,0 +1,14 @@
+# AmpliPro E-Ink Display Error Codes
+
+The following numerical codes are displayed on the AmpliPro's E-Ink Display when the corresponding error occurs. Note that these codes do not appear on older AmpliPi/AmpliPro devices that use TFT touchscreen displays.


I assume this can be changed since it shows on all displays now? Also there are other references in this doc to just E-Ink displays that should be generalized.

Suggested change

The following numerical codes are displayed on the AmpliPro's E-Ink Display when the corresponding error occurs. Note that these codes do not appear on older AmpliPi/AmpliPro devices that use TFT touchscreen displays.

The following numerical codes are displayed on the AmpliPro's Display when the corresponding error occurs.

Lohrer · 2024-05-28T13:54:47Z

amplipi/display/statusinterface.py

@@ -0,0 +1,41 @@
+#!/usr/bin/env python3
+"""E-Ink Display Interface"""


Suggested change

"""E-Ink Display Interface"""

"""Display Status Interface"""

Or something similar, since this shows on all displays now.

rtertiaer

two nitpicks, but otherwise heck yes 😎

rtertiaer · 2024-05-29T14:50:12Z

amplipi/display/common.py

+
+import netifaces as ni
+
+from loguru import logger as log


could you use logging pls?

rtertiaer · 2024-05-29T14:51:08Z

amplipi/display/common.py

+
+import netifaces as ni
+
+from loguru import logger as log


Suggested change

from loguru import logger as log

import logging as log

rtertiaer · 2024-05-29T14:59:16Z

hw/tests/config_streamer.py

+BOARD_REV_RE = re.compile(r"(\d+)([A-z])")  # accept lower or upper case change designator
+# we will uppercase later


for future reference, it's normally less than ideal to obscure git history through changes like this that only address stylistic concerns.

Out of curiosity, what do you feel this is obscuring? I suggested this change in the first place so I'd like to know for my own future reference. I felt that since we were already making changes in this area of the file, since the auto-formatter added a space on this line already, and since in the commit you can clearly see it's just a change in the comment, that it was worth fixing the typo now.

mjustian force-pushed the eink-status branch from 4d9673c to b6cb6fa Compare May 20, 2024 20:10

rtertiaer self-requested a review May 20, 2024 20:21

rtertiaer approved these changes May 20, 2024

View reviewed changes

amplipi/display/einkdisplay.py Outdated Show resolved Hide resolved

amplipi/display/einkdisplay.py Outdated Show resolved Hide resolved

amplipi/display/einkdisplay.py Outdated Show resolved Hide resolved

amplipi/display/einkinterface.py Outdated Show resolved Hide resolved

linknum23 reviewed May 21, 2024

View reviewed changes

linknum23 reviewed May 22, 2024

View reviewed changes

amplipi/eeprom.py Outdated Show resolved Hide resolved

linknum23 reviewed May 22, 2024

View reviewed changes

mjustian force-pushed the eink-status branch from 559c1fd to 4042a4d Compare May 22, 2024 14:41

Lohrer requested changes May 22, 2024

View reviewed changes

rtertiaer mentioned this pull request May 22, 2024

Add status to E-INK display #690

Closed

Lohrer reviewed May 23, 2024

View reviewed changes

mjustian force-pushed the eink-status branch from 5f414de to f594d4c Compare May 28, 2024 14:33

Lohrer approved these changes May 28, 2024

View reviewed changes

rtertiaer approved these changes May 29, 2024

View reviewed changes

Added Status and Serial No. to Display, added Serial No. to API

aaa3b4d

mjustian force-pushed the eink-status branch from f54fa83 to aaa3b4d Compare May 29, 2024 15:26

mjustian merged commit aaa3b4d into main May 29, 2024
3 checks passed

mjustian temporarily deployed to github-pages May 29, 2024 15:32 — with GitHub Pages Inactive

mjustian deleted the eink-status branch May 29, 2024 15:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eink status #719

Eink status #719

mjustian commented May 15, 2024 •

edited

rtertiaer left a comment

Lohrer commented May 21, 2024 •

edited by mjustian

linknum23 May 21, 2024

mjustian May 21, 2024

Lohrer May 22, 2024

mjustian May 22, 2024 •

edited

linknum23 May 21, 2024

mjustian May 22, 2024

linknum23 May 22, 2024

linknum23 May 22, 2024

linknum23 commented May 22, 2024

codecov-commenter commented May 22, 2024

Lohrer May 23, 2024

rtertiaer May 23, 2024

mjustian May 24, 2024

rtertiaer commented May 23, 2024

Lohrer left a comment

Lohrer May 28, 2024

Lohrer May 28, 2024

rtertiaer left a comment

rtertiaer May 29, 2024

rtertiaer May 29, 2024

rtertiaer May 29, 2024

Lohrer May 29, 2024


		SysInfo = namedtuple('SysInfo', ['hostname', 'password', 'ip'])

		SysInfo = namedtuple('SysInfo', ['hostname', 'password', 'ip', 'status_code', "serial_number", "ext_count"])

	SysInfo = namedtuple('SysInfo', ['hostname', 'password', 'ip', 'status_code', "serial_number", "ext_count"])
	_info_fields = ['hostname', 'password', 'ip', 'status_code', "serial_number", "ext_count"]
	SysInfo = namedtuple('SysInfo', _info_fields, (None,) * len(_info_fields))

		@@ -0,0 +1,14 @@
		# AmpliPro E-Ink Display Error Codes

		The following numerical codes are displayed on the AmpliPro's E-Ink Display when the corresponding error occurs. Note that these codes do not appear on older AmpliPi/AmpliPro devices that use TFT touchscreen displays.

		@@ -0,0 +1,41 @@
		#!/usr/bin/env python3
		"""E-Ink Display Interface"""

		BOARD_REV_RE = re.compile(r"(\d+)([A-z])") # accept lower or upper case change designator
		# we will uppercase later

Eink status #719

Eink status #719

Conversation

mjustian commented May 15, 2024 • edited

What does this change intend to accomplish?

Checklist

rtertiaer left a comment

Choose a reason for hiding this comment

Lohrer commented May 21, 2024 • edited by mjustian

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjustian May 22, 2024 • 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

Choose a reason for hiding this comment

linknum23 commented May 22, 2024

codecov-commenter commented May 22, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtertiaer commented May 23, 2024

Lohrer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtertiaer 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

mjustian commented May 15, 2024 •

edited

Lohrer commented May 21, 2024 •

edited by mjustian

mjustian May 22, 2024 •

edited