Sparkline memory improvements #59

matemaciek · 2022-12-30T23:36:16Z

use cyclic buffer for storing values to reduce fragmentation
draw as a single Polygon instead of multiple Lines

In my case current implemntation was running out of memory on ~64 data points, now I can show all 160 data points that I wanted to.

Since we specify the maximum ammount of data points, we can use cyclic buffer underneath, therefore avoiding memory fragmentation. This should also help for problems decribed in adafruit#25

Apparently contributors are not listed there

Update docs

Previous implementation used multiple Line elements, causing unnecessary memory overhead

matemaciek · 2022-12-30T23:36:50Z

This should help for #25

Formatting fixes

Triangle is a subclass of Polygon, so we need to provide support for more than one color in palette

Decouple polygon drawing from creating bitmap, so the code can be reused for drawing on external bitmaps

Fix some of the old warnings, reenable some of overrides

And rewrite Sparkline to be a subclass of MultiSparkline

FoamyGuy

@matemaciek Thanks for working on improving the efficiency of this!

I've spotted a few issues attempting to run the existing examples with the new version of the repo. We'll definitely want to get those straightened out either by modifying the classes as needed, or modifying the examples if an API has changed or something causing a need for that.

Some are noted at relevant sections of the code.

With the "triple" example, specifically sparkline2 within it, I've also found that the new version of the library raises a pixel out of bounds error:

Traceback (most recent call last):
  File "code.py", line 245, in <module>
  File "/lib/adafruit_display_shapes/sparkline.py", line 96, in add_value
  File "/lib/adafruit_display_shapes/multisparkline.py", line 197, in add_values
  File "/lib/adafruit_display_shapes/multisparkline.py", line 316, in update_line
  File "/lib/adafruit_display_shapes/multisparkline.py", line 237, in _draw
  File "/lib/adafruit_display_shapes/polygon.py", line 101, in draw
  File "/lib/adafruit_display_shapes/polygon.py", line 161, in _line_on
IndexError: pixel coordinates out of bounds

It seems to only be a problem with sparkline2, but I'm not certain exactly why that one is the problematic one. Likely something about the behavior around the range, or size and shape has changed a little bit causing it.

I've started debugging a bit, but don't have a conclusive answer yet. So far it's looking like it tries to put pixels either at negative y values, or at y value 100 which is 1 pixel too high, 99 is the largest available index in the bitmap in question.

From that it seems to point toward there being some logic issue in the conversion between points in the range of the sparkline graph to points in the 2D plane of the pixels in the bitmap. Or maybe a bug in the example that was previously handled more gracefully despite being a bug?

We'll need to get it figured out and fixed as well though. We want to make sure all examples in the repo. are able to run successfully.

adafruit_display_shapes/multisparkline.py

adafruit_display_shapes/sparkline.py

matemaciek · 2023-01-07T19:11:51Z

Thanks for the review! I've addressed inline comments directly. As for of-by-one errors: The existing version (in original repo) seem to have a semi-bug where in some scenarios it draws on larger bitmap than requested - but since bitmap size is auto-set basing on list of points provided, they always fit. I'll debug the existing example that is failing, as I tought I fixed that bug (-:

matemaciek · 2023-01-07T19:17:18Z

While I'm working on fixing issues above, a screenshot of how it looks in action:

matemaciek · 2023-01-07T21:58:01Z

OK, I know where the bug is (-: When reorganising the code I broke an edge case when first point of sparkline would be out of screen. Working on a fix.

FoamyGuy

Thanks for digging in and figuring out some fixes!

I think you also fixed a another issue I noticed yesterday (with the released version ) where the dynxwidth config sometimes causes the line to be drawn below the chart. The latest version from this PR does not exhibit that issue any more.

The functionality on this is looking good to me now. I tested the 3 existing examples and all are looking good with this version.

I have one question about the static functions, and one question whether we can use a builtin draw_line instead of needing an implementation here.

FoamyGuy · 2023-01-08T15:11:24Z

adafruit_display_shapes/polygon.py


        super().__init__(
            self._bitmap, pixel_shader=self._palette, x=x_offset, y=y_offset
        )

-    # pylint: disable=invalid-name, too-many-locals, too-many-branches
+    @staticmethod
+    def draw(


I'm curious about a few of these methods being static. draw(), _line_on() and _safe_draw() Did this change to static factor in to memory efficiency somehow?

Is there a situation in which they are intended for use outside of in an instance of this class ultimately inside of a sparkline?

My first inclination was that they could be regular functions and could access bitmap from self rather than having it passed in separately. That way they are always acting upon only the bitmap that is part of this instance.

I see resposibilities as follows:

Polygon class: class containing logic of drawing polygons

polygon object: bitmap containing single drawn polygon

Having draw() method as static alows us to reuse drawing logic to draw multiple times on the same bitmap - which allows for two things previously not possible:

having multiple polygons on single bitmap (lower memory usage)

reusing same bitmap for sequential redrawing of different polygons on the same bitmap (common case for sparklines, and not recreating bitmap over and over potentially means less fragmentation).

FoamyGuy · 2023-01-08T15:15:45Z

adafruit_display_shapes/polygon.py

+
+    # pylint: disable=too-many-branches, too-many-locals
+    @staticmethod
+    def _line_on(


I think it may be possible to use this built-in bitmaptools function from the core rather than having the implementation in python here. https://docs.circuitpython.org/en/latest/shared-bindings/bitmaptools/index.html#bitmaptools.draw_line

Perhaps ultimately we could make a draw_polygon() function inside bitmaptools as well which could then be used generally from anywhere, including this library.

It's great to see that built-in function, I didn't knew it exists! Feel free to approve/merge this MR already, I'll work on using built-in function either way - one doubt I have is handling out-of-bonds coordinates by it, I'll need to check how it behaves.

As for adding draw_polygon() to bitmaptools I'm totally up for it, sounds like a nice starting point for contribting to circuitpython core (-:

I just checked, bitmaptools.draw_line() does not like lines going out of bitmap - I think that it could be a valid scenario, so I may end up trying to change that behaviour in bitmaptools before using it here.

FoamyGuy · 2023-01-08T15:26:45Z

While I'm working on fixing issues above, a screenshot of how it looks in action:

That looks awesome! Thank you for sharing! Do post anywhere online about your project? If you interested it could totally be submitted for inclusion in the weekly python on hardware newsletter.

matemaciek · 2023-01-08T18:53:34Z

That looks awesome! Thank you for sharing! Do post anywhere online about your project? If you interested it could totally be submitted for inclusion in the weekly python on hardware newsletter.

Thanks! I'm definitely up for including my project in the newsletter (-: I think it would be best to do it after I publish code of it, so If someone is interested, they can reuse it. My goal was to build stand-alone CO₂ sensor, so one can know when to open the window and let more air in (just by color-coding values). I ended up adding screen, then some in-memory logging, then simple webserver for better viewing of charts, all living on Pico W (I try to be cost-aware in this project). Without improvements it was running out of memory after ~30 data points, now it seem to be safely running on 160 data points while serving some data online, but I want to fit more (-:

FoamyGuy

Looks good to me. Thank you!

Updating https://github.com/adafruit/Adafruit_CircuitPython_Display_Shapes to 2.6.0 from 2.5.6: > Merge pull request adafruit/Adafruit_CircuitPython_Display_Shapes#59 from matemaciek/main > Add .venv to .gitignore > Update .pylintrc for v2.15.5 > Fix release CI files > Update pylint to 2.15.5 > Updated pylint version to 2.13.0 > Switching to composite actions Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Updated download stats for the libraries

matemaciek added 5 commits December 30, 2022 21:59

Change sparkline to use cyclic buffer

63f9d2f

Since we specify the maximum ammount of data points, we can use cyclic buffer underneath, therefore avoiding memory fragmentation. This should also help for problems decribed in adafruit#25

Add option to draw polylines (not closed polygons)

2f70924

Remove myself from authors

c5774ef

Apparently contributors are not listed there

Update polygon.py

c701027

Update docs

Change Sparkline to use polygon

b99718b

Previous implementation used multiple Line elements, causing unnecessary memory overhead

matemaciek added 12 commits December 31, 2022 00:47

Add docstrings to new methods

81c1cee

Update sparkline.py

f7742d2

Formatting fixes

Remove unused color from palette

3bee680

Fix CI errors

8393bcd

Triangle is a subclass of Polygon, so we need to provide support for more than one color in palette

Fix bug spotted in self-review

3be187b

Use color index constants in triangle

a1a80d8

Refactor polygon.py

2e28750

Decouple polygon drawing from creating bitmap, so the code can be reused for drawing on external bitmaps

Pylint fixes

8fda975

Fix some of the old warnings, reenable some of overrides

Reuse the same bitmap in sparkline

d1e96bf

Add MultiSparkline

064e171

And rewrite Sparkline to be a subclass of MultiSparkline

Fix/override pylint remarks

34e8662

One more pylint remark

fc7c4bd

FoamyGuy requested changes Jan 7, 2023

View reviewed changes

adafruit_display_shapes/multisparkline.py Outdated Show resolved Hide resolved

adafruit_display_shapes/sparkline.py Show resolved Hide resolved

Address review remarks

b2aec7f

matemaciek added 2 commits January 7, 2023 23:53

Move checking if line fits on bitmap to polygon class

fe8779f

Pylint compliance

72ebdc4

matemaciek requested a review from FoamyGuy January 7, 2023 23:04

FoamyGuy reviewed Jan 8, 2023

View reviewed changes

matemaciek requested a review from FoamyGuy January 8, 2023 20:03

FoamyGuy approved these changes Jan 9, 2023

View reviewed changes

FoamyGuy merged commit cf2b173 into adafruit:main Jan 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Sparkline memory improvements #59

Sparkline memory improvements #59

Uh oh!

matemaciek commented Dec 30, 2022

Uh oh!

matemaciek commented Dec 30, 2022

Uh oh!

FoamyGuy left a comment

Uh oh!

Uh oh!

Uh oh!

matemaciek commented Jan 7, 2023

Uh oh!

matemaciek commented Jan 7, 2023

Uh oh!

matemaciek commented Jan 7, 2023

Uh oh!

FoamyGuy left a comment

Uh oh!

FoamyGuy Jan 8, 2023

Uh oh!

matemaciek Jan 8, 2023

Uh oh!

FoamyGuy Jan 8, 2023

Uh oh!

matemaciek Jan 8, 2023

Uh oh!

matemaciek Jan 8, 2023

Uh oh!

FoamyGuy commented Jan 8, 2023

Uh oh!

matemaciek commented Jan 8, 2023

Uh oh!

FoamyGuy left a comment

Uh oh!

Uh oh!

Sparkline memory improvements #59

Sparkline memory improvements #59

Uh oh!

Conversation

matemaciek commented Dec 30, 2022

Uh oh!

matemaciek commented Dec 30, 2022

Uh oh!

FoamyGuy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

matemaciek commented Jan 7, 2023

Uh oh!

matemaciek commented Jan 7, 2023

Uh oh!

matemaciek commented Jan 7, 2023

Uh oh!

FoamyGuy left a comment

Choose a reason for hiding this comment

Uh oh!

FoamyGuy Jan 8, 2023

Choose a reason for hiding this comment

Uh oh!

matemaciek Jan 8, 2023

Choose a reason for hiding this comment

Uh oh!

FoamyGuy Jan 8, 2023

Choose a reason for hiding this comment

Uh oh!

matemaciek Jan 8, 2023

Choose a reason for hiding this comment

Uh oh!

matemaciek Jan 8, 2023

Choose a reason for hiding this comment

Uh oh!

FoamyGuy commented Jan 8, 2023

Uh oh!

matemaciek commented Jan 8, 2023

Uh oh!

FoamyGuy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!