Skip to content

JS audio.tone(): make the non-blocking flag work - #2965

Open
p1oner15 wants to merge 1 commit into
BruceDevices:devfrom
p1oner15:fix/js-tone-nonblocking
Open

p1oner15 wants to merge 1 commit into
BruceDevices:devfrom
p1oner15:fix/js-tone-nonblocking

Conversation

@p1oner15

Copy link
Copy Markdown

Proposed Changes

audio.tone(freq, ms, nonBlocking) in JS always blocks:

int nonBlocking = false;
if (argc > 2) { nonBlocking = JS_ToInt32(ctx, &nonBlocking, argv[2]); }

JS_ToInt32 returns a status code (0 on success), so the flag is always 0. And if it had been read, speaker boards would have played nothing: the HAS_SPEAKER branch only handles !nonBlocking.

On T-Embed CC1101 one blocking audio.tone() stops the script for ~90 ms whatever the duration (measured on 1.16.1: 6, 10, 15 and 40 ms tones all took 88-91 ms). In a 30 fps game that is three frozen frames per beep. The blocking loop also calls check(AnyKeyPress), so a wheel step or click during the beep is consumed to cut the tone short and never reaches the script.

Changes:

  • audio_js.cpp: read the flag with JS_ToBool; with the flag set call _tone(freq, ms, PLAYBACK_ASYNC). Without it nothing changes (still goes through the tone CLI command).
  • playTone() and _tone() get a PlaybackMode mode = PLAYBACK_BLOCKING parameter, the same as playAudioFile(), playAudioRTTTLString() and tts(). In PLAYBACK_ASYNC playTone() hands the WAV generator to the existing startAsyncPlayback() task. That task does not poll keys.
  • Like the other players, playTone() now stops whatever async playback is still running before it starts, so two I2S outputs never fight over the same pins.
  • _tone(): Cardputer keeps its own blocking cardputerTone(); on M5Unified boards PLAYBACK_ASYNC skips the delay() after M5.Speaker.tone(), which is non-blocking by itself.

All existing C++ callers use the defaults and behave as before.

Types of Changes

Bugfix (JS API flag that never worked)

Verification

JS script timing now() around the call, tone 880 Hz / 10 ms:

audio.tone(880, 10) audio.tone(880, 10, true)
before (1.16.1 release, same code path as dev) 88-91 ms 88-91 ms (flag ignored)
after unchanged by this PR 3 ms

The "after" value was measured with this PR and the two other tone fixes applied (frequency and short tones, opened alongside); none of them touches the async path.

Also checked after the change: the tone is audible in async mode; a wheel step during an async tone reaches keyboard.getNextPress(); two async tones in a row do not crash.

Testing

Tested on LilyGO T-Embed CC1101 Plus (lilygo-t-embed-cc1101). Not tested on Cardputer or M5Unified boards (the change there is limited to skipping delay() in async mode).

Linked Issues

None found.

User-Facing Change

JS: audio.tone(freq, ms, true) now plays the tone in the background instead of blocking the script.

Further Comments

Observation, not changed here: the ~90 ms per blocking tone come from tearing the I2S output down and setting it up again for every tone, plus waiting for the DMA buffer to play out. Keeping one output open between tones would fix it for good, but that is a bigger change to the audio module.

The third argument was parsed as nonBlocking = JS_ToInt32(ctx, &nonBlocking, ...),
which stores the return code (0) instead of the value, so every call
blocked. Had the flag been read, speaker boards would have played
nothing: the non-blocking branch was empty.

playTone() and _tone() now take a PlaybackMode like playAudioFile(),
playAudioRTTTLString() and tts(). PLAYBACK_ASYNC hands the tone to the
existing playback task, which does not poll the keys, so a key press
during the tone is no longer eaten. Like the other players, a new tone
stops whatever is still playing. Default stays PLAYBACK_BLOCKING, so
existing callers are unchanged. Cardputer keeps its blocking
cardputerTone(); on M5Unified boards the async mode skips the delay.

This branch has not been deployed

No deployments
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.

1 participant