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

Fix "espulp module broken in CircuitPython 9" #9277

Merged
merged 12 commits into from
Jun 3, 2024

Conversation

Sola85
Copy link

@Sola85 Sola85 commented May 26, 2024

This pull request fixes Issue #8770 as well as a few more minor issues with the existing espulp implementation. The changes/issues addressed are:

  • Enable FSM-ULP and RISCV-ULP simultaneously at compile-time (IDFGH-11916) espressif/esp-idf#12999 is fixed in adafruits fork of esp-idf. This allows both RISCV and FSM ULPs to be enabled again for esp's that have both.
  • RISCV ULP is now disabled for all boards, as discussed in Issue espulp module broken in CircuitPython 9 #8770. This was necessary because with IDF Version 5.0, espressif only allows one type of ULP to be active at compile time, as documented here. This meant that some boards had access to only the FSM, while others had access to only the RISCV ULP.
  • The wakeup period of the ULP was previously hard-coded. This is now exposed as a method on the ULP object.
  • The entrypoint to the ulp binary was previously hard-coded to 0 for the FSM ULP. This meant that programs that had any data variables before the main code could not be run. To fix this, the entrypoint can now be specified in the ulp.run() method.
  • If any pins are provided to the ulp.run() function, then ESP_PD_DOMAIN_RTC_PERIPH is now configured to keep gpio powered during deep-sleep, such that the ulp can read/write gpio during deep sleep of the main cpu.
  • The ulp.run() method checks that the ulp is not already running. But since the ulp.halt() method was not implemented for the FSM, this meant that ulp.run() could only be called a second time after a power cycle or a hard-reset, because the ulp keeps running independent of the state of circuitpython. This is fixed by implementing the ulp.halt() method.

Summary of the status of ULP support for all variants of the ESP32 that are currently supported by circuitpython:

Variant Before this PR After this PR
esp32 FSM working (i think) FSM working
esp32s2/s3 FSM implemented but broken
RISCV working
FSM working
RISCV working
esp32c6 RISCV LP not implemented RISCV LP not implemented
esp32c2/c3/h2 - (no ulp available) - (no ulp available)

Note: The RISCV-LP ulp of the esp32c6 was previously not implemented and is still not implemented since I do not own an esp32c6 and this is therefore outside the scope of this PR.

Here are two examples for using the ULPs to increment a variable (that now work after this PR).
ULP FSM (for esp32/esp32s2/esp32s3):

import espulp
import memorymap
from time import sleep

# for esp32s2/s3
binary = b'ulp\x00\x0c\x00\x18\x00\x00\x00\x00\x00{\x00\x00\x00\x03\x00\x80t\x0e\x00\x00\xd0\x1a\x00\x00t\x8e\x01\x00h\x00\x00\x00\xb0'
# for esp32
# binary = b'ulp\x00\x0c\x00\x18\x00\x00\x00\x00\x00\x00\x00\x00\x00\x03\x00\x80r\x0e\x00\x00\xd0\x1a\x00\x00r\x0e\x00\x00h\x00\x00\x00\xb0'

ulp = espulp.ULP()
ulp.halt()
ulp.set_wakeup_period(0, 1000000) # 1 million microseconds = wake up once per second
ulp.run(binary, entrypoint=4)
data_variable = memorymap.AddressRange(start=0x50000000, length=2)

while True:
    print(int.from_bytes(data_variable[:], "little"))
    sleep(1)

ULP RISCV (for esp32s2/esp32s3):

import espulp
import memorymap
import time

binary = b"\x6f\x00\xe0\x01\x13\x00\x00\x00\x13\x00\x00\x00\x13\x00\x00\x00\x82\x80\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x17\x11\x00\x00\x13\x01\x21\xfe\x19\x28\x19\x20\x1d\x20\x01\xa0\x83\x27\x40\x07\x01\x45\x85\x07\x23\x2a\xf0\x06\x82\x80\xa1\x67\x93\x87\x47\x10\x98\x43\xb7\x06\xc0\xfd\xfd\x16\x75\x8f\x98\xc3\x82\x80\xa1\x67\x93\x87\x47\x10\x98\x43\xb7\x46\xc0\xff\xfd\x16\x75\x8f\xb7\xc6\x0f\x00\x55\x8f\x98\xc3\x98\x43\xb7\x06\x40\x02\x55\x8f\x98\xc3\x01\xa0"

variable = memorymap.AddressRange(start=0x50000074, length=4)
ulp = espulp.ULP(espulp.Architecture.RISCV)
ulp.halt()
ulp.set_wakeup_period(0, 1000000) # 1 million microseconds = wake up once per second
ulp.run(binary)

while True:
    time.sleep(1)
    print("Value from RISC-V ULP:", int.from_bytes(variable[:], "little"))

Copy link

@bill88t bill88t left a comment

Choose a reason for hiding this comment

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

PR looks good, module looks amazing even by my cranky code standards.
Looks like a lovely excuse for me to give ULP a try.

jepler
jepler previously requested changes May 26, 2024
Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

I would like to discuss further which type of ULP should be available. My gut was that riscv would be more useful, because it can be targeted with a gcc toolchain.

@Sola85
Copy link
Author

Sola85 commented May 26, 2024

I would like to discuss further which type of ULP should be available. My gut was that riscv would be more useful, because it can be targeted with a gcc toolchain.

I would certainly love if the RISCV ULP were more accessible as it is definitely the more capable of the two ulps, and I do hope that espressif addresses the underlying issue preventing us from using both.
My motivation for going with FSM in this PR was two-fold:

  1. It is available on more boards (RISCV is only available on esp32s2/S3 afaik), so the experience is more consistent.
  2. And this was my main motivation: The python based assembler for FSM mentioned above makes it quite easy to get your feet wet with the ULP, since you can embed human readable ULP source code directly within python code. In my opinion this makes it match the python experience best. It's precisely the fact that you need a gcc toolchain that makes the RISCV ULP less appealing (at least in the python world), as you need to switch from writing on-device python, go to a separate pc with the toolchain installed, compile the ULP binary, and then hard code this binary in the python code.
    At least this is true until we have a python implementation of the RISCV assembler, or some other python API for generating riscv ulp binaries (like for the rp2040 pio).

@bill88t
Copy link

bill88t commented May 26, 2024

Risquev would be a lot more useful since it can be targetted by gcc, but it would be a lot less intuitive for a beginner to work with.
Plus, it's probably gonna be a nightmare on windows.
(I haven't touched windows since 2018, so I can only guess)

The ULP assembly seems very friendly and simple from a quick read.
That can be a selling point for education imo.
Also, on the fly recompilation with other data (.format() configurable variables)?

Plus as Sola85 stated, og esp32 is a pretty big deal.

@tannewt
Copy link
Member

tannewt commented May 29, 2024

C5, C6, C61 and P4 all have low power RISC-V cores. (Defined as SOC_LP_CORE_SUPPORTED in ESP-IDF.) So, I'd vote for FSM on ESP32 because that's the only option. On S2 and S3, I'd go RISC-V because it is the future and improving tooling for it is more worth while. (A RISC-V assembler would be awesome IMO.)

My hope was to use clang for basic RISC-V code because clang is cross-target by default. My prototype of it was here: https://github.com/adafruit/Adafruit_CircuitPython_ULP_Blink The idea is that the build system runs the compiler and wraps the resulting code in a Python library.

@tannewt
Copy link
Member

tannewt commented May 29, 2024

Thanks for fixing this up @Sola85 !

@Sola85
Copy link
Author

Sola85 commented May 29, 2024

Edit: This comment is no longer relevant. See the new comment below.
Thanks for the comments. I didn't know that more ESP's are coming that only have the RISCV ulp. I've made another suggestion for how we can keep both FSM and RISCV ulp in a separate PR.

The new PR only deviates from this one by one commit.

@Sola85
Copy link
Author

Sola85 commented May 30, 2024

I just learned that circuitpython maintains its own fork of esp-idf. I have submitted a PR adafruit/esp-idf#16 to that repo which would fix the underlying issue preventing us from using both ULPs.

Should that PR get merged, we can enable both ULPs in this PR, by simply adding CONFIG_ULP_COPROC_TYPE_RISCV=y to sdkconfig-esp32s2.defaults and sdkconfig-esp32s3.defaults

@Sola85
Copy link
Author

Sola85 commented May 31, 2024

The PR to adafruit/esp-idf mentioned above got merged. I therefore pulled these changes and enabled the RISCV ulp again on supporting boards. See also the updated description of this PR above.

@Sola85 Sola85 requested a review from jepler May 31, 2024 09:15
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thank you for getting this going! Just a couple comments about error checking. Generally we don't want to add new messages unless they are very specific.

ports/espressif/common-hal/espulp/ULP.c Outdated Show resolved Hide resolved
ports/espressif/common-hal/espulp/ULP.c Show resolved Hide resolved
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!

@tannewt tannewt dismissed jepler’s stale review June 3, 2024 17:21

Both ULP archs are supported.

@tannewt tannewt merged commit dc3edf4 into adafruit:main Jun 3, 2024
514 checks passed
@dhalbert dhalbert linked an issue Jun 3, 2024 that may be closed by this pull request
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

Successfully merging this pull request may close these issues.

espulp module broken in CircuitPython 9
4 participants