nimble/services: add PACS #1675
Conversation
KKopyscinski
force-pushed
the
pacs
branch
3 times, most recently
from
524ff53 to
134b6cc
Compare
| int ble_svc_audio_pacs_set_sink_audio_loc(uint32_t audio_locations); | ||
| int ble_svc_audio_pacs_set_source_audio_loc(uint32_t audio_locations); | ||
| int ble_svc_audio_pacs_set_sup_contexts(uint16_t sink_contexts, | ||
| uint16_t source_contexts); | ||
| int ble_svc_audio_pacs_set_avail_contexts(uint16_t conn_handle, | ||
| uint16_t sink_contexts, | ||
| uint16_t source_contexts); |
There was a problem hiding this comment.
I wonder whether is there a need to have those 4 functions instead of 1 e.g. ble_svc_audio_pacs_init. As those are basically used to initialize the characteristic values. What's your opinion on that?
struct ble_svc_audio_pacs_init_param {
uint32_t audio_locations;
uint16_t available_contexts;
uint16_t supported_contexts;
};
struct ble_svc_audio_pacs_init(uint8_t flags, struct ble_svc_audio_pacs_init_param *param);
where flags are the ones from Codec API. Hmm, this makes me feel that those should not be flags, but rather type` with 2 possible values Sink and Source defined as enum probably so that we could reuse it in different services.
There was a problem hiding this comment.
One problem I see is that available_contexts are per connection - so they cannot be set on service init, but after connection is established. So we have one exception. And because in previous comment you asked to disallow re-setting these values, because there is no notifications support for them (which I 100% agree with), we would have to split these into these that init on startup and available_contexts. On the other hand we here, as you notice, SRC/SNK pairs, and there will be more in other services. So I'm not sure if we can solve this cleanly with flags(types?) as we do in codecs.
There was a problem hiding this comment.
We discussed this offline and I made it into single functions, as you suggested. There is also second function that sets available_contexts - this must be done after connection exists and we know conn_handle.
KKopyscinski
force-pushed
the
pacs
branch
2 times, most recently
from
13658bc to
0de3fc0
Compare
|
Fixed |
|
TODO: add LC3 PACS package edit: done |
nimble/host/services/audio/pacs/include/services/pacs/ble_audio_svc_pacs.h
Outdated
Show resolved
Hide resolved
KKopyscinski
requested review from
rymanluk
and removed request for
MariuszSkamra
|
@rymanluk Added LC3 PACS package, resolved some bugs and tested code. I'll add doxygen soon |
KKopyscinski
force-pushed
the
pacs
branch
5 times, most recently
from
9b269eb to
466134a
Compare
nimble/host/services/audio/pacs/include/services/pacs/ble_audio_svc_pacs.h
Outdated
Show resolved
Hide resolved
KKopyscinski
force-pushed
the
pacs
branch
3 times, most recently
from
e9cf703 to
3ea7781
Compare
nimble/host/services/audio/pacs/lc3/include/services/pacs/ble_audio_svc_pacs_lc3.h
Outdated
Show resolved
Hide resolved
KKopyscinski
force-pushed
the
pacs
branch
4 times, most recently
from
383de73 to
1e13bce
Compare
|
This patch is blocked by #1714. I'll update this with working one once the other one is merged (There are issues with includes in current tree structure. They need to be fixed first, and this PR will use new directories) Edit: rebased after refactor was merged |
|
@MariuszSkamra @rymanluk rebased onto master |
| /** @brief Codec Registered */ | ||
| struct ble_audio_event_codec_changed { | ||
| /** Change type */ | ||
| enum { | ||
| CODEC_REGISTERED, | ||
| CODEC_UNREGISTERED, | ||
| } change; | ||
|
|
||
| /** Codec Record */ | ||
| const struct ble_audio_codec_record *record; | ||
| }; |
There was a problem hiding this comment.
I'm not sure about this one. IMO it would be cleaner to split this into 2 events:
BLE_AUDIO_EVENT_CODEC_REGISTERED
BLE_AUDIO_EVENT_CODEC_UNREGISTERED
There was a problem hiding this comment.
Ok, I'll also give them different structs (ble_audio_event_codec_registered and ble_audio_event_codec_unregistered), maybe in the future it'll have something more than ble_audio_codec_record
| - nimble/host | ||
|
|
||
| pkg.init: | ||
| # ble_svc_audio_pacs_lc3_init: 'MYNEWT_VAL(BLE_SVC_AUDIO_PACS_LC3_SYSINIT_STAGE)' |
| BLE_AUDIO_MAX_CODEC_RECORDS: | ||
| description: > | ||
| Maximum number of registered audio codecs. | ||
| value: 0 |
There was a problem hiding this comment.
does it make sense to include audio package and have no audio codecs registered?
There was a problem hiding this comment.
I think you can have app withouth codecs registered - it's not mandatory to register codecs you will use. So you can for example include this package, create broadcast application (bare bones, just sending advertising chain and BISes), use some codec to encode audio and not register it. But you need to register so it'll be visible in PACS (and also include PACS itself).
I think I should add compiler guards so ble_audio_codec.c is compiled only when this is >0
This commit adds Published Audio Capabilities Service/Prifile. In pair ble_audio_codec module is added, that supports registering supported codecs with their corresponding configurations.
KKopyscinski
requested review from
andrzej-kaczmarek,
MariuszSkamra and
rymanluk
This commit adds Published Audio Capabilities Service/Prifile. In pair ble_audio_codec module is added, that supports registering supported codecs with their corresponding configurations.