[PATCH 0/3] sandbox: fix sound driver

[PATCH 0/3] sandbox: fix sound driver

[Heinrich Schuchardt]

The playback from the sound driver does not provide the expected sound.

To make this easier to hear add a sine wave generator to the sound driver
and add a configuration symbol to select it.

This series is on top of
[PATCH 1/1] test: test sandbox sound driver more rigorously
https://lists.denx.de/pipermail/u-boot/2022-December/501266.html

Heinrich Schuchardt (3):
  sound: function to generate sine wave
  sound: add CONFIG_SOUND_SINE symbol
  sandbox: fix sound driver

 arch/sandbox/cpu/sdl.c       | 11 +++++-----
 drivers/sound/Kconfig        |  6 ++++++
 drivers/sound/sound-uclass.c | 10 +++++++--
 drivers/sound/sound.c        | 39 ++++++++++++++++++++++++++++++++++++
 include/sound.h              | 12 +++++++++++
 5 files changed, 70 insertions(+), 8 deletions(-)

-- 
2.37.2

[PATCH 1/3] sound: function to generate sine wave

[Heinrich Schuchardt]

Add function sound_create_sine_wave().

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 drivers/sound/sound.c | 39 +++++++++++++++++++++++++++++++++++++++
 include/sound.h       | 12 ++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/drivers/sound/sound.c b/drivers/sound/sound.c
index c0fc50c99d..10551f1e45 100644
--- a/drivers/sound/sound.c
+++ b/drivers/sound/sound.c
@@ -8,6 +8,45 @@
 #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;
+
+	printf("%d\n", AMP2PI);
+
+	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 2/3] 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>
---
 drivers/sound/Kconfig        |  6 ++++++
 drivers/sound/sound-uclass.c | 10 ++++++++--
 test/dm/sound.c              | 20 ++++++++++++--------
 3 files changed, 26 insertions(+), 10 deletions(-)

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 3/3] 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>
---
 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

Re: [PATCH 2/3] sound: add CONFIG_SOUND_SINE symbol

[Simon Glass]

Hi Heinrich,

On Mon, 5 Dec 2022 at 09:53, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> 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>
> ---
>  drivers/sound/Kconfig        |  6 ++++++
>  drivers/sound/sound-uclass.c | 10 ++++++++--
>  test/dm/sound.c              | 20 ++++++++++++--------
>  3 files changed, 26 insertions(+), 10 deletions(-)

This should be a runtime setting, so we can test both. Probably it
needs another arg to sound_beep().

Regards,
Simon

Re: [PATCH 1/3] sound: function to generate sine wave

[Simon Glass]

On Mon, 5 Dec 2022 at 09:53, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Add function sound_create_sine_wave().
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  drivers/sound/sound.c | 39 +++++++++++++++++++++++++++++++++++++++
>  include/sound.h       | 12 ++++++++++++
>  2 files changed, 51 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH 3/3] sandbox: fix sound driver

[Simon Glass]

On Mon, 5 Dec 2022 at 09:53, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> 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>
> ---
>  arch/sandbox/cpu/sdl.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>