[PATCH v2 0/7] sound: fix drivers

[PATCH v2 0/7] sound: fix drivers

[Heinrich Schuchardt]

* 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

-- 
2.37.2

[PATCH v2 1/7] sound: avoid endless loop

[Heinrich Schuchardt]

'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)
-- 
2.37.2

[PATCH v2 2/7] test: test sandbox sound driver more rigorously

[Heinrich Schuchardt]

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));
 
-- 
2.37.2

[PATCH v2 3/7] cmd: fix long text for sound command

[Heinrich Schuchardt]

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"
 );
-- 
2.37.2

[PATCH v2 4/7] doc: man-page for the sound command

[Heinrich Schuchardt]

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
-- 
2.37.2

[PATCH v2 5/7] sound: function to generate sine wave

[Heinrich Schuchardt]

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
  *
-- 
2.37.2

[PATCH v2 6/7] sound: add CONFIG_SOUND_SINE symbol

[Heinrich Schuchardt]

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);
-- 
2.37.2

[PATCH v2 7/7] sandbox: fix sound driver

[Heinrich Schuchardt]

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;
 
-- 
2.37.2