Bluetooth: Audio: Audio Input Control Service and client #32785
Conversation
github-actions
bot
added
area: API
Thalley
force-pushed
the
audio_input_control_service
branch
5 times, most recently
from
4be2f8b to
d14200d
Compare
Thalley
force-pushed
the
audio_input_control_service
branch
3 times, most recently
from
d4f1b25 to
1a7ab00
Compare
Thalley
added
DNM
Thalley
force-pushed
the
audio_input_control_service
branch
from
1a7ab00 to
c8c6447
Compare
Thalley
force-pushed
the
audio_input_control_service
branch
from
c8c6447 to
9446983
Compare
subsys/bluetooth/audio/aics.c
Outdated
There was a problem hiding this comment.
The license should probably be separated from the description.
Shouldn't Nordic be added? And 2021
There was a problem hiding this comment.
Do we have any guidelines on dealing with this?
There weren't any similar comments for my VOCS PR (#31893). Regarding the year; should it be the year that Nordic started working on it, a 2020-2021 or the year Nordic upstreamed this file? I've heard different expectations.
There was a problem hiding this comment.
But added Nordic to the files.
There was a problem hiding this comment.
I believe Nordic policy is the year the work was started
Thalley
force-pushed
the
audio_input_control_service
branch
from
9446983 to
43fd68b
Compare
Thalley
force-pushed
the
audio_input_control_service
branch
from
43fd68b to
ec02f42
Compare
subsys/bluetooth/audio/aics.c
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Set up a discussion for it.
This is a larger question than this PR.
There was a problem hiding this comment.
Moved to #33592 (comment)
subsys/bluetooth/audio/aics.c
Outdated
There was a problem hiding this comment.
It might be a good ideia to rename the struct bt_aics_init to be different than the function name, or perhaps rename the function name to something like bt_aics_register as it appears to be registering service instances at runtime.
Btw, we probably need to make clear that both forms of registration seems to be available.
There was a problem hiding this comment.
@Vudentz Thanks, that was actually something that was done to VOCS but not AICS for some reason.
Regarding the single register (i.e. just bt_aics_init/register instead of bt_aics_init and bt_aics_register) makes a lot of sense to me. There's no reason why these can't be done in the same call, unless we want to unregister the callbacks, but I doubt that will make sense to do.
I'll merge the two, and also add a complementary update to VOCS (and VCS/MICS)
subsys/bluetooth/audio/aics.c
Outdated
There was a problem hiding this comment.
Is this calling register the instances from bt_aics_free_instance_get? Im not sure how that would work because those instances are already registered in the static portion of the table so this would register it again.
There was a problem hiding this comment.
The instances are defined with BT_GATT_SERVICE_INSTANCE_DEFINE which doesn't statically register the GATT services, so this should work correctly.
Thalley
force-pushed
the
audio_input_control_service
branch
from
ec02f42 to
889a99d
Compare
subsys/bluetooth/audio/aics_client.c
Outdated
There was a problem hiding this comment.
"Handle not set for control point"?
There was a problem hiding this comment.
This is on purpose, since this is a common cp write function it's useful to log which opcode failed.
There was a problem hiding this comment.
Agree. But the handle is the same for all opcodes, right?
There was a problem hiding this comment.
(I am OK as is, I just thought it might confuse people to believe that different opcodes have different handles.)
Thalley
force-pushed
the
audio_input_control_service
branch
2 times, most recently
from
18dfe96 to
1a52003
Compare
This commit implements the secondary service Audio Input Control Service (AICS) server and client. Signed-off-by: Emil Gydesen <[email protected]>
Thalley
force-pushed
the
audio_input_control_service
branch
from
1a52003 to
d9b0a05
Compare
5 participants
This PR implements the Audio Input Control Service (AICS) server and client.
The shell has purposely not been modified to control this server, as the service is a secondary service and only has purpose in the context of a primary service.
This PR is the first in a series of 3 to add the Volume Control Service server and client:
The reason why VOCS and AICS should be implemented first, is that VCS (as a primary service) uses these two secondary services, and having them implemented first will make it easier to merge.