modules: Add zephyr lc3 codec #44225

Casper-Bonde-Bose · 2022-03-25T15:24:15Z

Sample code demonstrating how the LC3 codec can be used for encoding and decoding audio data.
This is a simple integration into the existing BAP unicast application that uses a generated sine wave as audio data, encodes it and transfers it over a CIS. The receiver decodes the data and discards it after checking for errors.
Fixes #41228
This replaces #42443 using the new c-version of the LC3 codec

github-actions · 2022-03-25T15:29:19Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff
liblc3codec	N/A	zephyrproject-rtos/liblc3codec@`3951cf1` (zephyr)	N/A

Note: This message is automatically posted and updated by the Manifest GitHub Action.

Thalley

A couple of minor comments

samples/bluetooth/unicast_audio_client/src/main.c

samples/bluetooth/unicast_audio_server/src/main.c

Thalley · 2022-03-25T16:13:23Z

samples/bluetooth/unicast_audio_server/src/main.c

+	const uint8_t bad_frame_indicator = buf->len == 0 ? 1 : 0;
+	uint8_t *in_buf = (bad_frame_indicator ? NULL : buf->data);


Hmm, apparently we discard the const struct bt_iso_recv_info *info in BAP, so you won't ever get that information directly. Maybe we'll need to fix that.

Yes that would be ideal - also if controllers would actually issue ISO data packets over HCI when they detect a failure to receive :-) (I have only seen one controller that does that.)

The fact that they can, should be enough reason that they should :) For something like audio, that information is of significant worth.

samples/bluetooth/unicast_audio_server/src/main.c

samples/bluetooth/unicast_audio_client/boards/native_posix.conf

samples/bluetooth/unicast_audio_client/boards/nrf5340dk_nrf5340_cpuapp.conf

Thalley · 2022-03-28T10:37:02Z

modules/liblc3codec/Kconfig

+config LIBLC3CODEC
+	bool "liblc3codec Support"
+	help
+	  This option enables the Android liblc3codec library for Bluetooth LE Audio


Should this by default enably e.g. CONFIG_FPU?

Should we add a warning if CONFIG_SYSTEM_WORKQUEUE_STACK_SIZE is too low, given the high stack usage of this module?

CONFIG_FPU is not strictly required if you have a fast CPU and can do it in SW. I'll add it - at least that forces people to think about if they want to disable it.
CONFIG_SYSTEM_WORKQUEUE_STACK_SIZE - Good point - do you have an example on how to do that?

Added select FPU in the LC3 module Kconfig.
Which stack to run the LC3 codec on is up to the application. I added a comment stating that a large stack is needed.

If it is not a strict requirement then I suggest to use default y if FPU or imply FPU as those are less restrictive than depends on or select.

Changed to select FPU after all, but thanks for your inputs - I did not know the imply keyword.
I figured that by imply you allow it to be enabled on any system, but typically systems without a FPU run at 100MHz and below - hence most likely it will not work. On systems with FPU it would most likely work (if the CPU speed is high enough), but I'm sure there are systems out there that do not run 64MHz+, hence I would like not to enable it by default as that would also mean that we need to decide which c-lib people should use.
Would this compromise work for you?

Did you consider depends on?

That is usually recommended instead of select, and I guess also more semantically correct.

Personally I think depends on works better here than select if we want to keep it as a requirement, but also that imply is better than select here, as it not a strict requirement.

I did consider depends on, but that hides the option until you actually set FPU in menuconfig so even though it might be preferred by some, I don't like it. For example FPU is not enabled by default in the nRF5340, hence you would spend ages browsing through the configuration system before you realize you need to enable FPU to show the LC3 option - which is not obvious. IMO depends on is only meant to be used for adding additional configuration options to a config - like if you enable serial port support you get additional options related to serial port.
If there is a general Zephyr guideline that select should not be used I'll change it of cause, but select is nice and simple in this case - as a user it is clear to see that if I enable the feature I also get the features it depends on - hence to me select works as depends on should work in this case. I do not know if there are different tools for doing the configuration and some of those actually implements depends on as select + a warning to the user that the depends on features will also be enabled.
I used select over imply as I'm not sure anyone will want to spend time debugging why the system takes 10 times the expected execution time, and if they really want to run on a fixed point system they can make the required change to enforce it.
I can agree to change to imply if you think the risk of people debugging slow systems makes up the benefit of being able to enable the feature on non-FPU systems without making the required modifications to kconfig.
If optimizations are made in the future that allows the codec to run acceptable on fixed point systems, I fully support changing it :-)

If there is a general Zephyr guideline that select should not be used I'll change it of cause, but select is nice and simple in this case

We have https://docs.zephyrproject.org/latest/guides/build/kconfig/tips.html#alternatives-to-select

I am aware of the issue with the hiding configs when using Kconfig, and this is a divisive issue among Zephyr developers as depends on is clearly preferred by everyone who doesn't use menuconfig (like myself), and vice versa for those who do.

I used select over imply as I'm not sure anyone will want to spend time debugging why the system takes 10 times the expected execution time, and if they really want to run on a fixed point system they can make the required change to enforce it.

If the system does not have FPU enabled, then a warning will appear during build, which should state that FPU=n IIRC.

I am OK with select or imply here

As I read the guide it favors Zephyr developers over Zephyr users - hence favoring code maintenance over ease of use.
If you are not using menuconfig to browse through possible config options, what do you use? (I know that when you get familiar with the zephyr code-base you know where to find the relevant kconfig files, but what would you use if you were a user of Zephyr and wanted to write an application and do initial configuration of the system - enable the features you need).
I guess I need to follow the Zephyr guide-line - will change to depends on. Will do this when updating the west.yml after the module PR is merged as that will trigger a new review anyway.

If you are not using menuconfig to browse through possible config options, what do you use?

I generally browser the Kconfig files and/or source code :) A quick search gives me a result of a total of more than 11000 Kconfig options; browsing that many option in a GUI to me seems nearly impossible, and generally searching for them as text works much faster for me.

If you use VSCode there are also extensions to help with Kconfigs: https://marketplace.visualstudio.com/items?itemName=trond-snekvik.kconfig-lang

I guess I need to follow the Zephyr guide-line - will change to depends on.

It is only a guideline and not a requirement of course, and as mentioned this is typically a personal preference depending on what tools you use :)

samples/bluetooth/unicast_audio_client/src/main.c

Casper-Bonde-Bose · 2022-03-28T11:54:55Z

@carlescufi you might want to have a quick look at the LC3 module commit:
d2c6c91

carlescufi

Integration commit looks fine to me.

Initial addition of the LC3 codec as a module. Usage of the module will be seperate commits. Even though this codec is a generic audio codec, it is tagged with Bluetooth: as it can only be used for LE-Audio. Using the codec for other purposes is a violation of the granted rights for the codec. Signed-off-by: Casper Bonde <casper_bonde@bose.com>

LC3 codec integration into BAP Unicast client and server sample applications. Will be enabled by default for a native_posix and nrf5430 cpuapp builds. The client will continuously transmit a LC3 encoded 400Hz sine wave and the client will receive and decode the data. Implementation verified using UART based HCI controller with ISO interface (userchan.c modified to open the UART directly as bluez do not have support for ISO data) as well as on-target execution on the nrf5340. Signed-off-by: Casper Bonde <casper_bonde@bose.com> Co-authored-by: Emil Gydesen <Thalley@users.noreply.github.com>

Casper-Bonde-Bose · 2022-03-29T11:35:43Z

@carlescufi I updated the reference in west.yml - could you please re-approve if you are happy with it? (834e446)
@Thalley since your last review I only changed the depends on setting as well as deleting a white-space found by the compliance check.

Casper-Bonde-Bose requested review from Thalley and asbjornsabo March 25, 2022 15:24

Casper-Bonde-Bose requested review from jhedberg, Vudentz, alwa-nordic and nashif as code owners March 25, 2022 15:24

stephanosio closed this Mar 25, 2022

stephanosio reopened this Mar 25, 2022

github-actions bot added area: Modules area: native port Host native arch port (native_sim) area: Samples Samples manifest west manifest-liblc3codec DNM This PR should not be merged (Do Not Merge) labels Mar 25, 2022

Thalley requested changes Mar 25, 2022

View reviewed changes

Thalley reviewed Mar 28, 2022

View reviewed changes

samples/bluetooth/unicast_audio_client/boards/native_posix.conf Outdated Show resolved Hide resolved

alwa-nordic removed their request for review March 28, 2022 08:50

Casper-Bonde-Bose force-pushed the 41228_add_zephyr_lc3_codec_to_unicast_sample_c_version branch 4 times, most recently from e065031 to 008b791 Compare March 28, 2022 10:08

Thalley requested changes Mar 28, 2022

View reviewed changes

Casper-Bonde-Bose force-pushed the 41228_add_zephyr_lc3_codec_to_unicast_sample_c_version branch 2 times, most recently from 4fc4abc to 5bd9e44 Compare March 28, 2022 11:53

Casper-Bonde-Bose requested review from Thalley and carlescufi March 28, 2022 11:53

carlescufi changed the title ~~41228 add zephyr lc3 codec to unicast sample c version~~ modules: Add zephyr lc3 codec Mar 28, 2022

carlescufi previously approved these changes Mar 28, 2022

View reviewed changes

zephyrbot added the area: Bluetooth label Mar 28, 2022

zephyrbot requested a review from alwa-nordic March 28, 2022 16:18

zephyrbot assigned jhedberg Mar 28, 2022

alwa-nordic removed their request for review March 28, 2022 19:16

Casper-Bonde-Bose mentioned this pull request Mar 29, 2022

41228 add zephyr lc3 codec to unicast sample #42443

Closed

Thalley previously approved these changes Mar 29, 2022

View reviewed changes

Casper-Bonde-Bose and others added 2 commits March 29, 2022 13:28

Casper-Bonde-Bose dismissed stale reviews from Thalley and carlescufi via 99c561f March 29, 2022 11:32

Casper-Bonde-Bose force-pushed the 41228_add_zephyr_lc3_codec_to_unicast_sample_c_version branch from 5bd9e44 to 99c561f Compare March 29, 2022 11:32

github-actions bot removed the DNM This PR should not be merged (Do Not Merge) label Mar 29, 2022

Thalley approved these changes Mar 29, 2022

View reviewed changes

Casper-Bonde-Bose requested a review from carlescufi March 30, 2022 07:36

carlescufi approved these changes Mar 30, 2022

View reviewed changes

carlescufi merged commit 237e3c8 into zephyrproject-rtos:main Mar 30, 2022

Casper-Bonde-Bose deleted the 41228_add_zephyr_lc3_codec_to_unicast_sample_c_version branch March 30, 2022 12:19

kartben mentioned this pull request Aug 20, 2023

LE Audio: Add LC3 TX for Unicast Server Sample #45595

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modules: Add zephyr lc3 codec #44225

modules: Add zephyr lc3 codec #44225

Casper-Bonde-Bose commented Mar 25, 2022

github-actions bot commented Mar 25, 2022 •

edited

Loading

Thalley left a comment

Thalley Mar 25, 2022

Casper-Bonde-Bose Mar 28, 2022

Thalley Mar 28, 2022

Thalley Mar 28, 2022

Casper-Bonde-Bose Mar 28, 2022

Casper-Bonde-Bose Mar 28, 2022

Thalley Mar 28, 2022

Casper-Bonde-Bose Mar 29, 2022

Thalley Mar 29, 2022 •

edited

Loading

Casper-Bonde-Bose Mar 29, 2022

Thalley Mar 29, 2022 •

edited

Loading

Casper-Bonde-Bose Mar 29, 2022

Thalley Mar 29, 2022

Casper-Bonde-Bose commented Mar 28, 2022

carlescufi left a comment

Casper-Bonde-Bose commented Mar 29, 2022

		const uint8_t bad_frame_indicator = buf->len == 0 ? 1 : 0;
		uint8_t *in_buf = (bad_frame_indicator ? NULL : buf->data);

modules: Add zephyr lc3 codec #44225

modules: Add zephyr lc3 codec #44225

Conversation

Casper-Bonde-Bose commented Mar 25, 2022

github-actions bot commented Mar 25, 2022 • edited Loading

Thalley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thalley Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thalley Mar 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Casper-Bonde-Bose commented Mar 28, 2022

carlescufi left a comment

Choose a reason for hiding this comment

Casper-Bonde-Bose commented Mar 29, 2022

github-actions bot commented Mar 25, 2022 •

edited

Loading

Thalley Mar 29, 2022 •

edited

Loading

Thalley Mar 29, 2022 •

edited

Loading