Bluetooth: pacs: Add dynamic PACS registration #83730

fredrikdanebjer · 2025-01-09T08:52:50Z

Added option to set the PACS Characteristics through the bap API, making PACS configuration runtime available. Source and Sink PAC, as well as Source/Sink PAC Location is can be set through a register function in the PACS api.

jthm-ot · 2025-01-17T10:18:17Z

doc/releases/migration-guide-4.1.rst

@@ -321,6 +321,10 @@ Bluetooth Audio
    * :kconfig:option:`CONFIG_BT_PAC_SNK`
    * :kconfig:option:`CONFIG_BT_PAC_SRC`

+* PACS have been changed to support dynamic, runtime configuration. This means that PACS now has
+  to be regsitered with :c:func:`bt_pacs_register` before it can be used. All Kconfig option still


Just some typo:

regsitered -> registered
Runetime -> Runtime

doc/releases/migration-guide-4.1.rst

include/zephyr/bluetooth/audio/pacs.h

Thalley · 2025-01-17T11:25:14Z

include/zephyr/bluetooth/audio/pacs.h

@@ -45,6 +45,18 @@ struct bt_pacs_cap {
 	sys_snode_t _node;
 };

+
+/** Structure for registering PACS */
+struct bt_bap_pacs_register_param {


Would it make sense to also add support for registering initial values here? i.e. the initial contexts, locations and records?

It could, but it could also make sense not to do it. The current content of struct bt_bap_pacs_register_param strictly necessary to run the service, and they cannot easily be changed, in fact they would need a reregistration to change.

Contexts, locations, and records can more easily change at runtime.

I would prefer to keep it as is for now - but if you really want this change Im happy to create an issue for future change.

Sure :) It makes sense to keep as is, with the caveat that the default values are invalid, so the user shall call the other functions afterwards (similar as they have to today)

samples/bluetooth/bap_broadcast_sink/src/main.c

samples/bluetooth/bap_unicast_server/src/main.c

subsys/bluetooth/audio/pacs.c

doc/releases/migration-guide-4.1.rst

subsys/bluetooth/audio/pacs.c

include/zephyr/bluetooth/audio/pacs.h

Thalley · 2025-01-20T10:31:35Z

include/zephyr/bluetooth/audio/pacs.h

+ * @return 0 in case of success or negative value in case of error.
+ */
+int bt_pacs_register(const struct bt_bap_pacs_register_param *param);
+
+/**
+ * @brief Unregister the Published Audio Capability Service instance.
+ *
+ * @return 0 in case of success or negative value in case of error.
+ */


If possible, please use @retval instead of @return and specific the possible return values. It makes it easier for applications to know exactly which return values can be expected, and why

I prefer to keep it as is, as it will forward error codes from other functions.

I prefer to keep it as is, as it will forward error codes from other functions.

That's one of the reasons to use retval: Simply forwarding error will make it harder to handle cases as an application.

We do not have any requirement in the coding guidelines for this, so I won't block the PR on this

samples/bluetooth/cap_acceptor/src/main.c

samples/bluetooth/tmap_peripheral/src/bap_unicast_sr.c

tests/bsim/bluetooth/audio/src/pbp_public_broadcast_sink_test.c

subsys/bluetooth/audio/pacs.c

doc/releases/migration-guide-4.1.rst

include/zephyr/bluetooth/audio/pacs.h

Thalley · 2025-01-20T12:42:02Z

include/zephyr/bluetooth/audio/pacs.h

+ * @return 0 in case of success or negative value in case of error.
+ */
+int bt_pacs_register(const struct bt_bap_pacs_register_param *param);
+
+/**
+ * @brief Unregister the Published Audio Capability Service instance.
+ *
+ * @return 0 in case of success or negative value in case of error.
+ */


I prefer to keep it as is, as it will forward error codes from other functions.

That's one of the reasons to use retval: Simply forwarding error will make it harder to handle cases as an application.

We do not have any requirement in the coding guidelines for this, so I won't block the PR on this

subsys/bluetooth/audio/pacs.c

Thalley · 2025-01-21T13:57:51Z

include/zephyr/bluetooth/audio/pacs.h

+	bool snk_pac;
+#endif /* CONFIG_BT_PAC_SNK */
+
+#if defined(CONFIG_BT_PAC_SNK_LOC)
+	/**
+	 * @brief Enables or disables registration of Sink Location Characteristic.
+	 *
+	 * Registration of Sink Location is dependent on @ref bt_bap_pacs_register_param.snk_pac
+	 * also being set.
+	 */
+	bool snk_loc;


Hmm, I'm starting to wonder if we should add chrc or attr to these names? Or prefix them with register_?

If we ever want to expand bt_bap_pacs_register_param to also contain default values, snk_pac and snk_loc are the obvious names for the default sink PAC record and sink location

Don't you think we should make those name changes in such a patch where it becomes relevant then?

Possibly. At least while the API is still unstable we can more easily do that, but will be much harder to do later

Added option to set the PACS Characteristics through the bap API, making PACS configuration runtime available. Source and Sink PAC, as well as Source/Sink PAC Location is can be set through a register function in the PACS api. Signed-off-by: Fredrik Danebjer <frdn@demant.com>

larsgk

LGTM - should we have a before/after RAM/ROM usage check?

Thalley · 2025-01-22T14:19:41Z

LGTM - should we have a before/after RAM/ROM usage check?

Would be nice if you can provide that @fredrikdanebjer :) Just so that we are aware of what I assume will be a memory increase

fredrikdanebjer · 2025-01-22T14:51:15Z

LGTM - should we have a before/after RAM/ROM usage check?

Would be nice if you can provide that @fredrikdanebjer :) Just so that we are aware of what I assume will be a memory increase

Before pacs.c:
724/5394 RAM/ROM
After pacs.c:
1116/6104 RAM/ROM

Built with audio_shell using:
west build -b nrf5340_audio_dk/nrf5340/cpuapp -d build/nrf5340audiodk/audio_shell tests/bluetooth/shell/ --pristine -- -DCONF_FILE=audio.conf -DOVERLAY_CONFIG=boards/nrf5340_audio_dk_nrf5340_cpuapp.conf

You can find the full reports attached.

post_ram.txt
post_rom.txt
pre_ram.txt
pre_rom.txt

Thalley · 2025-01-23T08:31:05Z

Before pacs.c:
724/5394 RAM/ROM
After pacs.c:
1116/6104 RAM/ROM

Alright, so that's quite a significant change. I guess it's the same for the others.

400 more RAM sounds like a lot. Is it mainly the pacs_attr array?

fredrikdanebjer · 2025-01-27T05:36:21Z

@Thalley Yes, pacs_attrs make up of 380 bytes. As a reference for other services we did this for:
ascs_attr 320 bytes
has_attrs 200 bytes
vcs_attrs 220 bytes

fredrikdanebjer force-pushed the dyn_pacs branch 11 times, most recently from 523edd9 to 63351e3 Compare January 14, 2025 08:04

fredrikdanebjer marked this pull request as ready for review January 14, 2025 09:47

zephyrbot added area: Samples Samples area: Bluetooth Audio area: Bluetooth Release Notes To be mentioned in the release notes labels Jan 14, 2025

zephyrbot requested review from asbjornsabo, babrsn, Casper-Bonde-Bose, fabiobaltieri, jhedberg, jthm-ot, kartben, larsgk, MariuszSkamra, nashif, niym-ot, pin-zephyr, sjanc and Thalley January 14, 2025 09:47

zephyrbot assigned Thalley Jan 14, 2025

jthm-ot reviewed Jan 17, 2025

View reviewed changes

Thalley requested changes Jan 17, 2025

View reviewed changes

fredrikdanebjer force-pushed the dyn_pacs branch 2 times, most recently from 756ef02 to eebe150 Compare January 20, 2025 05:44

fredrikdanebjer requested review from Thalley and jthm-ot January 20, 2025 07:42

Thalley reviewed Jan 20, 2025

View reviewed changes

doc/releases/migration-guide-4.1.rst Outdated Show resolved Hide resolved

subsys/bluetooth/audio/pacs.c Outdated Show resolved Hide resolved

Thalley requested changes Jan 20, 2025

View reviewed changes

fredrikdanebjer force-pushed the dyn_pacs branch from eebe150 to 8812707 Compare January 20, 2025 12:12

fredrikdanebjer requested a review from Thalley January 20, 2025 12:12

Thalley requested changes Jan 20, 2025

View reviewed changes

Thalley reviewed Jan 21, 2025

View reviewed changes

subsys/bluetooth/audio/pacs.c Outdated Show resolved Hide resolved

subsys/bluetooth/audio/pacs.c Outdated Show resolved Hide resolved

fredrikdanebjer force-pushed the dyn_pacs branch from 8812707 to 972f130 Compare January 21, 2025 08:58

Thalley reviewed Jan 21, 2025

View reviewed changes

subsys/bluetooth/audio/pacs.c Outdated Show resolved Hide resolved

fredrikdanebjer force-pushed the dyn_pacs branch 2 times, most recently from a76a429 to c9b2e77 Compare January 21, 2025 13:52

Thalley reviewed Jan 21, 2025

View reviewed changes

fredrikdanebjer force-pushed the dyn_pacs branch 2 times, most recently from bba57c3 to d48096c Compare January 22, 2025 08:05

fredrikdanebjer force-pushed the dyn_pacs branch from d48096c to 172fd65 Compare January 22, 2025 12:04

Thalley approved these changes Jan 22, 2025

View reviewed changes

larsgk approved these changes Jan 22, 2025

View reviewed changes

kartben merged commit 21b5f0c into zephyrproject-rtos:main Jan 22, 2025
27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: pacs: Add dynamic PACS registration #83730

Bluetooth: pacs: Add dynamic PACS registration #83730

fredrikdanebjer commented Jan 9, 2025

jthm-ot Jan 17, 2025

Thalley Jan 17, 2025

fredrikdanebjer Jan 17, 2025

Thalley Jan 20, 2025

Thalley Jan 20, 2025

fredrikdanebjer Jan 20, 2025

Thalley Jan 20, 2025

Thalley Jan 20, 2025

Thalley Jan 21, 2025

fredrikdanebjer Jan 22, 2025

Thalley Jan 22, 2025

larsgk left a comment

Thalley commented Jan 22, 2025

fredrikdanebjer commented Jan 22, 2025 •

edited

Loading

Thalley commented Jan 23, 2025

fredrikdanebjer commented Jan 27, 2025

Bluetooth: pacs: Add dynamic PACS registration #83730

Bluetooth: pacs: Add dynamic PACS registration #83730

Conversation

fredrikdanebjer commented Jan 9, 2025

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

larsgk left a comment

Choose a reason for hiding this comment

Thalley commented Jan 22, 2025

fredrikdanebjer commented Jan 22, 2025 • edited Loading

Thalley commented Jan 23, 2025

fredrikdanebjer commented Jan 27, 2025

fredrikdanebjer commented Jan 22, 2025 •

edited

Loading