Bluetooth: Audio: PAC location refactor #42593

Thalley · 2022-02-08T16:45:12Z

The location values in PACs were build-time initiated and unchangable by the server itself.
This PR adds support for changing the value in the server, as the value will live in the server (currently implemented in the capabilities.c layer), instead of in the service.

This also adds notification for the value when it is changed, by either the server or a remove client.

fixes #42342
fixes #42343

Thalley · 2022-02-08T16:45:24Z

Depends on #42314

Casper-Bonde-Bose · 2022-03-01T10:23:44Z

include/bluetooth/audio/audio.h

+enum bt_audio_location {
+	BT_AUDIO_LOCATION_FRONT_LEFT = BIT(0),
+	BT_AUDIO_LOCATION_FRONT_RIGHT = BIT(1),
+	BT_AUDIO_LOCATION_FRONT_CENTER = BIT(2),
+	BT_AUDIO_LOCATION_LOW_FREQ_EFFECTS_1 = BIT(3),
+	BT_AUDIO_LOCATION_BACK_LEFT = BIT(4),
+	BT_AUDIO_LOCATION_BACK_RIGHT = BIT(5),
+	BT_AUDIO_LOCATION_FRONT_LEFT_OF_CENTER = BIT(6),
+	BT_AUDIO_LOCATION_FRONT_RIGHT_OF_CENTER = BIT(7),
+	BT_AUDIO_LOCATION_BACK_CENTER = BIT(8),
+	BT_AUDIO_LOCATION_LOW_FREQ_EFFECTS_2 = BIT(9),
+	BT_AUDIO_LOCATION_SIDE_LEFT = BIT(10),
+	BT_AUDIO_LOCATION_SIDE_RIGHT = BIT(11),
+	BT_AUDIO_LOCATION_TOP_FRONT_LEFT = BIT(12),
+	BT_AUDIO_LOCATION_TOP_FRONT_RIGHT = BIT(13),
+	BT_AUDIO_LOCATION_TOP_FRONT_CENTER = BIT(14),
+	BT_AUDIO_LOCATION_TOP_CENTER = BIT(15),
+	BT_AUDIO_LOCATION_TOP_BACK_LEFT = BIT(16),
+	BT_AUDIO_LOCATION_TOP_BECK_RIGHT = BIT(17),
+	BT_AUDIO_LOCATION_TOP_SIDE_LEFT = BIT(18),
+	BT_AUDIO_LOCATION_TOP_SIDE_RIGHT = BIT(19),
+	BT_AUDIO_LOCATION_TOP_BACK_CENTER = BIT(20),
+	BT_AUDIO_LOCATION_BOTTOM_FRONT_CENTER = BIT(21),
+	BT_AUDIO_LOCATION_BOTTOM_FRONT_LEFT = BIT(22),
+	BT_AUDIO_LOCATION_BOTTOM_FRONT_RIGHT = BIT(23),
+	BT_AUDIO_LOCATION_FRONT_LEFT_WIDE = BIT(24),
+	BT_AUDIO_LOCATION_FRONT_RIGHT_WIDE = BIT(25),
+	BT_AUDIO_LOCATION_LEFT_SURROUND = BIT(26),
+	BT_AUDIO_LOCATION_RIGHT_SURROUND = BIT(27),
+};
+


Even though it makes sense to group these together, I do not think using an enum is a good way to do it.
Enums contains a finite number of possible values - hence you would need to suppress compiler warnings to be able to OR multiple values together - or you would need to typecast the result back to the enum type - which could result in errors if the compiler have made assumptions about the finite number of values (for example a switch statement do not need a default case when doing switch on an enum type and all possible cases are handled). I do not know if newer c-versions have support for this though?
I have seen others define a type and then typecast to this in the define:

typedef uint32_t bt_audio_loc_t
#define BT_AUDIO_LOCATION_FRONT_LEFT ((bt_audio_loc_t) BIT(0))

then you can use bt_audio_loc_t rather than uint32_t for function parameters that expect audio location(s). And you can freely OR multiple together as the result would still be uint32_t.

To group them together in the API documentation use the @name Audio Locations @{ ... @} grouping feature

I hear your concerns and see what issues this may cause.

The reasoning behind this approach is:

(named) enums provide an easy way for an API to express the "group" of values expected, without relying on doxygen-specific grouping

https://www.kernel.org/doc/html/v4.10/process/coding-style.html#macros-enums-and-rtl (which we follow) express that enums are preferred for groups of constants

This is the approach (although without the name) used by e.g. BT_LE_ADV_OPT_, and other option enums in bluetooth.h.

If we see compiler issues, then this is indeed something we need to fix. If the issue is caused by using the enum type in the APIs, then we can replace the enum type with uint32_t (similar to how the BT_LE_ADV_OPT_ are used), and simply add a reference to the enum in the function description.

If the issue is caused by just using the enum keyword, then this problem exist in many places in the code base, and we need a general fix as well as an exception to the Linux kernel code guideline.

Do you have any references to the compiler issues (warnings) you are experiencing that I can read?

If this is an accepted way to do it in Zephyr, I'm OK with it.
I do not have a reference, but in my early days I was told to refactor a full FM radio API I made due to this issue (not a GCC based compiler) - hence I always try to avoid using enums when I specify bit-values.
I had a look at the different enum related warnings that GCC support, and non of them covers this issue, hence I think we can go ahead using enums in this case.

Otherwise refactoring to a group of #defines can be done without breaking the API if we just change the API from enum bt_audio_location to uint32_t bt_audio_location.

Casper-Bonde-Bose · 2022-03-01T10:24:15Z

subsys/bluetooth/audio/CMakeLists.txt

@@ -46,7 +46,7 @@ zephyr_library_sources_ifdef(CONFIG_BT_ASCS ascs.c)
 zephyr_library_sources_ifdef(CONFIG_BT_PACS pacs.c)
 zephyr_library_sources_ifdef(CONFIG_BT_AUDIO_STREAM stream.c)
 zephyr_library_sources_ifdef(CONFIG_BT_AUDIO_UNICAST_SERVER unicast_server.c)
-zephyr_library_sources_ifdef(CONFIG_BT_AUDIO_CAPABILITIES capabilities.c)
+zephyr_library_sources_ifdef(CONFIG_bt_audio_capability capabilities.c)


Wrong renaming I think?
Same below

Casper-Bonde-Bose

LGTM

Casper-Bonde-Bose

LGTM

Thalley · 2022-03-10T11:56:54Z

@asbjornsabo @MariuszSkamra @jhedberg This is blocking other development, will one (or more) of you please review?

include/bluetooth/audio/audio.h

asbjornsabo · 2022-03-10T14:22:53Z

subsys/bluetooth/audio/pacs.c

+
+	if (unicast_server_cb == NULL ||
+	    unicast_server_cb->publish_location == NULL) {
+		BT_WARN("Not callback for publish_location");


Not -> No?
(Other places as well)

asbjornsabo · 2022-03-10T14:39:49Z

subsys/bluetooth/audio/pacs.c

+	}
+
+	return bt_gatt_attr_read(conn, attr, buf, len, offset,
+				 &sys_cpu_to_le32(location_32),


Doesn't the result have to be assigned to a variable before applying the & operator (so that the operand is an "lvalue")?
(Also elsewhere)

i.e. if we need to do something like

uint32_t le_location = sys_cpu_to_le32(location_32); return bt_gatt_attr_read(conn, attr, buf, len, offset, &le_location, sizeof(le_location));

instead?

I actually do not really know. It doesn't throw any errors.

Keep in mind that sys_cpu_to_le32 is a macro, and for LE it will be

#define sys_cpu_to_le32(val) (val)

and

#define sys_cpu_to_le32(val) __bswap_32(val)

for BE. Not sure if this will give an error for BE, and I am not sure how I can test it.

I tried to use the same thing in some code here, and I get compile errors for both variants.
In my understanding, this should not work - I think one ends up with a value, not a variable, and one can take take the address of a value.
Will check again - I may not have tested correctly.

Tested again. This time it worked for the "le" variant, but fails for the "be" variant.

uint32_t location; uint32_t *p; location = 7; p = &sys_cpu_to_be32(location);

gives
error: lvalue required as unary '&' operand
324 | p = &sys_cpu_to_be32(location);
| ^

MariuszSkamra · 2022-03-10T15:29:52Z

include/bluetooth/audio/audio.h

+/** @brief Audio Capability type */
+enum bt_audio_pac_type {
+	BT_AUDIO_SINK = 0x01,
+	BT_AUDIO_SOURCE = 0x02,
+};
+


I know you move the code only, but I saw this is used for ASE direction as well. Maybe the enum name should be different then.

I agree. Do you have any suggestions? bt_audio_type seems a bit too generic for my taste. bt_audio_ep_type perhaps?

Let's fix this in a new issue: #43693

MariuszSkamra · 2022-03-10T15:44:16Z

include/bluetooth/audio/audio.h

+	 *
+	 *  Publish location callback is called whenever a remote client
+	 *  requests to read the Published Audio Capabilities (PAC) location,
+	 *  or if the location needs to be notified.
+	 *
+	 *  @param[in]  conn      The connection that requests the location.
+	 *                        Will be NULL if the location is requested
+	 *                        for sending a notification, as a result of
+	 *                        callling
+	 *                        bt_audio_unicast_server_location_changed().
+	 *  @param[in]  type      Type of the endpoint.
+	 *  @param[out] location  Pointer to the location that needs to be set.
+	 *
+	 *  @return 0 in case of success or negative value in case of error.
+	 */
+	int (*publish_location)(struct bt_conn *conn,
+				enum bt_audio_pac_type type,
+				enum bt_audio_location *location);
+
+	/** @brief Write location callback
+	 *
+	 *  Write location callback is called whenever a remote client
+	 *  requests to write the Published Audio Capabilities (PAC) location.
+	 *
+	 *  @param conn      The connection that requests the write.
+	 *  @param type      Type of the endpoint.
+	 *  @param location  The location being written.
+	 *
+	 *  @return 0 in case of success or negative value in case of error.
+	 */
+	int (*write_location)(struct bt_conn *conn, enum bt_audio_pac_type type,
+			      enum bt_audio_location location);


Why the location cannot be stored internally?

It can, however PACS was (is) designed to be stateless, i.e. all the values are located in an upper layer.

It would have been easy to store the locaiton in PACS and still keep e.g. the capabilities in an upper layer, but I did not want to mix-match where the service data/states were stored, so used the same approach for the location as we have for the other values in PACS.

This of course is a more complex implementation and possibly erroneous as the notifications when the location is changed is no longer ensured by the stack.

We have yet to decided what final approach we want to take for the LE Audio services, as both approaches have their pros and cons.

I believe an application can change the value run-time - hence I think this implementation is how it should be.
But reading the description again I think:
* Write location callback is called whenever a remote client
* requests to write the Published Audio Capabilities (PAC) location.
should be:
* Write location callback is called whenever a remote client
* requests to read the Published Audio Capabilities (PAC) location.

PAC's are read only and my assumption was that the callback was called write as the application (PAC server) needs to write the value that the client wants to read?

@Casper-Bonde-Bose No, the write description is correct. The Sink and Source location characteristics are (optionally) writeable:

I see - Maybe a feature to be able to configure a speaker to be at a specific location?

Possibly, yes. We (@asbjornsabo and I) raised this in the GAWG some time ago, and it seems that in hindsight it shouldn't have been writable.

Refactor the PAC location read and write. Instead of storing the location in the service, the location is now stored in the application, and is retrieved by the service via callbacks. Similarly, if a client writes the location, this request is being sent to the application. Signed-off-by: Emil Gydesen <[email protected]>

Thalley · 2022-03-11T11:24:30Z

Re-pushed to fix a bad merge conflict fix

MariuszSkamra

LGTM

asbjornsabo

My comments have been addressed.

github-actions bot added area: API Changes to public APIs area: Bluetooth area: Bluetooth Audio area: Tests Issues related to a particular existing or missing test labels Feb 8, 2022

Thalley force-pushed the pac_location_update branch 5 times, most recently from f07e161 to d07b820 Compare February 9, 2022 14:13

Thalley force-pushed the pac_location_update branch 3 times, most recently from 5fae16d to 0127753 Compare February 23, 2022 08:57

Casper-Bonde-Bose reviewed Mar 1, 2022

View reviewed changes

Thalley force-pushed the pac_location_update branch from 0127753 to 894bc92 Compare March 1, 2022 10:32

Casper-Bonde-Bose previously approved these changes Mar 1, 2022

View reviewed changes

Thalley dismissed Casper-Bonde-Bose’s stale review via b6ebfe1 March 9, 2022 11:15

Thalley force-pushed the pac_location_update branch from 894bc92 to b6ebfe1 Compare March 9, 2022 11:15

Thalley marked this pull request as ready for review March 9, 2022 11:15

Thalley requested review from alwa-nordic, jhedberg, Vudentz, aescolar, wopu-ot and asbjornsabo as code owners March 9, 2022 11:15

Thalley force-pushed the pac_location_update branch from b6ebfe1 to 10186d8 Compare March 9, 2022 11:16

Thalley requested a review from Casper-Bonde-Bose March 9, 2022 13:35

Casper-Bonde-Bose previously approved these changes Mar 10, 2022

View reviewed changes

asbjornsabo requested changes Mar 10, 2022

View reviewed changes

Thalley dismissed Casper-Bonde-Bose’s stale review via d4324fa March 10, 2022 15:25

Thalley force-pushed the pac_location_update branch from 10186d8 to d4324fa Compare March 10, 2022 15:25

Thalley requested a review from asbjornsabo March 10, 2022 15:25

MariuszSkamra reviewed Mar 10, 2022

View reviewed changes

Casper-Bonde-Bose previously approved these changes Mar 10, 2022

View reviewed changes

Thalley requested a review from MariuszSkamra March 11, 2022 09:29

Thalley dismissed Casper-Bonde-Bose’s stale review via adfdca2 March 11, 2022 10:41

Thalley force-pushed the pac_location_update branch from d4324fa to adfdca2 Compare March 11, 2022 10:41

Thalley requested a review from Casper-Bonde-Bose March 11, 2022 10:43

Casper-Bonde-Bose previously approved these changes Mar 11, 2022

View reviewed changes

Thalley dismissed Casper-Bonde-Bose’s stale review via 19f6c24 March 11, 2022 11:21

Thalley force-pushed the pac_location_update branch from adfdca2 to 19f6c24 Compare March 11, 2022 11:21

Casper-Bonde-Bose approved these changes Mar 11, 2022

View reviewed changes

MariuszSkamra approved these changes Mar 11, 2022

View reviewed changes

asbjornsabo approved these changes Mar 11, 2022

View reviewed changes

mbolivar-nordic merged commit 1846653 into zephyrproject-rtos:main Mar 11, 2022

Thalley deleted the pac_location_update branch April 5, 2022 11:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: Audio: PAC location refactor #42593

Bluetooth: Audio: PAC location refactor #42593

Thalley commented Feb 8, 2022

Thalley commented Feb 8, 2022

Casper-Bonde-Bose Mar 1, 2022

Thalley Mar 1, 2022

Casper-Bonde-Bose Mar 1, 2022

Thalley Mar 1, 2022

Casper-Bonde-Bose Mar 1, 2022

Casper-Bonde-Bose left a comment

Casper-Bonde-Bose left a comment

Thalley commented Mar 10, 2022

asbjornsabo Mar 10, 2022

asbjornsabo Mar 10, 2022

Thalley Mar 10, 2022

asbjornsabo Mar 11, 2022

asbjornsabo Mar 11, 2022

Thalley Mar 11, 2022

MariuszSkamra Mar 10, 2022

Thalley Mar 11, 2022

Thalley Mar 11, 2022

MariuszSkamra Mar 10, 2022

Thalley Mar 11, 2022

Casper-Bonde-Bose Mar 11, 2022

Thalley Mar 11, 2022

Casper-Bonde-Bose Mar 11, 2022

Thalley Mar 11, 2022

Thalley commented Mar 11, 2022

MariuszSkamra left a comment

asbjornsabo left a comment

Bluetooth: Audio: PAC location refactor #42593

Bluetooth: Audio: PAC location refactor #42593

Conversation

Thalley commented Feb 8, 2022

Thalley commented Feb 8, 2022

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

Casper-Bonde-Bose left a comment

Choose a reason for hiding this comment

Casper-Bonde-Bose left a comment

Choose a reason for hiding this comment

Thalley commented Mar 10, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thalley commented Mar 11, 2022

MariuszSkamra left a comment

Choose a reason for hiding this comment

asbjornsabo left a comment

Choose a reason for hiding this comment