[PATCH v2 0/7] sound: fix drivers
* Avoid endless loop in sound_create_square_wave() * In unit test add edge cases. * Fix long text for sound command. * Add man-page for sound command. * Add sine wave generator. * Fix sandbox driver.
v2: bring all sound related patches into one series add default values in man-page remove stray printf() update man-page concerning sine wave
Heinrich Schuchardt (7): sound: avoid endless loop test: test sandbox sound driver more rigorously cmd: fix long text for sound command doc: man-page for the sound command sound: function to generate sine wave sound: add CONFIG_SOUND_SINE symbol sandbox: fix sound driver
arch/sandbox/cpu/sdl.c | 11 ++++----- arch/sandbox/include/asm/test.h | 10 ++++++++ cmd/sound.c | 2 +- doc/usage/cmd/sound.rst | 44 +++++++++++++++++++++++++++++++++ doc/usage/index.rst | 1 + drivers/sound/Kconfig | 6 +++++ drivers/sound/sandbox.c | 9 +++++++ drivers/sound/sound-uclass.c | 10 ++++++-- drivers/sound/sound.c | 42 ++++++++++++++++++++++++++++++- include/sound.h | 12 +++++++++ test/dm/sound.c | 25 +++++++++++++++---- 11 files changed, 157 insertions(+), 15 deletions(-) create mode 100644 doc/usage/cmd/sound.rst
'sound play 1 100000' results in an endless loop on the sandbox.
If the frequency exceeds half the sampling rate, zero out the output buffer.
Fixes: 511ed5fdd389 ("SOUND: SAMSUNG: Add I2S driver") Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: no change --- drivers/sound/sound.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/sound/sound.c b/drivers/sound/sound.c index 041dfdccfe..c0fc50c99d 100644 --- a/drivers/sound/sound.c +++ b/drivers/sound/sound.c @@ -15,7 +15,10 @@ void sound_create_square_wave(uint sample_rate, unsigned short *data, int size, const int period = freq ? sample_rate / freq : 0; const int half = period / 2;
- assert(freq); + if (!half) { + memset(data, 0, size); + return; + }
/* Make sure we don't overflow our buffer */ if (size % 2)
Consider unexpected values for frequency:
* negative frequency * zero frequency * frequency exceeding sampling frequency
As in these cases the sum of the samples is zero also check the count of the samples.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: no change --- arch/sandbox/include/asm/test.h | 10 ++++++++++ drivers/sound/sandbox.c | 9 +++++++++ test/dm/sound.c | 11 +++++++++++ 3 files changed, 30 insertions(+)
diff --git a/arch/sandbox/include/asm/test.h b/arch/sandbox/include/asm/test.h index 0406085917..568738c16d 100644 --- a/arch/sandbox/include/asm/test.h +++ b/arch/sandbox/include/asm/test.h @@ -188,6 +188,16 @@ int sandbox_get_setup_called(struct udevice *dev); */ int sandbox_get_sound_active(struct udevice *dev);
+/** + * sandbox_get_sound_count() - Read back the count of the sound data so far + * + * This data is provided to the sandbox driver by the sound play() method. + * + * @dev: Device to check + * Return: count of audio data + */ +int sandbox_get_sound_count(struct udevice *dev); + /** * sandbox_get_sound_sum() - Read back the sum of the sound data so far * diff --git a/drivers/sound/sandbox.c b/drivers/sound/sandbox.c index 4a2c87a84c..c6cbd81fdb 100644 --- a/drivers/sound/sandbox.c +++ b/drivers/sound/sandbox.c @@ -29,6 +29,7 @@ struct sandbox_i2s_priv { struct sandbox_sound_priv { int setup_called; /* Incremented when setup() method is called */ bool active; /* TX data is being sent */ + int count; /* Use to count the provided audio data */ int sum; /* Use to sum the provided audio data */ bool allow_beep; /* true to allow the start_beep() interface */ int frequency_hz; /* Beep frequency if active, else 0 */ @@ -68,6 +69,13 @@ int sandbox_get_sound_active(struct udevice *dev) return priv->active; }
+int sandbox_get_sound_count(struct udevice *dev) +{ + struct sandbox_sound_priv *priv = dev_get_priv(dev); + + return priv->count; +} + int sandbox_get_sound_sum(struct udevice *dev) { struct sandbox_sound_priv *priv = dev_get_priv(dev); @@ -168,6 +176,7 @@ static int sandbox_sound_play(struct udevice *dev, void *data, uint data_size)
for (i = 0; i < data_size; i++) priv->sum += ((uint8_t *)data)[i]; + priv->count += data_size;
return i2s_tx_data(uc_priv->i2s, data, data_size); } diff --git a/test/dm/sound.c b/test/dm/sound.c index b73f6ab111..15d545ab5a 100644 --- a/test/dm/sound.c +++ b/test/dm/sound.c @@ -26,8 +26,19 @@ static int dm_test_sound(struct unit_test_state *uts) ut_asserteq(0, sandbox_get_setup_called(dev));
ut_assertok(sound_beep(dev, 1, 100)); + ut_asserteq(48, sandbox_get_sound_count(dev)); ut_asserteq(4560, sandbox_get_sound_sum(dev)); ut_assertok(sound_beep(dev, 1, 100)); + ut_asserteq(96, sandbox_get_sound_count(dev)); + ut_asserteq(9120, sandbox_get_sound_sum(dev)); + ut_assertok(sound_beep(dev, 1, -100)); + ut_asserteq(144, sandbox_get_sound_count(dev)); + ut_asserteq(9120, sandbox_get_sound_sum(dev)); + ut_assertok(sound_beep(dev, 1, 0)); + ut_asserteq(192, sandbox_get_sound_count(dev)); + ut_asserteq(9120, sandbox_get_sound_sum(dev)); + ut_assertok(sound_beep(dev, 1, INT_MAX)); + ut_asserteq(240, sandbox_get_sound_count(dev)); ut_asserteq(9120, sandbox_get_sound_sum(dev)); ut_asserteq(false, sandbox_get_sound_active(dev));
Make it clear that if only 1 parameter is provided this is the duration.
The ISO symbol for hertz is Hz.
Fixes: c0c88533fffd ("Sound: Add command for audio playback") Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: no change --- cmd/sound.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cmd/sound.c b/cmd/sound.c index f82f2aa670..20ac3f758e 100644 --- a/cmd/sound.c +++ b/cmd/sound.c @@ -86,5 +86,5 @@ U_BOOT_CMD( sound, 4, 1, do_sound, "sound sub-system", "init - initialise the sound driver\n" - "sound play [len] [freq] - play a sound for len ms at freq hz\n" + "sound play [len [freq]] - play a sound for len ms at freq Hz\n" );
Provide a man-page for the sound command.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: add default values --- doc/usage/cmd/sound.rst | 41 +++++++++++++++++++++++++++++++++++++++++ doc/usage/index.rst | 1 + 2 files changed, 42 insertions(+) create mode 100644 doc/usage/cmd/sound.rst
diff --git a/doc/usage/cmd/sound.rst b/doc/usage/cmd/sound.rst new file mode 100644 index 0000000000..d3fac243b1 --- /dev/null +++ b/doc/usage/cmd/sound.rst @@ -0,0 +1,41 @@ +.. SPDX-License-Identifier: GPL-2.0+ +.. Copyright 2022, Heinrich Schuchardt xypron.glpk@gmx.de + +sound command +============= + +Synopsis +-------- + +:: + + sound init + sound play [len [freq]] + +Description +----------- + +The *sound* command is used to play a beep sound. + +sound init + initializes the sound driver. + +sound play + plays a square wave sound. It does not depend on previously calling + *sound init*. + +len + duration of the sound in ms, defaults to 1000 ms + +freq + frequency of the sound in Hz, defaults to 400 Hz + +Configuration +------------- + +The sound command is enabled by CONFIG_CMD_SOUND=y. + +Return value +------------ + +The return value $? is 0 (true) if the command succeeds, 1 (false) otherwise. diff --git a/doc/usage/index.rst b/doc/usage/index.rst index 0bc82887e9..bbd40a6e18 100644 --- a/doc/usage/index.rst +++ b/doc/usage/index.rst @@ -73,6 +73,7 @@ Shell commands cmd/scp03 cmd/setexpr cmd/size + cmd/sound cmd/temperature cmd/tftpput cmd/true
Add function sound_create_sine_wave().
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: remove stray printf() --- drivers/sound/sound.c | 37 +++++++++++++++++++++++++++++++++++++ include/sound.h | 12 ++++++++++++ 2 files changed, 49 insertions(+)
diff --git a/drivers/sound/sound.c b/drivers/sound/sound.c index c0fc50c99d..abf9bb6de2 100644 --- a/drivers/sound/sound.c +++ b/drivers/sound/sound.c @@ -8,6 +8,43 @@ #include <log.h> #include <sound.h>
+/* Amplitude between 1 and 32767 */ +#define AMP 16384 +#define AMP2PI ((int)(6.28318530718 * AMP)) + +void sound_create_sine_wave(uint sample_rate, unsigned short *data, int size, + uint freq, uint channels) +{ + int v, x = 0, y = AMP; + + if (freq >= 0 && freq < sample_rate / 8) { + v = (AMP2PI * freq) / sample_rate; + /* tan(x) = x + x^3/3 + ... */ + v += ((v * v) / AMP * v) / (3 * AMP); + } else { + v = 0; + } + + for (int i = 0; i < size - (2 * channels - 1);) { + int s, dx, dy; + + dx = (v * y) / AMP; + dy = -((v * x) / AMP); + x += dx; + y += dy; + + /* Normalize radius: (1+x)^2 ~ 1+2x, for small x */ + s = AMP * AMP - x * x - y * y; + s /= 2 * AMP; + s += AMP; + x = (s * x) / AMP; + y = (s * y) / AMP; + + for (int j = 0; size && j < channels; ++j, i += 2) + *data++ = x; + } +} + void sound_create_square_wave(uint sample_rate, unsigned short *data, int size, uint freq, uint channels) { diff --git a/include/sound.h b/include/sound.h index dab9ea186e..cf9c3e8fb7 100644 --- a/include/sound.h +++ b/include/sound.h @@ -34,6 +34,18 @@ struct sound_uc_priv { int setup_done; };
+/** + * Generates sine wave sound data for 1 second + * + * @sample_rate: Sample rate in Hz + * @data: data buffer pointer + * @size: size of the buffer in bytes + * @freq: frequency of the wave + * @channels: Number of channels to use + */ +void sound_create_sine_wave(uint sample_rate, unsigned short *data, int size, + uint freq, uint channels); + /** * Generates square wave sound data for 1 second *
Provide a configuration symbol to allow the sound command play a sine wave instead of a square wave.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: update documentation --- doc/usage/cmd/sound.rst | 3 +++ drivers/sound/Kconfig | 6 ++++++ drivers/sound/sound-uclass.c | 10 ++++++++-- test/dm/sound.c | 20 ++++++++++++-------- 4 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/doc/usage/cmd/sound.rst b/doc/usage/cmd/sound.rst index d3fac243b1..aa988fa261 100644 --- a/doc/usage/cmd/sound.rst +++ b/doc/usage/cmd/sound.rst @@ -35,6 +35,9 @@ Configuration
The sound command is enabled by CONFIG_CMD_SOUND=y.
+By default a square wave is generated. With CONFIG_SOUND_SINE=y a sine wave is +used instead. + Return value ------------
diff --git a/drivers/sound/Kconfig b/drivers/sound/Kconfig index 0948d8caab..1005192fd4 100644 --- a/drivers/sound/Kconfig +++ b/drivers/sound/Kconfig @@ -12,6 +12,12 @@ config SOUND audio codecs are called from the sound-i2s code. This could be converted to driver model.
+config SOUND_SINE + bool "Generate sine wave" + help + When this setting is enabled playing a sound produces a sine + wave. If the settings is not enabled, a square wave is produced. + config I2S bool "Enable I2S support" depends on SOUND diff --git a/drivers/sound/sound-uclass.c b/drivers/sound/sound-uclass.c index 2ffc4fc7c1..637f6b11ab 100644 --- a/drivers/sound/sound-uclass.c +++ b/drivers/sound/sound-uclass.c @@ -99,8 +99,14 @@ int sound_beep(struct udevice *dev, int msecs, int frequency_hz) return -ENOMEM; }
- sound_create_square_wave(i2s_uc_priv->samplingrate, data, data_size, - frequency_hz, i2s_uc_priv->channels); + if (CONFIG_IS_ENABLED(SOUND_SINE)) + sound_create_sine_wave(i2s_uc_priv->samplingrate, data, + data_size, frequency_hz, + i2s_uc_priv->channels); + else + sound_create_square_wave(i2s_uc_priv->samplingrate, data, + data_size, frequency_hz, + i2s_uc_priv->channels);
ret = 0; while (msecs >= 1000) { diff --git a/test/dm/sound.c b/test/dm/sound.c index 15d545ab5a..f16ea80157 100644 --- a/test/dm/sound.c +++ b/test/dm/sound.c @@ -17,6 +17,12 @@ static int dm_test_sound(struct unit_test_state *uts) { struct sound_uc_priv *uc_priv; struct udevice *dev; + int expected; + + if (CONFIG_IS_ENABLED(SOUND_SINE)) + expected = 3494; + else + expected = 4560;
/* check probe success */ ut_assertok(uclass_first_device_err(UCLASS_SOUND, &dev)); @@ -24,24 +30,22 @@ static int dm_test_sound(struct unit_test_state *uts) ut_asserteq_str("audio-codec", uc_priv->codec->name); ut_asserteq_str("i2s", uc_priv->i2s->name); ut_asserteq(0, sandbox_get_setup_called(dev)); - ut_assertok(sound_beep(dev, 1, 100)); ut_asserteq(48, sandbox_get_sound_count(dev)); - ut_asserteq(4560, sandbox_get_sound_sum(dev)); + ut_asserteq(expected, sandbox_get_sound_sum(dev)); ut_assertok(sound_beep(dev, 1, 100)); ut_asserteq(96, sandbox_get_sound_count(dev)); - ut_asserteq(9120, sandbox_get_sound_sum(dev)); + expected *= 2; + ut_asserteq(expected, sandbox_get_sound_sum(dev)); ut_assertok(sound_beep(dev, 1, -100)); ut_asserteq(144, sandbox_get_sound_count(dev)); - ut_asserteq(9120, sandbox_get_sound_sum(dev)); + ut_asserteq(expected, sandbox_get_sound_sum(dev)); ut_assertok(sound_beep(dev, 1, 0)); ut_asserteq(192, sandbox_get_sound_count(dev)); - ut_asserteq(9120, sandbox_get_sound_sum(dev)); + ut_asserteq(expected, sandbox_get_sound_sum(dev)); ut_assertok(sound_beep(dev, 1, INT_MAX)); ut_asserteq(240, sandbox_get_sound_count(dev)); - ut_asserteq(9120, sandbox_get_sound_sum(dev)); - ut_asserteq(false, sandbox_get_sound_active(dev)); - + ut_asserteq(expected, sandbox_get_sound_sum(dev)); return 0; } DM_TEST(dm_test_sound, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
In the callback function we have to use memcpy(). Otherwise we add the new samples on top of what is stored in the stream buffer.
If we don't have enough data, zero out the rest of the stream buffer.
Our sampling frequency is 48000. Let the batch size for the callback function be 960. If we play a multiple of 20 ms, this will always be a full batch.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: no change --- arch/sandbox/cpu/sdl.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/arch/sandbox/cpu/sdl.c b/arch/sandbox/cpu/sdl.c index f4ca36b35c..2c570ed8d1 100644 --- a/arch/sandbox/cpu/sdl.c +++ b/arch/sandbox/cpu/sdl.c @@ -441,7 +441,6 @@ void sandbox_sdl_fill_audio(void *udata, Uint8 *stream, int len) { struct buf_info *buf; int avail; - bool have_data = false; int i;
for (i = 0; i < 2; i++) { @@ -453,10 +452,9 @@ void sandbox_sdl_fill_audio(void *udata, Uint8 *stream, int len) } if (avail > len) avail = len; - have_data = true;
- SDL_MixAudio(stream, buf->data + buf->pos, avail, - SDL_MIX_MAXVOLUME); + memcpy(stream, buf->data + buf->pos, avail); + stream += avail; buf->pos += avail; len -= avail;
@@ -466,7 +464,8 @@ void sandbox_sdl_fill_audio(void *udata, Uint8 *stream, int len) else break; } - sdl.stopping = !have_data; + memset(stream, 0, len); + sdl.stopping = !!len; }
int sandbox_sdl_sound_init(int rate, int channels) @@ -484,7 +483,7 @@ int sandbox_sdl_sound_init(int rate, int channels) wanted.freq = rate; wanted.format = AUDIO_S16; wanted.channels = channels; - wanted.samples = 1024; /* Good low-latency value for callback */ + wanted.samples = 960; /* Good low-latency value for callback */ wanted.callback = sandbox_sdl_fill_audio; wanted.userdata = NULL;
participants (1)
-
Heinrich Schuchardt