Skip to content
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

ST7789 using array instead of numpy #368

Merged
merged 1 commit into from
May 23, 2023

Conversation

jdlcdl
Copy link

@jdlcdl jdlcdl commented May 22, 2023

This change results in a ~10x speed-up for ShowImage() calls (from ~20ms to 2ms on pi2),
...as measured by wrapping deleted-lines/inserted-lines with something like:
t0 = time.time()
and
print("elapsed: ", time.time() - t0)

The original goal was to do-away with our dependency on 'numpy', but picamera still requires it.
Still, this 'hack' gets us closer to not needing numpy and makes the interface feel a touch more 'snappy' on both pi2 and pi0.

Leaving this here because it's what I'm using from this point forward (and I'll forget about sharing it if not reminded), for others to consider. (btw: I also found many ways to do the same thing which were 2 orders of magnitude slower than this hack).

@overcat
Copy link
Contributor

overcat commented May 22, 2023

Great PR, and we should also remove numpy from requirements.txt

@jdlcdl
Copy link
Author

jdlcdl commented May 22, 2023

... we should also remove numpy from requirements.txt

I did try this (along w/ removing an unused import from src/seedsigner/hardware/camera.py).
Since numpy would not be used in the codebase, I'd think that we don't need it specified in requirements.txt

But unless I'm doing something wrong, when I create a new virtualenv where 'numpy' is not accessible (after pip3 install -r requirements.txt) I get complaints just starting seedsigner.

File "/home/pi/seedsigner-dev/src/seedsigner/hardware/pivideostream.py", line 2, in <module>
    from picamera.array import PiRGBArray
  File "/home/pi/.envs/seedsigner-dev-env/lib/python3.10/site-packages/picamera/array.py", line 49, in <module>
    import numpy as np
ModuleNotFoundError: No module named 'numpy'

I'm aware that picamera does need numpy, and that there are some tests in pyzbar that can use numpy too.

I don't know if we can remove numpy from requirements.txt.
I don't know if this is something that picamera (and maybe pyzbar) should be requiring so that we don't have to.
I'm not sure which direction to take from here on fully removing that requirement, but I'm open to ideas that work.

Thank you for bringing that up, since it was part of the initial goal.

@newtonick
Copy link
Collaborator

ACK, tested

@newtonick newtonick merged commit dbb1d6b into SeedSigner:dev May 23, 2023
@jdlcdl jdlcdl deleted the sans_numpy_ST7789 branch May 23, 2023 20:53
@jdlcdl
Copy link
Author

jdlcdl commented May 31, 2023

While this is merged/closed, leaving a note that the comment is wrong (it's not GBR), it should simply be:

# 24-bit RGB-8:8:8 becomes 16-bit RGB-5:6:5.

I'll explain below.

Using PIL.Image.convert("BGR;16") is misnamed and deceiving, at least from a mode="RGB" instance.

What it really gives would be better named "GBRG;16", because it converts each RGB-8:8:8 pixel to:
3 highest bits of green + 5 highest bits of blue in the first byte and
5 highest bits of red + the next 3 highest bits of green (not yet expressed) in the second byte.

The per-pixel byte-order toggle then turns it into RGB-5:6:5 for the screen.

@kdmukai
Copy link
Contributor

kdmukai commented Jun 6, 2023

While this is merged/closed, leaving a note that the comment is wrong (it's not GBR), it should simply be:

# 24-bit RGB-8:8:8 becomes 16-bit RGB-5:6:5.

Can you create another mini-PR with this change so we don't lose this clarification?

@jdlcdl jdlcdl mentioned this pull request Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants