Bluetooth LE Audio Service APIs

[Thalley]

A while ago on Slack (where we unfortunately lost the history) we had a discussion on the APIs for the upcoming Bluetooth Audio service specifications. One such specification (VOCS) implementation was recently merged (#31893) with an API, which may not be optimal.

What I would like to do here, is open the API discussion and use the VOCS implementation as a reference as how we should design this and some of the other upcoming service implementations.

The API that was merged was based on the idea that we will have a shared API between server and client, and that the struct bt_conn *conn value decided whether it was a local operation (if it was null) or if it was a remote operation (non-null). Example:

/**
 * @brief Read the Volume Offset Control Service location.
 *
 * The value is returned in the bt_vocs_cb.location callback.
 *
 * @param conn          Connection to peer device, or NULL to read local server value.
 * @param inst          Pointer to the Volume Offset Control Service instance.
 *
 * @return 0 on success, GATT error value on fail.
 */
int bt_vocs_location_get(struct bt_conn *conn, struct bt_vocs *inst);

/**
 * @brief Set the Volume Offset Control Service location.
 *
 * @param conn          Connection to peer device, or NULL to read local server value.
 * @param inst          Pointer to the Volume Offset Control Service instance.
 * @param location      The location to set.
 *
 * @return 0 on success, GATT error value on fail.
 */
int bt_vocs_location_set(struct bt_conn *conn, struct bt_vocs *inst, uint32_t location);

One comment regarding this from @joerchan was that it was not clear when it was a client (remote) operation or when it was a server (local) operation, and his suggestion was to have something like

int bt_vocs_location_read(struct bt_conn *conn, struct bt_vocs *inst);
int bt_vocs_location_get(struct bt_vocs *inst);
int bt_vocs_location_write(struct bt_conn *conn, struct bt_vocs *inst);
int bt_vocs_location_set(struct bt_vocs *inst);

i.e using read/get and write/set to differentiate between client/remote and server/local operations. The biggest downside of this is that it will effectively double the size of the APIs as we will need two function for each action (one for each role). For VOCS it isn't that bad, but we will have other implementation (such as TBS/CCP) with 30+ functions that will effectively become 60+ for a single service.

Another approach that I've considered myself is to get rid of the connection pointer in the API, and simply rely on the instance pointer, so something like:

int bt_vocs_location_get(struct bt_vocs *inst);
int bt_vocs_location_set(struct bt_vocs *inst);

As the instance is either a local instance (returned from init) or a remote instance (returned from doing discovery), but I don't feel like that will help with the confusion that @joerchan pointed out. A solution to that could be to expand the API to be

int bt_vocs_location_get(struct bt_vocs *inst);
int bt_vocs_location_set(struct bt_vocs *inst);
int bt_vocs_client_location_get(struct bt_vocs *inst);
int bt_vocs_client_location_set(struct bt_vocs *inst);

Which will return us to having a larger number of API functions.

The point of this discussion is how we can design and API for these GATT services that makes sense for everyone.
Things to consider:

1. Some services may appear more than once on a server
2. Some service may include other services
3. Some services do not have a corresponding profile specification
4. The API needs to handle multiple connections
5. We may not want to expose read/write for some characteristics for some profiles/services

Also keep in mind that some profiles/services for LE Audio is not public yet. We can freely discuss AICS, VOCS, VCS, VCP, TBS, CCP, MICS, MICP, MCS and MCP (see https://www.bluetooth.com/specifications/specs/)

[Abildgren]

I fully see your point @Thalley. @joerchan proposal with a get/set for local and read/write for remote operations requires that this (get/set/read/write) is the convention all over the rest of the Bluetooth API which it is not. Take for example bt_gatt_attr_read. This also allows for a local connection point to do local database storing

If we go with the bt_vocs_location_get(struct bt_vocs *inst), what would then happen if the function was supplied with an inst which was not local?

To me, the most unambiguous is to allow the conn handle to be local as well, and in that way make it clear to the user where the action is taking place. And it is in line with the GATT API in zephyr as well, so it is not a new concept.

[Thalley]

If we go with the bt_vocs_location_get(struct bt_vocs *inst), what would then happen if the function was supplied with an inst which was not local?

Assuming that it is a valid pointer, then that would trigger a remote read. The instance contains a pointer to a connection to the remote device, as it would have been from a discovery procedure. Worst case would be that it return an error stating that it isn't connected.

[asbjornsabo]

Frankly, I thought this was agreed and settled half a year ago. But let us see if we can reach a broader consensus this time.

As for the proposed "shared" client/server API, that seems very workable to me. It is well documented that for the local interface, give a NULL pointer. So for using the API (writing code), this should be clear. For reading code, local/remote should be clear from context and the parameters given. (I.e., when "NULL" is given as the connection parameter, this is local use.)

The doubling of the API is indeed significant, some of the services have a large number of characterstics. Media control is one of the large ones in this respect, the client header filer currently has around 33 function calls and 38 callbacks (OTC use included). The media player has 46 functions/callbacks.

That being said, media control is the single module in le-audio that is written so that one side (the media player) may be only local (no Bluetooth). So here a local-only interface could possibly actually make sense for some cases.

[joerchan]

Sorry for kicking the beehive on this one, I guess I didn't understand the full scope of this when we discussed this last time, or now when I made the comment on the VOCS PR.

My understanding from the last discussion was that instead of having bt_vocs_server and bt_vocs_client API's we would put all of them under bt_vocs, and then the name would signal which role is using the API, since read/write is client, and notify is server.
I wasn't expecting APIs to serve in both roles. Not saying that I'm against this, it just wasn't something that I considered.

The main reason why I reacted to the bt_vocs_location_get function was that it was in turn calling the internal bt_vocs_client_location_get. Based on my previous understanding of how we had agreed upon this APIs this should have been bt_vocs_location_get for server and bt_vocs_location_read for client.

To me it seems like there is just as many functions, with the difference being how many are exposed as public ones. And then you need to call them differently.

If the argument for using the doubling of the APIs is to limit the amount of public APIs that is exposed to the application then that is OK with me. It is hard to see the scope of the APIs based on a single PR here and it appears to be quite a lot of these services.
When I only looked at VOCS then my suggestion made sense to me.

Now i've stated my reasoning I'll say that I'm happy to go with either suggestion now that I have a better understand of the reasoning behind these API decisions.

[Thalley]

The number of API functions shouldn't really be a blocker, but it is something to consider. I'm not convinced that stating read => client operation and get => server operation is clear for everyone either.

If we can be consistent across the entire Bluetooth API with that terminology, that would work I suppose.

[Vudentz]

I think the general problem I see with this approach of creating services APIs as a kind of helper library is that it gives the impression that all service would be implemented at application layer, rather than in the stack itself or in a driver. Taking VOCS as example, the stack could implement it directly and just fetch the values for elsewhere, that way one wouldn't need any APIs for the service itself just the client procedures, well if there are any client procedures defined.

For MCS and VCS I believe we may need to allow registration of external capabilties, much like bt_audio_capability, so that in itself is the API where the user can provide fields and callbacks to the stack in a pull style rather than expecting the upper layer to push individual fields since that makes the implementation much more predictable and probably easier to qualify different implementation as it is just a matter of changing the capabilities provider.

[Thalley]

Taking VOCS as example, the stack could implement it directly and just fetch the values for elsewhere, that way one wouldn't need any APIs for the service itself just the client procedures, well if there are any client procedures defined.

I am not sure I completely followed that sentence. Can you elaborate? Regarding client procedures, there are none for VOCS specifically. For VCS there are some defined in VCP, but the only reason why there even is a VCP is because of the change counter iirc.

For MCS and VCS I believe we may need to allow registration of external capabilties, much like bt_audio_capability, so that in itself is the API where the user can provide fields and callbacks to the stack in a pull style rather than expecting the upper layer to push individual fields since that makes the implementation much more predictable and probably easier to qualify different implementation as it is just a matter of changing the capabilities provider.

You've mentioned this before, and is an interesting way of implementing this. I have some issues fully understanding the reasoning, what this would accomplish and how that API should be implemented. Can you perhaps give an example of what you are thinking here?

[Thalley]

One thing to keep in mind with these services may/can be used outside of LE audio streams, such a remote control for a e.g. speaker.

[Thalley]

Another thing to consider is how writes to the services should be done. From #32785 (comment):

From @Vudentz

If I get how this is working the new states are notified after the operation which can never fail, while this might work for testing Id expect the entity handling these settings would have to be requested to accept the operation only then we can confirm the operation has been performed and if there is an error report back.

From @Thalley

Hmm, if I understand you correctly, you'd expect the AICS to request the change towards the application, rather than telling the application that the change was made?

That's an interesting alternative. I don't really have a strong opinion about this. The request for change is how it's implemented in e.g. OTS (see ots.c:192), but other characteristics such as the device name are silently accepted without the application's approval (see gatt.c:111).

So basically we have two different ways of handling writes from the client:

Ask the application for permission to finalize the write and either accept or reject it
Tell the application that a write has happened.
Currently we seem to employ both in different services.

I think this is an interesting discussion to have, and I'd like to bring in @asbjornsabo , @jhedberg and @joerchan as well, although I think this is outside the scope of this particular PR.

[Vudentz]

From @Thalley

Hmm, if I understand you correctly, you'd expect the AICS to request the change towards the application, rather than telling the application that the change was made?
That's an interesting alternative. I don't really have a strong opinion about this. The request for change is how it's implemented in e.g. OTS (see ots.c:192), but other characteristics such as the device name are silently accepted without the application's approval (see gatt.c:111).

But this is exactly the problem here, we set the name there because the resource is part of the Bluetooth stack so we can change internally and not involve any other entity, but in case of audio the resource is likely handle somewhere else, so a change to lets say mute volume will have to reach the speaker/microphone driver to apply the change and only then we respond, otherwise we have to duplicate the mute setting at service layer since driver are likely going to track this separately and then synchronize them somehow, which perhaps it is the intend with these APIs but the callback also don't have any return to be able to tell if the values have been properly propagaded.

[asbjornsabo]

Some thoughts on API

Requirements, or possible requirements

• Ability to serve incoming requests from remote
• Ability to serve local requests - and without having to set up a GATT service (if only local control is wanted)
• Ability to send requests to remote peer
• Ability to add/register instances (or implementations) of functionality
• Same API for doing local requests and remote requests

Or, put another way, like I thought for Media Control:

• Separation between the GATT service and the functionality
  • The GATT service shall be an interface to the functionality, not contain the functionality
  • This makes the functionality easily replaceable without modifying the service
  • This allows the functionality to work without the service in place (e.g. for local control)
  • In practice: The Media Control Service calls out the the media player for the actual functionality.

From talk with @cvinayak (hopefully I understood him correctly):
In the case of the same API for local and remote calls:

• local and remote calls should either both use callbacks, or both use return value.
• using return value will cause the application to wait (for remote calls), which may be what we want (use the OS).
• if using callbacks, ensure that both local and remote callbacks come from the same context/thread, so that the user knows how to set up his stack.

From talk with @Thalley:

• If much state is kept in the application (=outside of the service), that makes qualification more difficult.

So I propose a system along the lines of what is shown in https://github.com/asbjornsabo/zephyr-bt-sig/blob/api/subsys/bluetooth/host/audio/msc/api_proposal.svg

Here, there is

• functionality controllers
  • application for local control
  • GATT service for control from remote peer
• functionality provider instance(s)
  • "plug-able" and independent of the service/stack
• A functionality control/access module that acts as the connection point between the controllers

A minimal sample header file for the "middle-man" module with some additional explanation is found in https://github.com/asbjornsabo/zephyr-bt-sig/blob/api/subsys/bluetooth/host/audio/mplc.h

This allows multiple functionality instances, and decouples these from the stack.
It also allows local control, remote control or both.
It can be extended to do local and remote requests with the same API, if the client is hooked up to the middle-man module (see alternatives on the left in the figure).

I think that in this proposal, we should be able to follow much of what Emil has been doing for the VCS/VOCS/AICS API, while at the same time adressing my concern for separating the server interface from the functionality and Luiz's wish to be able to handle things outside of the stack.

[asbjornsabo]

I think your thoughts on qualification (2,3) are sound. If a user writes their own media player, or volume renderer, those would not be covered by an existing Zephyr qualification.

As for (4), I agree that some adaptation layer may be required to match e.g. an independent player to the Media Control Server specification. I think this is possible to do - some companies are already doing this (not in Zephyr, but elsewhere). And the Media Control Spec has been relaxed on several points explicitly to adapt to existing players. (But, unless I misunderstand you, I think this layer will be "in between" rather than "above".)

[Thalley]

@asbjornsabo

(But, unless I misunderstand you, I think this layer will be "in between" rather than "above".)

I would expect the layers to be something like
GATT <-> MCS <-> Bluetooth Media Player <-> Media Player <-> Application

Where (GATT <-> MCS <-> Bluetooth Media Player) is what needs to be qualified according to the Bluetooth SIG.

The type of API between each layer doesn't need to be the same either, e.g. the API between GATT and MCS are callback based, but the API between MCS and Bluetooth Media Player can be synchronous.

[Vudentz]

@asbjornsabo

(But, unless I misunderstand you, I think this layer will be "in between" rather than "above".)

I would expect the layers to be something like
GATT <-> MCS <-> Bluetooth Media Player <-> Media Player <-> Application

Where (GATT <-> MCS <-> Bluetooth Media Player) is what needs to be qualified according to the Bluetooth SIG.

The type of API between each layer doesn't need to be the same either, e.g. the API between GATT and MCS are callback based, but the API between MCS and Bluetooth Media Player can be synchronous.

That is all fine if such layer are all instantiated by the application, so much like in GATT we don't need to reserve memory for services, the application would instantiate the Media Player which internally instantiate MCS instance, etc, since anyway we don't have support for statically defined GATT services instances but if we ever want to change that perhaps there should be a concept of static Media Player as well, anyway I think we are getting ahead of ourselves.

Regarding the synchronous nature of GATT callbacks, this is actually intentional to not build latency and simplify the thread model as everything is run on Bluetooth RX thread context, so entities handling the callbacks should be prepared to respond within the same context and not do things like fetch data from another entity, with this I hoped to encourage the approach of defining the service at the layer where values could be provided without blocking the RX thread.

[Thalley]

That is all fine if such layer are all instantiated by the application, so much like in GATT we don't need to reserve memory for services, the application would instantiate the Media Player which internally instantiate MCS instance

I think it will difficult to keep the memory at the application level, while ensuring that the application does not change values or states in a matter that is not allowed by the specifications.

That sort of API is indeed what it basically used in gatt.h and to some extend even iso.h, but it different from most of the other Bluetooth stack (e.g. bluetooth.h and conn.h), where we only expose opaque types as pointers to the application, and the application then requests "allocation" via functions.

This is also a good point to make in terms of how the API should be defined, and how much we want to optimize for memory and how much functionality/fields we want to directly expose to the application.

[asbjornsabo]

@Thalley

I would expect the layers to be something like
GATT <-> MCS <-> Bluetooth Media Player <-> Media Player <-> Application

I feel we may be talking past each other. Or maybe I just do no understand you correctly. Did you have a look at the figure?

I am thinking more like

GATT <-> MCS <-> Media player control <-> (Adaption layer, if required) <-> Media player
and
Application <-> Media player control <-> (Adaption layer, if required) <-> Media player

[Thalley]

Closing as the API has been settled on.