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

Rework the coproc API #7359

Merged
merged 4 commits into from
Dec 21, 2022
Merged

Rework the coproc API #7359

merged 4 commits into from
Dec 21, 2022

Conversation

tannewt
Copy link
Member

@tannewt tannewt commented Dec 19, 2022

It is now a generic memorymap API and an ESP specific espulp module.

Fixes #7218. Fixes #3234. Fixes #7300.

It is now a generic `memorymap` API and an ESP specific `espulp` module.

Fixes micropython#7218. Fixes micropython#3234. Fixes micropython#7300.
@tannewt
Copy link
Member Author

tannewt commented Dec 19, 2022

The ULP code to test is here: https://github.com/adafruit/Adafruit_CircuitPython_ULP_Blink

Here is a pre-compiled version along with the code.py's for them. (The blink examples compile .c to a .py wrapper library.)
Archive.zip

The libraries should work on S2 with sx_version=2 but I haven't tested it. They shouldn't be board specific because the pins are dynamically patched into the binary.

I plan on polishing this compilation tooling when I'm back from ☃️🎄.

@tannewt tannewt requested a review from dhalbert December 20, 2022 03:20
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.

I thought I did a review this afternoon, but my comments are still pending. Haven't tested yet, but here are some thoughts.

ports/espressif/bindings/espulp/ULP.c Outdated Show resolved Hide resolved
ports/espressif/bindings/espulp/ULP.c Show resolved Hide resolved
ports/espressif/common-hal/espulp/ULP.c Outdated Show resolved Hide resolved
shared-bindings/memorymap/AddressRange.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@microdev1 microdev1 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for working on this, the API looks more polished now. Just a couple suggestions.

Comment on lines 65 to 76
else ifeq ($(IDF_TARGET),esp32s2)
# Modules
CIRCUITPY_BLEIO = 0
CIRCUITPY_ESPULP = 1
CIRCUITPY_MEMORYMAP = 1

else ifeq ($(IDF_TARGET),esp32s3)
# Modules
CIRCUITPY_PARALLELDISPLAY = 0
CIRCUITPY_ESPULP = 1
CIRCUITPY_MEMORYMAP = 1
endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

I refactored mpconfigport.mk recently and set the following convention:

# Module on by default and conditionally turned off

CIRCUITPY_ESPULP ?= 1

ifeq ($(IDF_TARGET),esp32)
CIRCUITPY_ESPULP = 0
CIRCUITPY_MEMORYMAP = 0

else ifeq ($(IDF_TARGET),esp32c3)
CIRCUITPY_ESPULP = 0
CIRCUITPY_MEMORYMAP = 0

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd prefer to do the opposite so that future chips have it off by default.

Comment on lines +110 to +113
if (ulp_used) {
mp_raise_ValueError_varg(translate("%q in use"), MP_QSTR_ULP);
}
self->inited = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might want to infer the ULP state from registers.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was deliberate because the ULP might already be running when the object is created.

@tannewt tannewt requested a review from dhalbert December 20, 2022 18:56
@dhalbert dhalbert dismissed their stale review December 21, 2022 01:41

addressed

@dhalbert
Copy link
Collaborator

@microdev1 After discussion, we are going to merge as is for beta.6 and vacation scheduling reasons, but happy to address further things, probably after the new year.

@dhalbert dhalbert merged commit 2a1bb72 into adafruit:main Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants