8000 Enable collections deque for raspberrypi by rtyley · Pull Request #6474 · adafruit/circuitpython · GitHub
[go: up one dir, main page]
Skip to content

Enable collections deque for raspberrypi #6474

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 12, 2022
Merged

Enable collections deque for raspberrypi #6474

merged 2 commits into from
Jun 12, 2022

Conversation

rtyley
Copy link
@rtyley rtyley commented Jun 10, 2022

Hi, I'd like to use collections.deque on my RP2040-based Keybow 2040. For MicroPython, collections.deque is enabled for all rp2 devices, because they have MICROPY_CONFIG_ROM_LEVEL set to 'extra features':

#define MICROPY_CONFIG_ROM_LEVEL                (MICROPY_CONFIG_ROM_LEVEL_EXTRA_FEATURES)

...which includes MICROPY_PY_COLLECTIONS_DEQUE.

For CircuitPython, it looks like MICROPY_CONFIG_ROM_LEVEL defaults to 'core' (MICROPY_CONFIG_ROM_LEVEL_CORE_FEATURES) and isn't updated against any of the ports, so the only port getting collections.deque is the unix port, which explcitly sets MICROPY_PY_COLLECTIONS_DEQUE:

circuitpython/ports/unix/mpconfigport.h

Line 134 in 6925a00

#define MICROPY_PY_COLLECTIONS_DEQUE (1)

This commit just enables MICROPY_PY_COLLECTIONS_DEQUE in the same manner for the raspberrypi port.

See also:

@dhalbert
Copy link
Collaborator

In general, we try not to enable language features for just one port, because then we can't write port-independent code. So we would like to turn this for as many ports as possible, assuming there's room, rather than just RP2040. So let's try turning it on if CIRCUITPY_FULL_BUILD is true.

rtyley added a commit to rtyley/circuitpython that referenced this pull request Jun 10, 2022
@rtyley
Enable collections deque for CIRCUITPY_FULL_BUILD
6269bbd 
I'd like to use `collections.deque`:

https://docs.circuitpython.org/en/latest/docs/library/collections.html#collections.deque

...on my RP2040-based Keybow 2040 (https://circuitpython.org/board/pimoroni_keybow2040/).

For MicroPython, `collections.deque` is enabled for all `rp2` devices,
because they all have `MICROPY_CONFIG_ROM_LEVEL` set to 'extra features':

```
```
https://github.com/micropython/micropython/blob/cf7d962cf38db296d1ac419fc4d5302b64c59644/ports/rp2/mpconfigport.h#L44

...which includes `MICROPY_PY_COLLECTIONS_DEQUE` (see
https://github.com/micropython/micropython/blob/6bda80d81147217a1d830b99b93d2e35d372e8f9/py/mpconfig.h#L1225-L1227 ).

For CircuitPython, it looks like `MICROPY_CONFIG_ROM_LEVEL` defaults to
'core' (`MICROPY_CONFIG_ROM_LEVEL_CORE_FEATURES`) and isn't updated
against any of the ports, so the only port getting `collections.deque`
is the `unix` port, which explcitly sets `MICROPY_PY_COLLECTIONS_DEQUE`:

https://github.com/adafruit/circuitpython/blob/6925a001382d41940ea9b412b1d1ba517d9880f9/ports/unix/mpconfigport.h#L134

At Dan Halbert's suggestion...

adafruit#6474 (comment)

... this commit enables `MICROPY_PY_COLLECTIONS_DEQUE` for all builds where
`CIRCUITPY_FULL_BUILD` is true - which includes Raspberry Pi:

https://github.com/adafruit/circuitpython/blob/6925a001382d41940ea9b412b1d1ba517d9880f9/ports/raspberrypi/mpconfigport.mk#L11

See also:

* adafruit#5734
* micropython@970eedc
  which originally added collections.deque to MicroPython
@rtyley rtyley force-pushed the enable-collections-deque-for-raspberrypi branch from cc68d3d to 6269bbd Compare June 10, 2022 14:09
@Neradoc
Copy link
Neradoc commented Jun 10, 2022

I tried a local implementation with just this in py/circuitpy_mpconfig.mk

MICROPY_PY_COLLECTIONS_DEQUE ?= $(CIRCUITPY_FULL_BUILD)
CFLAGS += -DMICROPY_PY_COLLECTIONS_DEQUE=$(MICROPY_PY_COLLECTIONS_DEQUE)

@rtyley rtyley force-pushed the enable-collections-deque-for-raspberrypi branch from 6269bbd to 7f9ff9c Compare June 10, 2022 14:26
@rtyley rtyley closed this Jun 10, 2022
@rtyley rtyley force-pushed the enable-collections-deque-for-raspberrypi branch from 7f9ff9c to 6925a00 Compare June 10, 2022 14:26
@rtyley
Enable collections deque for CIRCUITPY_FULL_BUILD
09091ec 
I'd like to use `collections.deque`:

https://docs.circuitpython.org/en/latest/docs/library/collections.html#collections.deque

...on my RP2040-based Keybow 2040 (https://circuitpython.org/board/pimoroni_keybow2040/).

For MicroPython, `collections.deque` is enabled for all `rp2` devices,
because they all have `MICROPY_CONFIG_ROM_LEVEL` set to 'extra features':

https://github.com/micropython/micropython/blob/cf7d962cf38db296d1ac419fc4d5302b64c59644/ports/rp2/mpconfigport.h#L44

...which includes `MICROPY_PY_COLLECTIONS_DEQUE` (see
https://github.com/micropython/micropython/blob/6bda80d81147217a1d830b99b93d2e35d372e8f9/py/mpconfig.h#L1225-L1227 ).

For CircuitPython, it looks like `MICROPY_CONFIG_ROM_LEVEL` defaults to
'core' (`MICROPY_CONFIG_ROM_LEVEL_CORE_FEATURES`) and isn't updated
against any of the ports, so the only port getting `collections.deque`
is the `unix` port, which explcitly sets `MICROPY_PY_COLLECTIONS_DEQUE`:

https://github.com/adafruit/circuitpython/blob/6925a001382d41940ea9b412b1d1ba517d9880f9/ports/unix/mpconfigport.h#L134

At Dan Halbert's suggestion...

adafruit#6474 (comment)

... this commit enables `MICROPY_PY_COLLECTIONS_DEQUE` for all builds where
`CIRCUITPY_FULL_BUILD` is true - which includes Raspberry Pi:

https://github.com/adafruit/circuitpython/blob/6925a001382d41940ea9b412b1d1ba517d9880f9/ports/raspberrypi/mpconfigport.mk#L11

See also:

* adafruit#5734
* micropython@970eedc
  which originally added collections.deque to MicroPython
@dhalbert
Copy link
Collaborator

@rtyley did you mean to close this? I reopened.

@dhalbert dhalbert reopened this Jun 10, 2022
@rtyley
Copy link
Author
rtyley commented Jun 10, 2022

did you mean to close this? I reopened.

I didn't mean to close it! I managed to make a mess with git, and accidentally pushed the branch when it had only an empty merge commit - so GitHub decided that the the PR was effectively merged, and closed it!!

I've now fixed the git branch, and updated as per your comment, just setting MICROPY_PY_COLLECTIONS_DEQUE if CIRCUITPY_FULL_BUILD is true, as is already done for MICROPY_PY_COLLECTIONS_ORDEREDDICT.

@rtyley
Copy link
Author
rtyley commented Jun 10, 2022

Builds for bluemicro833, feather_m0_express, kicksat-sprite, pca10100, simmel, metro_m0_express failed due to Too little flash!!! warnings, mostly just for one or two locales (ru, fr):

Error: Build bluemicro833 for ru took 30.86s and failed
make: Entering directory '/home/runner/work/circuitpython/circuitpython/ports/nrf'
Use make V=1, make V=2 or set BUILD_VERBOSE similarly in your environment to increase build verbosity.
237676 bytes used, -108 bytes free in flash firmware space out of 237568 bytes (232.0kB).
50896 bytes used, 80176 bytes free in ram for stack and heap out of 131072 bytes (128.0kB).
Too little flash!!!

I don't know if there's a convenient flag distinguishing between those boards and larger boards like the raspberrypi?

@dhalbert
Copy link
Collaborator

I can make some decisions about which builds to remove deque from, so that things fit. I'll push some commits to your PR that should fix. I may not do that immediately, but it will be on my short-term to-do list.

@dhalbert dhalbert self-requested a review June 10, 2022 16:15
@dhalbert dhalbert self-assigned this Jun 10, 2022
@rtyley
Copy link
Author
rtyley commented Jun 10, 2022

Many thanks @dhalbert !

dhalbert
dhalbert approved these changes Jun 12, 2022
View reviewed changes
Copy link
Collaborator
@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I turned deque off on SAMD21 builds, and made it fit on a few nRF52833 builds that were too tight by turning off some other things that were generally not needed for those boards.

@dhalbert dhalbert merged commit 7579791 into adafruit:main Jun 12, 2022
@rtyley rtyley deleted the enable-collections-deque-for-raspberrypi branch June 12, 2022 07:16
@dhalbert dhalbert mentioned this pull request Oct 16, 2022
Closed
@dhalbert dhalbert mentioned this pull request Feb 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhalbert dhalbert dhalbert approved these changes

Assignees

@dhalbert dhalbert

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rtyley @dhalbert @Neradoc
0