-
Notifications
You must be signed in to change notification settings - Fork 8k
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.
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
base: main
Are you sure you want to change the base?
samples: i2s_codec: Refine i2s_codec sample #96976
Conversation
ZhaoxiangJin
commented
Oct 3, 2025
- Configure BLOCK count by using kconfig option 'EXTRA_BLOCKS' instad of fixed value. This allows user to customize the RAM usage based on their platform.
- Keep the PCM data in flash to avoid large RAM usage.
- Refine the sample to get the SAMPLE_BIT_WIDTH and BYTES_PER_SAMPLE from Kconfig options.
Hello @TomasBarakNXP, @VitekST, please help review. Thanks. |
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 = sizeof(__16kHz16bit_stereo_sine_pcm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ARRAY_SIZE be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
int "Extra block size" | ||
default 32 | ||
help | ||
Count of DMIC channels to capture and process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really the count of DMIC channels?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
3f5e03a
to
a587e39
Compare
a587e39
to
dd50879
Compare
Update: |
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 = |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
- 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).
__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
?
- 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.
- Storing it in a read-only segment conserves RAM. Although the savings are not much.
them and the codec will be expected to consume them. | ||
|
||
config EXTRA_BLOCKS | ||
int "Extra block size" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int "Extra block size" | |
int "Number of extra blocks" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
- Configure BLOCK count by using kconfig option 'EXTRA_BLOCKS' instad of fixed value. This allows user to customize the RAM usage based on their platform. - Keep the PCM data in flash to avoid large RAM usage. - Refine the sample to get the SAMPLE_BIT_WIDTH and BYTES_PER_SAMPLE from Kconfig options. Signed-off-by: Zhaoxiang Jin <Zhaoxiang.Jin_1@nxp.com>
dd50879
to
b50fec9
Compare
|