adafruit / Adafruit_CircuitPython_CLUE

A high-level library representing all the features of the Adafruit CLUE
MIT License
32 stars 20 forks source link

Remove max_size parameter #45

Closed lesamouraipourpre closed 3 years ago

lesamouraipourpre commented 3 years ago

Remove the max_size parameter. It is no longer used by displayio.Group Ref: adafruit/circuitpython#4959

I don't have a CLUE to test with.

jposada202020 commented 3 years ago

@lesamouraipourpre I tested this in

Adafruit CircuitPython 6.3.0 on 2021-06-01; Adafruit CLUE nRF52840 Express with nRF52840

I test the clue_ble_color_patchwork.py

Results with max_size

>>> import clue_ble_color_patchwork
scanning
after scan found 1 results
0
15597568

Results WITHOUT max_size

Adafruit CircuitPython 6.3.0 on 2021-06-01; Adafruit CLUE nRF52840 Express with nRF52840
>>> import clue_ble_color_patchwork
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "clue_ble_color_patchwork.py", line 146, in <module>
  File "/lib/adafruit_display_shapes/rect.py", line 47, in __init__
MemoryError: memory allocation failed, allocating 7680 bytes
>>> 

I remember that we were waiting for this change, so not sure what is the consequences memory wise in the core.

lesamouraipourpre commented 3 years ago

As far as I can see once it hits the C code below it is simply ignored, so removing it should have no effect. https://github.com/adafruit/circuitpython/blob/c37f354d2d9faca7bb827c775335421ffa98bf33/shared-bindings/displayio/Group.c#L38-L72

Did you use the .PY version of adafruit_clue.py or a .MPY version? That's the only thing I can think of that could cause a memory overflow.

jposada202020 commented 3 years ago

I used the .py version in both cases, just wondering why this will increase the memory usage

lesamouraipourpre commented 3 years ago

The .py contains all the comments and is loaded into memory, so depending on the code can be significantly bigger. That's before any of the compression that .mpy applies.

jposada202020 commented 3 years ago

Yes, I am aware of that, but I use the py in both cases, maybe something as put the big rock first with the changes. If you agree I will test with the .mpy files and if so, then I could do a different PR to add the note in the example that the . mpy needs to be used in order for the example to work? does this work for you?

lesamouraipourpre commented 3 years ago

I'm intrigued as to why this is happening then as there should be a very marginal space saving of a byte or two.

If the .mpy version runs and has the same or better free memory while running I would say merge it and go with your suggestion.

jposada202020 commented 3 years ago

yup me too. will do!

jposada202020 commented 3 years ago

I have replaced the following libraries for the mpy version and it worked Bus_device Register lsm6ds apds9960