Skip to content

samples: i2s_codec: Refine i2s_codec sample #96976

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+33 −16
Open

samples: i2s_codec: Refine i2s_codec sample #96976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions samples/drivers/i2s/i2s_codec/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,24 @@ config USE_CODEC_CLOCK
to generate those. If not selected, the I2S peripheral will generate
them and the codec will be expected to consume them.

config EXTRA_BLOCKS
int "Number of extra blocks"
default 32
help
Extra blocks for storing PCM sample.

config SAMPLE_WIDTH
int "Sample bit width"
default 16
help
PCM sample bit width.

config BYTES_PER_SAMPLE
int "Bytes per sample"
default 2
help
Number of bytes per PCM sample.

config USE_DMIC
bool "Use DMIC as an audio input"

Expand Down
6 changes: 3 additions & 3 deletions samples/drivers/i2s/i2s_codec/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@
#define I2S_CODEC_TX DT_ALIAS(i2s_codec_tx)

#define SAMPLE_FREQUENCY CONFIG_SAMPLE_FREQ
#define SAMPLE_BIT_WIDTH (16U)
#define BYTES_PER_SAMPLE sizeof(int16_t)
#define SAMPLE_BIT_WIDTH CONFIG_SAMPLE_WIDTH
#define BYTES_PER_SAMPLE CONFIG_BYTES_PER_SAMPLE
#if CONFIG_USE_DMIC
#define NUMBER_OF_CHANNELS CONFIG_DMIC_CHANNELS
#else
Expand All @@ -32,7 +32,7 @@
#define TIMEOUT (2000U)

#define BLOCK_SIZE (BYTES_PER_SAMPLE * SAMPLES_PER_BLOCK)
#define BLOCK_COUNT (INITIAL_BLOCKS + 32)
#define BLOCK_COUNT (INITIAL_BLOCKS + CONFIG_EXTRA_BLOCKS)

K_MEM_SLAB_DEFINE_IN_SECT_STATIC(mem_slab, __nocache, BLOCK_SIZE, BLOCK_COUNT, 4);

Expand Down
25 changes: 12 additions & 13 deletions samples/drivers/i2s/i2s_codec/src/sine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,16 @@
* SPDX-License-Identifier: Apache-2.0
*/

#ifndef SINE_H_
#define SINE_H_
#ifndef SINE_H_
#define SINE_H_

#if CONFIG_NOCACHE_MEMORY
#define __NOCACHE __attribute__((__section__(".nocache")))
#elif defined(CONFIG_DT_DEFINED_NOCACHE)
#define __NOCACHE __attribute__((__section__(CONFIG_DT_DEFINED_NOCACHE_NAME)))
#else /* CONFIG_NOCACHE_MEMORY */
#define __NOCACHE
#endif /* CONFIG_NOCACHE_MEMORY */

unsigned char __16kHz16bit_stereo_sine_pcm[] __NOCACHE;
unsigned char __16kHz16bit_stereo_sine_pcm[] = {
/*
* Keep the PCM data in flash to avoid large RAM usage.
* If a platform requires DMA-readable RAM buffers, copy
* chunks of this table into a small nocache TX buffer
* at runtime instead of keeping the entire table in RAM.
*/
static const unsigned char __16kHz16bit_stereo_sine_pcm[] = {
0x00, 0x00, 0x00, 0x00, 0x08, 0x0b, 0x08, 0x0b, 0xbb, 0x15, 0xbb, 0x15, 0xc9, 0x1f, 0xc9,
0x1f, 0xe4, 0x28, 0xe4, 0x28, 0xc8, 0x30, 0xc8, 0x30, 0x38, 0x37, 0x38, 0x37, 0x03, 0x3c,
0x03, 0x3c, 0x04, 0x3f, 0x04, 0x3f, 0x25, 0x40, 0x25, 0x40, 0x5d, 0x3f, 0x5d, 0x3f, 0xb1,
Expand Down Expand Up @@ -444,6 +441,8 @@ unsigned char __16kHz16bit_stereo_sine_pcm[] = {
0xa3, 0xc0, 0xa3, 0xc0, 0xdb, 0xbf, 0xdb, 0xbf, 0xfc, 0xc0, 0xfc, 0xc0, 0xfd, 0xc3, 0xfd,
0xc3, 0xc8, 0xc8, 0xc8, 0xc8, 0x38, 0xcf, 0x38, 0xcf, 0x1c, 0xd7, 0x1c, 0xd7, 0x37, 0xe0,
0x37, 0xe0, 0x45, 0xea, 0x45, 0xea, 0xf8, 0xf4, 0xf8, 0xf4};
const unsigned int __16kHz16bit_stereo_sine_pcm_len = sizeof(__16kHz16bit_stereo_sine_pcm);

static const unsigned int __16kHz16bit_stereo_sine_pcm_len =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const is probably not necessary here. See the reported issue: when cast to (void *) in main.c, const is lost.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @TomasBarakNXP, currently, both __16kHz16bit_stereo_sine_pcm and __16kHz16bit_stereo_sine_pcm_len are modified with const. I'm not sure what the "reported issue" you're referring to is. However, I understand that it's worth keeping the const declarations in both places.

Why is it worth keeping the const declaration for __16kHz16bit_stereo_sine_pcm?

  1. Using const for storing the PCM table in flash can significantly save RAM (also stated this motivation in the comments at the top of sine.h).
  2. __16kHz16bit_stereo_sine_pcm is used here:
mem_block = (void *)&__16kHz16bit_stereo_sine_pcm;
ret = i2s_buf_write(i2s_dev_codec, mem_block, block_size);

The prototype of i2s_buf_write is: int i2s_buf_write(const struct device *dev, void *buf, size_t size), and the implementation only memcpy data from buf to the driver-allocated TX slab and then transmits it via DMA, buf is not modified (see drivers/i2s/i2s_common.c: memcpy(mem_block, (void *)buf, size)).
The code mem_block = (void)&__16kHz16bit_stereo_sine_pcm; simply casts the parameter to "lose const" to accommodate the void parameter of i2s_buf_write. Since the function only reads it, this is safe; the only drawback is that it's not perfectly const-correct. In other words, the lack of const API parameters is due to historical reasons/interface design issues, the actual source buffer is read-only.

Why is it worth keeping the const for __16kHz16bit_stereo_sine_pcm_len?

  1. To prevent accidental modification, it explicitly states that "this is read-only data." Any attempt to write to it will result in a compilation failure or an error when read-only segment protection is enabled, preventing potential risks such as length corruption and out-of-bounds access.
  2. Storing it in a read-only segment conserves RAM. Although the savings are not much.

ARRAY_SIZE(__16kHz16bit_stereo_sine_pcm);

#endif /* SINE_H_ */