Repository navigation
Conversation
|
Code example used: |
The espressif port had no audiopwmio at all, so boards whose speaker is not wired to a DAC pin had no way to play samples. On the Adafruit FunHouse the piezo is on GPIO42, while the ESP32-S2 DAC is hardwired to GPIO17/GPIO18 (and GPIO18 is the light sensor), so audioio cannot reach it. LEDC has no DMA path for its duty registers, and the S2's I2S has no PDM transmitter (no SOC_I2S_SUPPORTS_PDM_TX), so samples are pushed from a GPTimer alarm ISR running at the sample rate. The ISR drains one of two device buffers; audiopwmout_background(), called from port_background_task(), refills the other from the audiosample protocol. The ISR deliberately touches nothing but its own object's volatile state. It must not queue a background callback: on this port that macro is taskENTER_CRITICAL(), which aborts when called from ISR context. Driving the refill by polling instead also means a buffer the ISR found empty is always picked up again, rather than relying on the ISR to have queued the refill before it stalled. Every gptimer call is checked, so a timer that cannot be configured raises instead of leaving `playing` true forever. alarm_config is static because CircuitPython builds with CONFIG_GPTIMER_CTRL_FUNC_IN_IRAM, under which gptimer_set_alarm_action() rejects a config outside internal RAM. The carrier is APB_CLK / 2**10 = 78.125 kHz, which keeps it out of the audio band while leaving 10 bits of amplitude resolution. The GPTimer runs at 40 MHz (the finest the timer group divider allows), putting the sample-rate error below 0.01% at 8k/11k/22k/44.1k. A single-buffer sample that loops wraps inside the ISR rather than round-tripping through the background task, so looping RawSamples do not click. ledc_ll.h lives in the per-target esp_hal_ledc directory, which was not on the include path, so add it. Enabled for esp32s2 only; the S3 has neither DAC nor audiopwmio and could reuse this later. Costs 2336 bytes of flash and 648 bytes of internal SRAM on adafruit_funhouse, and nothing on boards that leave it off. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gptimer_register_event_callbacks() returned ESP_ERR_INVALID_ARG on the FunHouse, so the alarm callback was never installed: the timer ran, no alarm was ever delivered, nothing consumed samples, and play() left `playing` true forever. The driver is built with CONFIG_GPTIMER_ISR_CACHE_SAFE (selected by CONFIG_GPTIMER_ISR_HANDLER_IN_IRAM, which defaults on), so gptimer_register_event_callbacks() requires the callback in IRAM and its user context in internal RAM. The callback was fine; the user context was not. It was `self`, a MicroPython heap object, and port_malloc() takes MALLOC_CAP_SPIRAM first, so on any board with PSRAM the whole object is external. Clearing that check alone would not be enough. A cache-safe ISR can run while the flash cache is disabled, which happens during any flash write, including the user saving code.py, so everything it dereferences has to be in internal RAM. Follow the pattern common-hal/frequencyio already uses: put the ISR-reachable state in an audiopwmio_internal_data_t allocated with MALLOC_CAP_INTERNAL and pass that as the user context. The LEDC channel numbers are copied in too, so the ISR no longer reaches into the pwmio objects. The audio buffers move to heap_caps_malloc(MALLOC_CAP_ INTERNAL) for the same reason, capped so a large sample fails with ESP_ERR_NO_MEM rather than exhausting internal RAM. The refill path still runs in task context and reaches the object as before. Buffers now hold one value per output channel rather than always a stereo pair, which halves the internal RAM a mono output needs. play() asserts esp_ptr_in_iram() for the callback and esp_ptr_internal() for the control block and both buffers, so a regression surfaces there instead of as a fault at a random later moment. Error paths report the underlying esp_err_t through raise_esp_error() instead of a generic message. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1f6a9dd to
65138d5
Compare
dhalbert
left a comment
dhalbert
left a comment
There was a problem hiding this comment.
Thanks - seems fine in principle. I wonder if it's worth adding to all ESP32-S2, given that it exists to solve a FunHouse problem.
And it would be nice to add to ESP32-S3, but it needs to be tested. However, that could be a later PR.
Could there be glitches due to higher priority tasks like WiFi? Have you tested that?
Willing to merge for the upcoming project immediately and we can fix these later.
| // Write one duty value to a LEDC channel. This mirrors what ledc_update_duty() | ||
| // does, minus the argument checks and the spinlock, so it is cheap enough to | ||
| // run at the sample rate. The fade parameters it relies on (scale 0, so the | ||
| // duty is latched rather than ramped) are left set up by pwmio's construct. |
There was a problem hiding this comment.
What does the absence of the spinlock mean in practice?
There was a problem hiding this comment.
Sorry for the clanker reply, but useful for us all probably:
- Line 60 — “What does the absence of the spinlock mean in practice?”
First, a correction to something I told you earlier. I assumed ledc_update_duty() was ISR-illegal; it isn’t. It uses portENTER_CRITICAL_SAFE and the _ISR arg-check variants, so it’s explicitly ISR-callable.
The actual reason we can’t call it is that it’s in flash. In your build ledc_update_duty links at 0x400f4e38 and ledc_set_duty at 0x400f5150 — both in the flash-mapped text region — while timer_alarm_cb sits at 0x40028c70 in IRAM. CONFIG_LEDC_CTRL_FUNC_IN_IRAM isn’t set. A cache-safe ISR calling into flash faults precisely when the cache is off during a flash write, which is the whole scenario the cache-safe build exists for.
What dropping the spinlock means: ledc_spinlock serialises the entire LEDC register block across tasks and cores. Every register set_duty() touches belongs to one channel — that channel’s DUTY register, and its sig_out_en / duty_start / para_up bits in its own CONF0. We own that channel exclusively: pwmio reserved it at construct, nothing else writes it while the timer runs, and our own stop() only sets duty after stop_timer(). So there’s no register that the ISR and a spinlock-holding task both write.
The honest caveat for your reply: the spinlock gives other LEDC users no protection from us. It’s safe by ownership, not by construction. If a future change made any LEDC op a read-modify-write spanning channels, or someone drove our channel from task context mid-playback, that’s a real race — and on S3 it could be cross-core.
| // Convert one source buffer into device buffer `buf_index`, scaling samples to | ||
| // PWM_BITS. Runs in task context, so it may touch the object freely. |
There was a problem hiding this comment.
Do you mean as opposed to ISR context?
There was a problem hiding this comment.
Yep. Will update the comment.
Clanker:
- Line 121 — “Do you mean as opposed to ISR context?”
Yes, and the comment is too terse to be useful. fill_buffer is reached only via audiopwmout_background() ← port_background_task() ← background_callback_run_all(), i.e. the VM task. So unlike timer_alarm_cb it may dereference self and the sample — both in the MicroPython heap, i.e. PSRAM — and may call audiosample_get_buffer(), which hits the filesystem for a WaveFile and PSRAM DSP buffers for synthio.
Worth rewording to something like: “Runs in the background task, not the ISR, so unlike timer_alarm_cb it may dereference self and the sample, which are not guaranteed to be in internal RAM — the MicroPython heap is PSRAM on boards that have it.”
| } | ||
|
|
||
| // Publish last, so the ISR never sees a half-filled buffer. | ||
| __asm__ volatile ("" ::: "memory"); |
There was a problem hiding this comment.
keep volatile transactions safe, probably worth reworking to be more obvious (volatile vars or the atomic suggestion)
Clanker:
- Line 181 — “Why the asm here?” ← the ASM bit
asm volatile ("" ::: "memory") emits no instructions. The empty string is the asm body; the "memory" clobber is the entire point — it forbids GCC from moving loads/stores across that line.
It’s needed because the next line publishes buffer_ready[buf_index] = (frames > 0). buffer_ready is volatile bool, but the things it publishes — the samples written through dest, plus buffer_frames[buf_index] and last_buffer[buf_index] — are not volatile. C only orders volatile accesses against other volatile accesses; a non-volatile store may legally be sunk past a volatile one. If that happened, the ISR could see buffer_ready == true over a half-written buffer or a stale buffer_frames, and play garbage.
A compiler barrier suffices — no hardware fence — because esp_intr_alloc registers on the calling core and play() runs in the VM task, so ISR and refill are on the same core. S2 is single-core regardless (SOC_CPU_CORES_NUM 1 vs 2 on S3).
Where the criticism lands: it’s blunt and under-documented. More idiomatic options are to make buffer_frames/last_buffer volatile too and lean on volatile-to-volatile ordering, or to use atomic_store_explicit(..., memory_order_release), which states the intent exactly. I’d lean to the atomic — it documents itself and a reviewer wouldn’t have to ask.
| // The GPTimer driver is built cache-safe here, so it requires the callback | ||
| // in IRAM and its user context in internal RAM, and it only checks the | ||
| // latter two. Check all of it, so a regression shows up as an error from | ||
| // play() rather than as a fault at a random later moment. | ||
| if (!esp_ptr_in_iram((const void *)timer_alarm_cb) || | ||
| !esp_ptr_internal(internal_data) || | ||
| !esp_ptr_internal(internal_data->buffers[0]) || |
There was a problem hiding this comment.
Isn't this guaranteed by how the routine and variables are declared and allocated? If not, could it be checked at compile-time? Why check at runtime?
There was a problem hiding this comment.
Checked at compile time possibly yeah (see last highlighted line in penultimate paragraph)
Clanker:
- Line 412 — “Isn’t this guaranteed...? compile-time? why runtime?”
He’s largely right, and this is the one I’d actually change.
timer_alarm_cb in IRAM is guaranteed by IRAM_ATTR placing it in .iram0.text. The runtime check adds nothing.
internal_data and the buffers are guaranteed internal by heap_caps_malloc(..., MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT) — that allocator only draws from heaps carrying those caps, so a non-NULL return is internal, and the NULL check is present.
Compile-time? No. The callback’s address is fixed at link time, so _Static_assert can’t see it (a linker assertion could). The allocations are inherently runtime.
Why it’s there: during bring-up this invariant was violated and the only symptom was ESP_ERR_INVALID_ARG from gptimer_register_event_callbacks, with no indication which of the pointers was wrong. Note the driver checks only two of the three — it does not check the sample buffers, and those are the ones whose failure mode is a fault at a random later moment during a flash write, nowhere near the cause.
Defensible options: drop it entirely; keep only the buffer checks, which nothing else covers; or make it assert() so it compiles out of release builds. I’d keep the buffer checks with a comment, but dropping it is reasonable and his instinct is sound.
Want me to draft the actual reply text for any of these, or go deeper on one?
do you mean PSRAM for #2, because teh s3 can come without
Good catch — that’s sloppy wording on my part, and it has a consequence I under-sold. Let me re-read the allocator.
Yes — and you’ve caught a real error, not just loose wording. Here’s the exact behaviour:
void *ptr = NULL;
#ifdef CONFIG_SPIRAM
ptr = heap_caps_malloc(size, MALLOC_CAP_8BIT | MALLOC_CAP_SPIRAM);
#endif
if (ptr == NULL) {
ptr = heap_caps_malloc(size, MALLOC_CAP_8BIT);
}
So the PSRAM attempt is compiled out entirely when CONFIG_SPIRAM is undefined. That gives three cases, not one:
No-PSRAM board → the MicroPython heap is always internal RAM.
PSRAM board → PSRAM normally…
…except any allocation made once PSRAM is exhausted, which falls back to internal.
So it’s not even deterministic per board — it’s per allocation.
Corrected wording for comment #2: not “which live in PSRAM” but “which are not guaranteed to be in internal RAM — the MicroPython heap is PSRAM on boards that have it.”
The consequence I under-sold: on a board without PSRAM, the original self-as-user_data version would have worked fine. esp_ptr_internal(self) would have returned true, gptimer_register_event_callbacks would have accepted it, and the ISR would have been dereferencing internal RAM all along. The bug only ever existed on PSRAM boards — your FunHouse has 2 MB, which is why it bit you.
That changes my advice on comment #4. I told you the runtime check was redundant and defensible to drop. The first half still holds for the code as written — heap_caps_malloc(..., MALLOC_CAP_INTERNAL) is deterministic, so the buffers and control block are always internal. But the reason to keep a guard is stronger than I made out: a future refactor that let a heap pointer back into the ISR path would pass on every no-PSRAM S2/S3 board, and pass intermittently on PSRAM boards depending on allocation order. That’s precisely the class of bug that survives CI and single-board testing. I’d now argue for keeping it (or as assert()), and that’s a better answer to dhalbert than “it’s redundant.”
It also bears on the two enablement questions. The ISR-RAM constraint only bites on PSRAM boards, so mikeysklar’s S3 Feather test (2 MB PSRAM) was actually the informative configuration — a no-PSRAM S3 would have proven much less. Worth saying explicitly if you reply to his offer to enable S3, since “it worked on my S3” is weaker evidence on a board without PSRAM.
|
Tested on an ESP32-S3 Feather (4MB flash, 2MB PSRAM) with a MAX98306 amp, built from this branch with 
The nvm slowdown looks like background refill starvation. Each nvm write blocks for about 140 ms, so the ISR runs out of data and holds the quiescent level. A single-buffer looping sample is unaffected. Probably the same for any background-refilled audio, so not a blocker. No source changes were needed for the S3 beyond turning the module on, so it could be enabled here: #### esp32s3 ##########################################################
else ifeq ($(IDF_TARGET),esp32s3)
@@
# No DAC
CIRCUITPY_AUDIOIO = 0
+# LEDC + GPTimer PWM audio.
+CIRCUITPY_AUDIOPWMIO ?= 1
+
endifThe comment further down that says "The S3 has no AUDIOIO and no AUDIOPWMIO" would need updating too. Happy to push that as a follow-up PR instead if you'd rather keep this one S2 only. WiFi scan test (any format 0 melody.mid)import array
import math
import time
import audiocore
import audiopwmio
import board
import synthio
import wifi
MIDI_FILE = "/melody.mid"
dac = audiopwmio.PWMAudioOut(board.A0)
length = 8000 // 440
sine_wave = array.array("H", [0] * length)
for i in range(length):
sine_wave[i] = int(math.sin(math.pi * 2 * i / length) * (2**15 - 1) + 2**15)
tone = audiocore.RawSample(sine_wave, sample_rate=8000)
def scan_for(seconds, player=None):
scans = nets = 0
end = time.monotonic() + seconds
while time.monotonic() < end and (player is None or player.playing):
for n in wifi.radio.start_scanning_networks():
nets += 1
wifi.radio.stop_scanning_networks()
scans += 1
return scans, nets
wifi.radio.enabled = True
print("1. tone, radio idle")
dac.play(tone, loop=True)
time.sleep(8)
dac.stop()
time.sleep(1)
print("2. tone, scanning")
dac.play(tone, loop=True)
t = time.monotonic()
print(" scans, networks seen:", scan_for(8), "in", round(time.monotonic() - t, 1), "s")
dac.stop()
time.sleep(1)
print("3. midi, scanning")
WAVE_LENGTH = 128
waveform = array.array("h", [0] * WAVE_LENGTH)
for i in range(WAVE_LENGTH):
waveform[i] = int(math.sin(math.pi * 2 * i / WAVE_LENGTH) * (2**15 - 1))
envelope = synthio.Envelope(attack_time=0.03, decay_time=0.05, release_time=0.2,
attack_level=1.0, sustain_level=0.8)
midi = synthio.from_file(open(MIDI_FILE, "rb"), sample_rate=11025,
waveform=waveform, envelope=envelope)
dac.play(midi)
t = time.monotonic()
print(" scans, networks seen:", scan_for(30, dac))
while dac.playing:
pass
print(" midi took", round(time.monotonic() - t, 2), "s (6.8 expected), error_location:", midi.error_location)
midi.deinit()
dac.deinit()
print("done") |
No need to merge immediately, no specific project (was just guilty of pointing someone at the possibility of synthio and midi on FunHouse - they hand transcribed toccata instead), although it is cool and very suitable for halloween shenanigans. I'll have to get back to you about some of the code decisions / questions, the other smaller PRs were clear cut, whereas this one was a bit more extensive.. |
|
I was also curious about the priority stuff, as there was one thing we altered so it didn't starve, but as a user I wondered about asking for priority for tasks etc. |
|
@tyeth - The ESP32-S3 should be a non-issue in terms of keeping a smooth audio stream with WiFi and flash writes. WiFi is pinned to core 0 with the VM pinned to core 1. The ESP32-S2 is more exposed with only a single core. The user work around would be larger buffers to try and avoid audio degradation. |
|
@mikeysklar I think this would be cool on the S3, maybe it should be enabled as an option, but it also has the i2s/PDM stuff that could work: Clanker Response for more details
The real version of your idea, on S3 SOC_I2S_SUPPORTS_PDM_TX is 1 on ESP32-S3 and absent on S2. So on S3 you could feed I2S PDM TX from GDMA — no per-sample ISR at all, the CPU only tops up DMA descriptors, and DMA buffers can be much larger. That is precisely “free up more time between buffer fills, operating from a larger buffer,” and it’s the architecturally right answer where the hardware offers it. It’d be a separate driver rather than a tweak to this one, and it’s S3-only — which cuts against mikeysklar’s “just enable the LEDC one on S3” diff, or rather suggests LEDC is the portable fallback and PDM is the good path on chips that have it. Worth noting that neither S2 nor S3 has ETM (SOC_ETM_SUPPORTED is absent on both), so there’s no event-matrix shortcut on either. Ignore this unless you're a glutton for verbiage (mentions priority talk): Review feedback before agent read the comments 🤦
The ISR never refills. That split isn't stylistic — it's forced. Why the internal-RAM block exists. Point 1 — all S2, or FunHouse only?The change is one line in the esp32s2 block:
Point 2 — ESP32-S3Stronger case than S2, and I checked the portability claim rather than assuming:
So it's the config line plus the now-stale comment at Point 3 — WiFi glitchesTwo separate things, and conflating them is what makes this question sound scarier than it is. Sample output timing is immune to task priority. The alarm is a hardware interrupt; it preempts every FreeRTOS task including WiFi's. WiFi cannot make the audio jitter or run slow. Buffer refill is not immune. It runs in What an underrun does: the ISR writes the quiescent value and returns without advancing Why nvm is worse than WiFi: a flash write blocks the VM ~140 ms with the cache off. WiFi tasks preempt the VM but yield frequently, so refills still land — hence his clean continuous-scan result. Why priority can't fix it (this is the answer to your own question, and I think it's the interesting one): the refill cannot run during a flash write at all. The lever that does exist is buffer depth: Single-buffer looping samples are immune by design — the ISR wraps in place ( On ISR priority specifically: One thing to be straight about in your reply: you haven't tested WiFi; mikeysklar has. Scans clean, nvm writes stretch. |
|
Agreed on both. S3 LEDC as an opt-in follow-up makes sense. It ran unchanged on my Feather ESP32-S3 + MAX98306. I agree PDM TX is the better S3 path as a separate driver, with LEDC as the portable fallback. Probably need @dhalbert to agree on that too. One correction to the review in your second fold:
With 10 trials each on the S3, 6.73 s MIDI track:
Steady WiFi traffic doesn't affect it on S3. nvm is the real problem, so deeper buffers or PDM's larger DMA buffers would help both. |
The espressif port has no
audiopwmio, so a board whose speaker is not wired to a DAC pin has no way to play samples at all. On the Adafruit FunHouse the piezo is on GPIO42, while the ESP32-S2 DAC is hardwired to GPIO17/GPIO18 — and GPIO18 is the ambient light sensor — soaudioiocannot reach it.Mechanism
LEDC plus a GPTimer alarm ISR running at the sample rate. The alternatives were checked against the in-tree IDF rather than assumed:
soc/esp32s2/include/soc/soc_caps.hhas noSOC_I2S_SUPPORTS_PDM_TX; PDM TX arrives on the S3.The alarm callback writes the duty registers through
ledc_ll_*, draining one of two buffers while a background callback refills the other from the audiosample protocol — the same shape the port'saudioiouses for its DMA refills.Carrier is
APB_CLK / 2**10= 78.125 kHz with 10 bits of duty, the same resolution choice the RP2040 port makes. The GPTimer runs at 40 MHz, the finest the timer-group divider allows, putting sample-rate error under 0.01% (8000 exact, 22050 -> 22050.7, 44100 -> 44101.4 Hz).ISR state must be in internal RAM
Worth highlighting for review, because it is not obvious and it is why a first attempt failed on hardware: CircuitPython builds with
CONFIG_GPTIMER_ISR_CACHE_SAFE, sogptimer_register_event_callbacks()requires the callback to be in IRAM anduser_datato be in internal RAM. Passingselffails withESP_ERR_INVALID_ARG, becauseport_malloc()takesMALLOC_CAP_SPIRAMfirst and the whole MicroPython heap is external on a PSRAM board.So, following
common-hal/frequencyio, everything the ISR dereferences lives in aMALLOC_CAP_INTERNALcontrol block passed asuser_data— including the sample buffers, which would otherwise come from that same PSRAM heap and fault whenever the cache is disabled during a flash write.play()assertsesp_ptr_in_iram()on the callback andesp_ptr_internal()on the block and both buffers, so a regression surfaces there rather than as a random crash.Verification
Confirmed on an Adafruit FunHouse: a 440 Hz
RawSampletest tone plays, and asynthioMIDI track plays throughPWMAudioOuton the onboard piezo to completion. Tested on that one board only.On a piezo specifically, expect a thin, bass-free sound — it is a resonant capacitive element driven straight off a GPIO with no amplifier. Note also that a 78 kHz carrier draws appreciably more current into that capacitance than the 1-4 kHz tones
pwmionormally produces; raisingPWM_BITSfrom 10 to 11 halves it at a 39 kHz carrier.Size
adafruit_funhouse,CIRCUITPY_AUDIOPWMIO0 vs 1:Scoped to esp32s2; boards leaving it off pay nothing. The S3 has neither DAC nor audiopwmio and could reuse this.
🤖 Generated with Claude Code
-- Edit -- The uf2 with example installable 11-alpha2 + this PR #11663 on top of this PR stack #11662.
This pr has now revised onto main as the synthio PRs merged.
funhouse_halloween_audiopwmio.zip
From tyeth#39 plus a melody.mid file.
Shows synthio playing a polyphonic midi file on the pwm "speaker" pin via audiopwmio, potentially allowing mixing and control too etc.