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

Consider offering control over creation of background bitmap to reduce memory usage #61

Closed
kevinjwalters opened this issue Jul 3, 2020 · 20 comments

Comments

@kevinjwalters
Copy link

kevinjwalters commented Jul 3, 2020

I've not followed the recent changes to this library but somewhere since https://github.com/adafruit/Adafruit_CircuitPython_Bundle/releases/tag/20200327 an extra bitmap is created for a background feature. This is the likely candidate for the primary cause of a MemoryError exception that now occurs using this library for the application in Adafruit Learn: CLUE Sensor Plotter in CircuitPython. An example of this exception output can be seen in the description of the second issue in Adafruit Forums: Clue_Plotter Problem.

This may affect other existing applications too. I'd guess ones with lots of text on screen, large (possibly scaled) text and/or memory tight ones are more at risk.

I'm not sure what the new bitmap is for but a decision on whether this feature should be off by default would be useful and dictate whether existing applications need testing/reviewing/changing.

@kevinjwalters
Copy link
Author

This might also be the cause for my audio becoming a little more choppy. I'm moving around some bitmaps with text underneath them while playing samples in my applicaiton. I'd shrunk them down a bit in size to reduce the pauses in playback to an acceptable amount but it's possible they got worse a week or two ago. I'd assumed this was my imagination but it's possible it coincided with me doing a library refresh and picking up a later adafruit_display_text library with the background bitmap in my Label objects.

@FoamyGuy
Copy link
Contributor

FoamyGuy commented Jul 5, 2020

@kevinjwalters if you have a moment try testing out the branch from this PR: kmatch98#2

It should avoid making the bitmap if background color is not used.

@kevinjwalters
Copy link
Author

I tried https://github.com/FoamyGuy/Adafruit_CircuitPython_Display_Text/blob/51e13b2e619c9069f74c60eeb4c2f9f64b052247/adafruit_display_text/label.py with the plotter program from my guide, same error

Traceback (most recent call last):
  File "code.py", line 197, in <module>
  File "code.py", line 162, in popup_text
  File "plotter.py", line 786, in info
  File "/lib/adafruit_display_text/label.py", line 130, in __init__
  File "/lib/adafruit_display_text/label.py", line 300, in _update_text
  File "/lib/adafruit_display_text/label.py", line 169, in _create_background_box
MemoryError: memory allocation failed, allocating 1616 bytes

I think that's expected as it sets a blue backround which triggers the creation of the bitmap even when the monospaced terminalio.FONT is in use? Perhaps there's a (reliable) way to detect if a font is proportionally spaced? If so, that would be a useful extra condition around the use of a bg bitmap to reduce memory usage for monospaced use.

@FoamyGuy
Copy link
Contributor

FoamyGuy commented Jul 6, 2020

Ahh, yeah I think you are right. It would still be making the bitmap now. I'm going to work on getting it finalized later today. I will look into the possibility of skipping the bitmap for termainli.FONT text.

@kmatch98
Copy link
Contributor

kmatch98 commented Jul 9, 2020

This was resolved in #59

The default for background color is None and now prevents creation of the bitmap. I hope this will resolve your memory usage issue.

@kevinjwalters
Copy link
Author

I've tested three different versions of the library from February, June and the most recent one in July and there's both performance differences and rendering differences using terminalio.FONT: https://www.youtube.com/watch?v=FtI1xx6o3c8

That's running on 5.3.0 btw.

@kmatch98
Copy link
Contributor

@kevinjwalters Thank you for the video, this is good insight. Can you clarify how you are generating this demo? Are your recreating the text label from scratch or moving its x,y location? If moving x,y are you doing this in a higher level group or relocating this box?

If possible please post the demo code, I want to be sure to understand the specific issue that is causing the performance change.

@kevinjwalters
Copy link
Author

@kmatch98
Copy link
Contributor

Thanks. Sorry I overlooked it in the description. Was too fixated on watching the complex storyline of the video :)

@kmatch98
Copy link
Contributor

@FoamyGuy just adding you to this so you will be aware of the performance change listed above. This probably should be added as a new issue.

So far I haven’t dug into this yet.

@caternuson
Copy link
Contributor

This appears to have been closed, but I just hit the same issue.

code.py is the clue-plotter.py program from this guide:
https://learn.adafruit.com/clue-sensor-plotter-circuitpython/circuitpython-sensor-plotter
along with the other .py files from there.

Using library bundle adafruit-circuitpython-bundle-5.x-mpy-20200728.zip, also printing __version__ below just to verify:

Adafruit CircuitPython 5.3.1 on 2020-07-13; Adafruit CLUE nRF52840 Express with nRF52840
>>> from adafruit_display_text import label
>>> label.__version__
'2.8.0'
>>>
soft reboot

Auto-reload is on. Simply save files over USB to run them or enter REPL to disable.
code.py output:
Traceback (most recent call last):
  File "code.py", line 195, in <module>
  File "code.py", line 162, in popup_text
  File "plotter.py", line 786, in info
  File "adafruit_display_text/label.py", line 128, in __init__
  File "adafruit_display_text/label.py", line 303, in _update_text
  File "adafruit_display_text/label.py", line 212, in _update_background_color
  File "adafruit_display_text/label.py", line 169, in _create_background_box
MemoryError: memory allocation failed, allocating 1616 bytes

@makermelissa
Copy link
Contributor

@caternuson I think this was closed in favor of #76.

@caternuson
Copy link
Contributor

Thanks @makermelissa. So this is still an open issue, just moved?

@makermelissa
Copy link
Contributor

As far as I can tell, that's correct.

@FoamyGuy
Copy link
Contributor

@caternuson @makermelissa the bitmap_label noted in #76 would likely help with this issue on the CLUE. But there are also some potential gains to be made in the existing label by changing the background not to use a fullsized bitmap. Possible alternatives include vectorio (but it's 6.x+) or displayio.Shape I will probably make an attempt at updating it to use displayio.Shape sometime soonish, but can't promise I will get it working, and I don't have any hard numbers on actually potential memory savings.

@kmatch98
Copy link
Contributor

kmatch98 commented Jul 28, 2020

@caternuson and @FoamyGuy

Since this could be an urgent issue with this example for the Clue, is there something we can do in the Clue learn guide program to shave a few bytes to make this work, maybe turn off background? See PR #74 that reduces memory if background is set to None.

I don’t have a Clue so I’m unable to assist other than with suggestions.

@FoamyGuy
Copy link
Contributor

Setting it to none would save whatever memory is used by the bitmap background. But the CLUE program probably needs the background to remain legible easily.

The only way that I could think of to solve the memory problem but keep the colored background was in #71 but it led to too much complexity within the label.

It may be possible to in the CLUE example code to set the background to none (avoiding the bitmap memory) and then "manually" take control of the palette in order to set the color the way that it did before bitmap backgrounds. I can give that a try.

@FoamyGuy
Copy link
Contributor

FoamyGuy commented Jul 29, 2020

Actually in this case the example code doesn't have the labels, they are inside of the CLUE library, so the change would need to be made there to attempt that last thing I mentioned. I don't think that is a good solution.

I think using displayio.Shape might the best chance for reducing the memory without the added complexity of treating built-in fonts differently.

@FoamyGuy
Copy link
Contributor

Looking a bit closer the label is inside the Plotter module which is part of the example code. Maybe as a temporary fix it could changed there to use None and manually control the palette.

@FoamyGuy
Copy link
Contributor

Okay I made an attempt to solve it by modifying the plotter module within the example to set the label to use a None background and then "manually" change the palette to get the blue color back.

adafruit/Adafruit_Learning_System_Guides#1192

I'm not sure if this is an ideal solution, but it does seem to be working to allow the example to run on the CLUE.

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

No branches or pull requests

5 participants