applications: nrf5340_audio: Enable bidirectional CIS #8915

alexsven · 2022-10-05T10:03:00Z

Implement a microphone return stream from
left headset to gateway

Signed-off-by: Erik Robstad erik.robstad@nordicsemi.no

NordicBuilder · 2022-10-06T11:32:35Z

Integration test specification

Test Module	File based changes	Manually selected	West overwrite
test-sdk-audio	X

Detailed information of selected test modules

Note: This message is automatically posted and updated by the CI

NordicBuilder · 2022-10-06T11:34:54Z

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

erikrobstad

In general it feels overly hardcoded, at least on the gateway. But I guess it has to be this way because of the current limitations.

Also, remember documentation change

applications/nrf5340_audio/src/audio/Kconfig

applications/nrf5340_audio/src/bluetooth/Kconfig.defaults

applications/nrf5340_audio/src/bluetooth/le_audio_cis_gateway.c

applications/nrf5340_audio/src/bluetooth/le_audio_cis_headset.c

applications/nrf5340_audio/src/bluetooth/le_audio_cis_gateway.c

applications/nrf5340_audio/src/audio/Kconfig

greg-fer · 2022-10-10T06:57:47Z

applications/nrf5340_audio/README.rst

@@ -91,10 +86,12 @@ Connected Isochronous Stream (CIS)
  In this configuration, you can use the nRF5340 Audio development kit in the role of the gateway, the left headset, or the right headset.

  .. note::
-     In the current version of the nRF5340 Audio application, the CIS mode offers only monodirectional communication.
+     In the current version of the nRF5340 Audio application, the CIS mode offers both unidirectional and bidirectional communication.


We need to add a section under Configuration -- "Selecting the CIS bidirectional communication" -- and describe there how to enable the bidirectional stream.

Also, we need to mention that Testing covers the unidirectional mode.

@erikrobstad , does this PR require changes in the figures? I gave a cursory look, but I'm unable to verify.

@greg-fer It does not look like we need to change the figures

@alexsven It does not look like you have added that (CIS) Testing covers the unidirectional mode.

Testing as in the CI-system? The tests for bidirectional are done, just waiting for this PR to be merged

In the documentation Testing  section it must be mentioned that this is for the unidirectional mode.

applications/nrf5340_audio/README.rst

greg-fer · 2022-10-11T10:05:17Z

Also, I would suggest adding a changelog entry, but it is your call :)

greg-fer

Approving, but please add those periods. Period.

applications/nrf5340_audio/README.rst

doc/nrf/releases/release-notes-changelog.rst

rick1082

Thanks for the discussion, understood that this is the first step for bidirection feature. And will keep improve for supporting TWS in the later.

applications/nrf5340_audio/src/bluetooth/le_audio_cis_headset.c

applications/nrf5340_audio/README.rst

erikrobstad · 2022-10-13T10:56:27Z

applications/nrf5340_audio/README.rst

@@ -91,10 +86,12 @@ Connected Isochronous Stream (CIS)
  In this configuration, you can use the nRF5340 Audio development kit in the role of the gateway, the left headset, or the right headset.

  .. note::
-     In the current version of the nRF5340 Audio application, the CIS mode offers only monodirectional communication.
+     In the current version of the nRF5340 Audio application, the CIS mode offers both unidirectional and bidirectional communication.


@greg-fer It does not look like we need to change the figures

erikrobstad · 2022-10-13T10:57:37Z

applications/nrf5340_audio/README.rst

@@ -91,10 +86,12 @@ Connected Isochronous Stream (CIS)
  In this configuration, you can use the nRF5340 Audio development kit in the role of the gateway, the left headset, or the right headset.

  .. note::
-     In the current version of the nRF5340 Audio application, the CIS mode offers only monodirectional communication.
+     In the current version of the nRF5340 Audio application, the CIS mode offers both unidirectional and bidirectional communication.


@alexsven It does not look like you have added that (CIS) Testing covers the unidirectional mode.

erikrobstad

Remember the part in the documentation Testing section, ref. previous comment

erikrobstad · 2022-10-13T11:54:35Z

applications/nrf5340_audio/README.rst

@@ -34,6 +34,7 @@ Overview
 The application can work as a gateway or a headset.
 The gateway receives the audio data from external sources (USB or I2S) and forwards it to one or more headsets.
 The headset is a receiver device that plays back the audio it gets from the gateway.
+There is also the bidirectional mode where both gateway and headset can receive and send at the same time.


Suggested change

There is also the bidirectional mode where both gateway and headset can receive and send at the same time.

It is also possible to enable a bidirectional mode where one gateway and one headset can send and receive audio to and from each other at the same time.

erikrobstad · 2022-10-13T11:56:13Z

doc/nrf/software_maturity.rst


          Play and pause emulated by disabling and enabling stream, respectively.
        - The following limitations apply:

          * BAP unicast, one CIG with two CIS.
          * Audio input: USB or I2S (Line in or using Pulse Density Modulation).
+          * Audio output: USB or I2S/Analog headset output.


Isnt it a limitation that only one headset can be used for bidirectional stream?

tejlmand · 2022-10-14T10:26:39Z

applications/nrf5340_audio/src/audio/Kconfig

@@ -196,10 +196,11 @@ config BUF_BLE_RX_PACKET_NUM
 	  gaps due to BLE retransmits.

 config STREAM_BIDIRECTIONAL
-	bool "Enable bi-directional stream - Currently not supported"
+	depends on TRANSPORT_CIS
+	bool "Enable bidirectional stream"


Please remove enable as per guide lines:
https://docs.zephyrproject.org/latest/build/kconfig/tips.html#prompt-strings

For a Kconfig symbol that enables a driver/subsystem FOO, consider having just “Foo” as the prompt, instead of “Enable Foo support” or the like.

tejlmand · 2022-10-14T10:30:32Z

applications/nrf5340_audio/src/bluetooth/Kconfig.defaults

@@ -143,8 +142,13 @@ config BT_AUDIO_UNICAST_CLIENT
 	bool
 	default y

+config BT_ISO_TX_BUF_COUNT
+	int


If this is just changing the default then there is no need for the int type.

Suggested change

int

https://docs.zephyrproject.org/latest/build/kconfig/tips.html#common-kconfig-shorthands

For a symbol defined in multiple locations (e.g., in a Kconfig.defconfig file in Zephyr), it is best to only give the symbol type for the “base” definition of the symbol

Done, I removed all int, bool, string etc from this file. But the rest of the Kconfig.default files we should do as a separate PR

But the rest of the Kconfig.default files we should do as a separate PR

agreed.

tejlmand · 2022-10-14T10:31:21Z

applications/nrf5340_audio/src/bluetooth/Kconfig.defaults

+config BT_ISO_TX_BUF_COUNT
+	int
+	default 2
+
 config BT_MAX_CONN
 	int


I know this is existing code, but when update the default then please remove the int type.

Suggested change

int

tejlmand · 2022-10-14T10:31:38Z

applications/nrf5340_audio/src/bluetooth/Kconfig.defaults

+	default 2
+
+config BT_AUDIO_UNICAST_CLIENT_ASE_SRC_COUNT
+	int


Suggested change

int

tejlmand · 2022-10-14T10:31:49Z

applications/nrf5340_audio/src/bluetooth/Kconfig.defaults

+	int
+	default 1 if STREAM_BIDIRECTIONAL
+	default 0
+
 config BT_VCS_CLIENT
 	bool


Suggested change

bool

tejlmand · 2022-10-14T10:31:55Z

applications/nrf5340_audio/src/bluetooth/Kconfig.defaults

@@ -159,6 +163,16 @@ config BT_AUDIO_UNICAST_CLIENT_GROUP_STREAM_COUNT
 	int


Suggested change

int

Implement a microphone return stream from left headset to gateway Signed-off-by: Erik Robstad <erik.robstad@nordicsemi.no> Signed-off-by: Alexander Svensen <alexander.svensen@nordicsemi.no>

tejlmand

lgtm

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 5, 2022

alexsven force-pushed the OCT-2068-enable-bidirectionality branch 4 times, most recently from a0e582a to 4468f96 Compare October 6, 2022 10:54

alexsven marked this pull request as ready for review October 6, 2022 10:54

alexsven requested review from tejlmand, koffes, erikrobstad, rick1082 and gWacey as code owners October 6, 2022 10:54

erikrobstad requested changes Oct 7, 2022

View reviewed changes

erikrobstad reviewed Oct 7, 2022

View reviewed changes

applications/nrf5340_audio/src/bluetooth/le_audio_cis_gateway.c Show resolved Hide resolved

alexsven force-pushed the OCT-2068-enable-bidirectionality branch from 4468f96 to d2ea0c9 Compare October 7, 2022 14:02

github-actions bot added the doc-required PR must not be merged without tech writer approval. label Oct 7, 2022

alexsven requested review from erikrobstad and greg-fer October 7, 2022 14:02

greg-fer requested changes Oct 10, 2022

View reviewed changes

alexsven force-pushed the OCT-2068-enable-bidirectionality branch 2 times, most recently from cd72a49 to 869fe3d Compare October 10, 2022 12:33

alexsven requested a review from greg-fer October 10, 2022 12:34

greg-fer requested changes Oct 11, 2022

View reviewed changes

applications/nrf5340_audio/README.rst Outdated Show resolved Hide resolved

applications/nrf5340_audio/README.rst Outdated Show resolved Hide resolved

applications/nrf5340_audio/README.rst Outdated Show resolved Hide resolved

alexsven force-pushed the OCT-2068-enable-bidirectionality branch from 869fe3d to d5c9098 Compare October 11, 2022 10:16

alexsven requested a review from carlescufi as a code owner October 11, 2022 10:16

alexsven requested a review from greg-fer October 11, 2022 10:16

github-actions bot removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 11, 2022

greg-fer approved these changes Oct 12, 2022

View reviewed changes

applications/nrf5340_audio/README.rst Outdated Show resolved Hide resolved

doc/nrf/releases/release-notes-changelog.rst Outdated Show resolved Hide resolved

alexsven force-pushed the OCT-2068-enable-bidirectionality branch from d5c9098 to 81a3efc Compare October 12, 2022 06:37

rick1082 approved these changes Oct 12, 2022

View reviewed changes

rick1082 requested changes Oct 13, 2022

View reviewed changes

applications/nrf5340_audio/src/bluetooth/le_audio_cis_headset.c Show resolved Hide resolved

alexsven force-pushed the OCT-2068-enable-bidirectionality branch from 81a3efc to 4427168 Compare October 13, 2022 07:22

alexsven requested a review from rick1082 October 13, 2022 07:22

alexsven force-pushed the OCT-2068-enable-bidirectionality branch from 4427168 to 0087d15 Compare October 13, 2022 08:17

rick1082 approved these changes Oct 13, 2022

View reviewed changes

erikrobstad reviewed Oct 13, 2022

View reviewed changes

applications/nrf5340_audio/src/bluetooth/le_audio_cis_headset.c Outdated Show resolved Hide resolved

applications/nrf5340_audio/src/bluetooth/le_audio_cis_headset.c Outdated Show resolved Hide resolved

alexsven force-pushed the OCT-2068-enable-bidirectionality branch from 0087d15 to b1656e2 Compare October 13, 2022 10:54

erikrobstad requested changes Oct 13, 2022

View reviewed changes

alexsven force-pushed the OCT-2068-enable-bidirectionality branch from b1656e2 to 4cb2cd2 Compare October 13, 2022 11:11

alexsven requested a review from erikrobstad October 13, 2022 11:26

erikrobstad requested changes Oct 13, 2022

View reviewed changes

alexsven force-pushed the OCT-2068-enable-bidirectionality branch from 4cb2cd2 to fdb05bf Compare October 13, 2022 12:11

erikrobstad approved these changes Oct 13, 2022

View reviewed changes

greg-fer approved these changes Oct 13, 2022

View reviewed changes

tejlmand requested changes Oct 14, 2022

View reviewed changes

applications: nrf5340_audio: Enable bidirectional CIS

d016088

Implement a microphone return stream from left headset to gateway Signed-off-by: Erik Robstad <erik.robstad@nordicsemi.no> Signed-off-by: Alexander Svensen <alexander.svensen@nordicsemi.no>

alexsven force-pushed the OCT-2068-enable-bidirectionality branch from fdb05bf to d016088 Compare October 14, 2022 10:47

alexsven requested a review from tejlmand October 14, 2022 10:48

tejlmand approved these changes Oct 14, 2022

View reviewed changes

tejlmand merged commit f543395 into nrfconnect:main Oct 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

applications: nrf5340_audio: Enable bidirectional CIS #8915

applications: nrf5340_audio: Enable bidirectional CIS #8915

alexsven commented Oct 5, 2022

NordicBuilder commented Oct 6, 2022 •

edited

NordicBuilder commented Oct 6, 2022

erikrobstad left a comment

greg-fer Oct 10, 2022

erikrobstad Oct 13, 2022

erikrobstad Oct 13, 2022

alexsven Oct 13, 2022

erikrobstad Oct 13, 2022

greg-fer commented Oct 11, 2022

greg-fer left a comment

rick1082 left a comment

erikrobstad Oct 13, 2022

erikrobstad Oct 13, 2022

erikrobstad left a comment •

edited

erikrobstad Oct 13, 2022

erikrobstad Oct 13, 2022

tejlmand Oct 14, 2022

alexsven Oct 14, 2022

tejlmand Oct 14, 2022

alexsven Oct 14, 2022

tejlmand Oct 14, 2022

tejlmand Oct 14, 2022

tejlmand Oct 14, 2022

tejlmand Oct 14, 2022

tejlmand Oct 14, 2022

tejlmand left a comment

	There is also the bidirectional mode where both gateway and headset can receive and send at the same time.
	It is also possible to enable a bidirectional mode where one gateway and one headset can send and receive audio to and from each other at the same time.

		@@ -159,6 +163,16 @@ config BT_AUDIO_UNICAST_CLIENT_GROUP_STREAM_COUNT
		int

applications: nrf5340_audio: Enable bidirectional CIS #8915

applications: nrf5340_audio: Enable bidirectional CIS #8915

Conversation

alexsven commented Oct 5, 2022

NordicBuilder commented Oct 6, 2022 • edited

Integration test specification

NordicBuilder commented Oct 6, 2022

erikrobstad 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

greg-fer commented Oct 11, 2022

greg-fer left a comment

Choose a reason for hiding this comment

rick1082 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikrobstad left a comment • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejlmand left a comment

Choose a reason for hiding this comment

NordicBuilder commented Oct 6, 2022 •

edited

erikrobstad left a comment •

edited