diff mbox series

[BlueZ,1/4] doc: extend MediaEndpoint1 API with SelectQoS

Message ID 6f03ad1eaaa00f84db8cd6a4a4b88ee83078951d.1698503903.git.pav@iki.fi
State New
Headers show
Series [BlueZ,1/4] doc: extend MediaEndpoint1 API with SelectQoS | expand

Commit Message

Pauli Virtanen Oct. 28, 2023, 2:39 p.m. UTC
Change unicast BAP configuration to proceed as:

0. SelectProperties(endpoint)
1. ASCS Config Codec
2. ASCS Server notifies ASE
3. SelectQoS(configuration)  [optional]
4. ASCS Config QoS
5. SetConfiguration(transport)

Previously, SelectProperties had to return also the QoS
configuration. However, it is impossible for it to provide it properly,
because the values supported by the server are known only after ASCS
Config Codec.

This resolves the issue by adding a new method call, which is supposed
to return suitable QoS values.

Remove the QoS input parameter from the SelectProperties() call, as the
server supported QoS settings may depend on the codec configuration, and
are not known yet at that point.

For convenience, e.g. when mandatory QoS presets are used, the endpoint
does not need to implement SelectQoS(). In this case the QoS values
returned by SelectProperties are used.
---

Notes:
    Alternative to this is calling SelectProperties() twice at the
    two different stages of the ASE setup steps.
    
    However, if the second SelectProperties() call returns a different
    Capabilities configuration, we'd need to either (i) do Config Codec again
    or (ii) fail the configuration.
    
    Doing Config Codec again introduces a chance of getting stuck looping,
    if client is not behaving correctly, which doesn't sound like good
    design. Failing the configuration raises question why have the
    Capabilities as return parameters at all. So instead, make it a separate
    method.
    
    ***
    
    If two methods is too much, we could in principle get rid of the
    SelectProperties() call and leave only SelectQoS.
    
    Instead, the sound server would call SetConfiguration() on a remote
    endpoint it chooses, and provide the configuration parameters there.
    IIUC, this is how it is supposed to work for BAP Broadcast currently.
    This might need some sort of "Ready" property on the Device1 DBus object
    or elsewhere (e.g. the endpoints), so that it's simple for the sound
    server to wait until all endpoints have been exposed in DBus.
    
    This might also be preferable way to do it, since only the component
    closer to the user i.e. the sound server knows which endpoint the user
    wanted to use, and when BlueZ guesses wrong it avoids needing to tear
    down the old configuration and reconfigure (which we have to do for
    A2DP).
    
    ***
    
    This series was tested also vs. this
    https://gitlab.freedesktop.org/pvir/pipewire/-/commits/bap-selectqos

 doc/org.bluez.MediaEndpoint.rst | 66 +++++++++++++++++++++++++++++----
 1 file changed, 58 insertions(+), 8 deletions(-)

Comments

bluez.test.bot@gmail.com Oct. 28, 2023, 4:41 p.m. UTC | #1
This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=797322

---Test result---

Test Summary:
CheckPatch                    FAIL      2.16 seconds
GitLint                       FAIL      1.20 seconds
BuildEll                      PASS      32.88 seconds
BluezMake                     PASS      1121.38 seconds
MakeCheck                     PASS      12.66 seconds
MakeDistcheck                 PASS      208.91 seconds
CheckValgrind                 PASS      354.45 seconds
CheckSmatch                   PASS      467.29 seconds
bluezmakeextell               PASS      146.12 seconds
IncrementalBuild              FAIL      2009.02 seconds
ScanBuild                     FAIL      904.53 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,4/4] client: implement SelectQoS
ERROR:SPACING: space prohibited before that close parenthesis ')'
#168: FILE: client/player.c:2298:
+					GDBUS_ARGS({ "properties", "a{sv}" } ),

ERROR:SPACING: space prohibited before that close parenthesis ')'
#169: FILE: client/player.c:2299:
+					GDBUS_ARGS({ "properties", "a{sv}" } ),

/github/workspace/src/src/13439494.patch total: 2 errors, 0 warnings, 74 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13439494.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[BlueZ,1/4] doc: extend MediaEndpoint1 API with SelectQoS

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
32: B2 Line has trailing whitespace: "    "
36: B2 Line has trailing whitespace: "    "
42: B2 Line has trailing whitespace: "    "
44: B2 Line has trailing whitespace: "    "
47: B2 Line has trailing whitespace: "    "
54: B2 Line has trailing whitespace: "    "
60: B2 Line has trailing whitespace: "    "
62: B2 Line has trailing whitespace: "    "
##############################
Test: IncrementalBuild - FAIL
Desc: Incremental build with the patches in the series
Output:
[BlueZ,2/4] shared/bap: rename PAC op select -> select_codec, add select_qos

tools/mgmt-tester.c: In function ‘main’:
tools/mgmt-tester.c:12763:5: note: variable tracking size limit exceeded with ‘-fvar-tracking-assignments’, retrying without
12763 | int main(int argc, char *argv[])
      |     ^~~~
unit/test-avdtp.c: In function ‘main’:
unit/test-avdtp.c:766:5: note: variable tracking size limit exceeded with ‘-fvar-tracking-assignments’, retrying without
  766 | int main(int argc, char *argv[])
      |     ^~~~
unit/test-avrcp.c: In function ‘main’:
unit/test-avrcp.c:989:5: note: variable tracking size limit exceeded with ‘-fvar-tracking-assignments’, retrying without
  989 | int main(int argc, char *argv[])
      |     ^~~~
profiles/audio/bap.c: In function ‘pac_found’:
profiles/audio/bap.c:1255:8: error: implicit declaration of function ‘bt_bap_select’; did you mean ‘bt_bap_detach’? [-Werror=implicit-function-declaration]
 1255 |   if (!bt_bap_select(lpac, rpac, select_cb, ep))
      |        ^~~~~~~~~~~~~
      |        bt_bap_detach
cc1: all warnings being treated as errors
make[1]: *** [Makefile:10576: profiles/audio/bluetoothd-bap.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:4677: all] Error 2
##############################
Test: ScanBuild - FAIL
Desc: Run Scan Build
Output:

src/shared/gatt-client.c:451:21: warning: Use of memory after it is freed
        gatt_db_unregister(op->client->db, op->db_id);
                           ^~~~~~~~~~
src/shared/gatt-client.c:696:2: warning: Use of memory after it is freed
        discovery_op_complete(op, false, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:993:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1099:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1291:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1356:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1631:6: warning: Use of memory after it is freed
        if (read_db_hash(op)) {
            ^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1636:2: warning: Use of memory after it is freed
        discover_all(op);
        ^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:2140:6: warning: Use of memory after it is freed
        if (read_db_hash(op)) {
            ^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:2148:8: warning: Use of memory after it is freed
                                                        discovery_op_ref(op),
                                                        ^~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:3237:2: warning: Use of memory after it is freed
        complete_write_long_op(req, success, 0, false);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:3259:2: warning: Use of memory after it is freed
        request_unref(req);
        ^~~~~~~~~~~~~~~~~~
12 warnings generated.
src/shared/shell.c:1228:13: warning: Access to field 'options' results in a dereference of a null pointer (loaded from variable 'opt')
                        if (c != opt->options[index - offset].val) {
                                 ^~~~~~~~~~~~
1 warning generated.
src/shared/gatt-client.c:451:21: warning: Use of memory after it is freed
        gatt_db_unregister(op->client->db, op->db_id);
                           ^~~~~~~~~~
src/shared/gatt-client.c:696:2: warning: Use of memory after it is freed
        discovery_op_complete(op, false, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:993:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1099:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1291:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1356:2: warning: Use of memory after it is freed
        discovery_op_complete(op, success, att_ecode);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1631:6: warning: Use of memory after it is freed
        if (read_db_hash(op)) {
            ^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:1636:2: warning: Use of memory after it is freed
        discover_all(op);
        ^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:2140:6: warning: Use of memory after it is freed
        if (read_db_hash(op)) {
            ^~~~~~~~~~~~~~~~
src/shared/gatt-client.c:2148:8: warning: Use of memory after it is freed
                                                        discovery_op_ref(op),
                                                        ^~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:3237:2: warning: Use of memory after it is freed
        complete_write_long_op(req, success, 0, false);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:3259:2: warning: Use of memory after it is freed
        request_unref(req);
        ^~~~~~~~~~~~~~~~~~
12 warnings generated.
src/shared/shell.c:1228:13: warning: Access to field 'options' results in a dereference of a null pointer (loaded from variable 'opt')
                        if (c != opt->options[index - offset].val) {
                                 ^~~~~~~~~~~~
1 warning generated.
tools/hciattach.c:816:7: warning: Although the value stored to 'n' is used in the enclosing expression, the value is never actually read from 'n'
        if ((n = read_hci_event(fd, resp, 10)) < 0) {
             ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/hciattach.c:864:7: warning: Although the value stored to 'n' is used in the enclosing expression, the value is never actually read from 'n'
        if ((n = read_hci_event(fd, resp, 4)) < 0) {
             ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/hciattach.c:886:8: warning: Although the value stored to 'n' is used in the enclosing expression, the value is never actually read from 'n'
                if ((n = read_hci_event(fd, resp, 10)) < 0) {
                     ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/hciattach.c:908:7: warning: Although the value stored to 'n' is used in the enclosing expression, the value is never actually read from 'n'
        if ((n = read_hci_event(fd, resp, 4)) < 0) {
             ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/hciattach.c:929:7: warning: Although the value stored to 'n' is used in the enclosing expression, the value is never actually read from 'n'
        if ((n = read_hci_event(fd, resp, 4)) < 0) {
             ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/hciattach.c:973:7: warning: Although the value stored to 'n' is used in the enclosing expression, the value is never actually read from 'n'
        if ((n = read_hci_event(fd, resp, 6)) < 0) {
             ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~
6 warnings generated.
src/oui.c:50:2: warning: Value stored to 'hwdb' is never read
        hwdb = udev_hwdb_unref(hwdb);
        ^      ~~~~~~~~~~~~~~~~~~~~~
src/oui.c:53:2: warning: Value stored to 'udev' is never read
        udev = udev_unref(udev);
        ^      ~~~~~~~~~~~~~~~~
2 warnings generated.
tools/hcidump.c:180:9: warning: Potential leak of memory pointed to by 'dp'
                                if (fds[i].fd == sock)
                                    ^~~
tools/hcidump.c:248:17: warning: Assigned value is garbage or undefined
                                dh->ts_sec  = htobl(frm.ts.tv_sec);
                                            ^ ~~~~~~~~~~~~~~~~~~~~
tools/hcidump.c:326:9: warning: 1st function call argument is an uninitialized value
                                if (be32toh(dp.flags) & 0x02) {
                                    ^~~~~~~~~~~~~~~~~
/usr/include/endian.h:46:22: note: expanded from macro 'be32toh'
#  define be32toh(x) __bswap_32 (x)
                     ^~~~~~~~~~~~~~
tools/hcidump.c:341:20: warning: 1st function call argument is an uninitialized value
                                frm.data_len = be32toh(dp.len);
                                               ^~~~~~~~~~~~~~~
/usr/include/endian.h:46:22: note: expanded from macro 'be32toh'
#  define be32toh(x) __bswap_32 (x)
                     ^~~~~~~~~~~~~~
tools/hcidump.c:346:14: warning: 1st function call argument is an uninitialized value
                                opcode = be32toh(dp.flags) & 0xffff;
                                         ^~~~~~~~~~~~~~~~~
/usr/include/endian.h:46:22: note: expanded from macro 'be32toh'
#  define be32toh(x) __bswap_32 (x)
                     ^~~~~~~~~~~~~~
tools/hcidump.c:384:17: warning: Assigned value is garbage or undefined
                        frm.data_len = btohs(dh.len);
                                     ^ ~~~~~~~~~~~~~
tools/hcidump.c:394:11: warning: Assigned value is garbage or undefined
                frm.len = frm.data_len;
                        ^ ~~~~~~~~~~~~
tools/hcidump.c:398:9: warning: 1st function call argument is an uninitialized value
                        ts = be64toh(ph.ts);
                             ^~~~~~~~~~~~~~
/usr/include/endian.h:51:22: note: expanded from macro 'be64toh'
#  define be64toh(x) __bswap_64 (x)
                     ^~~~~~~~~~~~~~
tools/hcidump.c:403:13: warning: 1st function call argument is an uninitialized value
                        frm.in = be32toh(dp.flags) & 0x01;
                                 ^~~~~~~~~~~~~~~~~
/usr/include/endian.h:46:22: note: expanded from macro 'be32toh'
#  define be32toh(x) __bswap_32 (x)
                     ^~~~~~~~~~~~~~
tools/hcidump.c:408:11: warning: Assigned value is garbage or undefined
                        frm.in = dh.in;
                               ^ ~~~~~
tools/hcidump.c:437:7: warning: Null pointer passed to 1st parameter expecting 'nonnull'
        fd = open(file, open_flags, 0644);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
11 warnings generated.
tools/rfcomm.c:228:3: warning: Value stored to 'i' is never read
                i = execvp(cmdargv[0], cmdargv);
                ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/rfcomm.c:228:7: warning: Null pointer passed to 1st parameter expecting 'nonnull'
                i = execvp(cmdargv[0], cmdargv);
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/rfcomm.c:348:8: warning: Although the value stored to 'fd' is used in the enclosing expression, the value is never actually read from 'fd'
                if ((fd = open(devname, O_RDONLY | O_NOCTTY)) < 0) {
                     ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/rfcomm.c:491:14: warning: Assigned value is garbage or undefined
        req.channel = raddr.rc_channel;
                    ^ ~~~~~~~~~~~~~~~~
tools/rfcomm.c:509:8: warning: Although the value stored to 'fd' is used in the enclosing expression, the value is never actually read from 'fd'
                if ((fd = open(devname, O_RDONLY | O_NOCTTY)) < 0) {
                     ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
5 warnings generated.
src/sdp-xml.c:126:10: warning: Assigned value is garbage or undefined
                buf[1] = data[i + 1];
                       ^ ~~~~~~~~~~~
src/sdp-xml.c:300:11: warning: Assigned value is garbage or undefined
                        buf[1] = data[i + 1];
                               ^ ~~~~~~~~~~~
src/sdp-xml.c:338:11: warning: Assigned value is garbage or undefined
                        buf[1] = data[i + 1];
                               ^ ~~~~~~~~~~~
3 warnings generated.
tools/ciptool.c:350:7: warning: 5th function call argument is an uninitialized value
        sk = do_connect(ctl, dev_id, &src, &dst, psm, (1 << CMTP_LOOPBACK));
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
tools/sdptool.c:941:26: warning: Result of 'malloc' is converted to a pointer of type 'uint32_t', which is incompatible with sizeof operand type 'int'
                        uint32_t *value_int = malloc(sizeof(int));
                        ~~~~~~~~~~            ^~~~~~ ~~~~~~~~~~~
tools/sdptool.c:980:4: warning: 1st function call argument is an uninitialized value
                        free(allocArray[i]);
                        ^~~~~~~~~~~~~~~~~~~
tools/sdptool.c:3777:2: warning: Potential leak of memory pointed to by 'si.name'
        return add_service(0, &si);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~
tools/sdptool.c:4112:4: warning: Potential leak of memory pointed to by 'context.svc'
                        return -1;
                        ^~~~~~~~~
4 warnings generated.
tools/avtest.c:224:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 3);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:234:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 4);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:243:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 3);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:257:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf,
                                ^     ~~~~~~~~~~~~~~
tools/avtest.c:264:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf,
                                ^     ~~~~~~~~~~~~~~
tools/avtest.c:271:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf,
                                ^     ~~~~~~~~~~~~~~
tools/avtest.c:278:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf,
                                ^     ~~~~~~~~~~~~~~
tools/avtest.c:289:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 4);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:293:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 2);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:302:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 3);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:306:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 2);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:315:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 3);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:322:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 2);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:344:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 4);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:348:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 2);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:357:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 3);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:361:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 2);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:374:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 4);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:378:5: warning: Value stored to 'len' is never read
                                len = write(sk, buf, 2);
                                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:385:4: warning: Value stored to 'len' is never read
                        len = write(sk, buf, 2);
                        ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:395:4: warning: Value stored to 'len' is never read
                        len = write(sk, buf, 2);
                        ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:559:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, 2);
                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:567:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, invalid ? 2 : 3);
                ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/avtest.c:581:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, 4 + sizeof(media_transport));
                ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/avtest.c:594:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, 3);
                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:604:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, 3);
                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:616:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, 3);
                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:631:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, 3);
                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:643:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, 3);
                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:652:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, 3);
                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:659:3: warning: Value stored to 'len' is never read
                len = write(sk, buf, 2);
                ^     ~~~~~~~~~~~~~~~~~
tools/avtest.c:695:2: warning: Value stored to 'len' is never read
        len = write(sk, buf, AVCTP_HEADER_LENGTH + sizeof(play_pressed));
        ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
32 warnings generated.
tools/btproxy.c:836:15: warning: Null pointer passed to 1st parameter expecting 'nonnull'
                        tcp_port = atoi(optarg);
                                   ^~~~~~~~~~~~
tools/btproxy.c:839:8: warning: Null pointer passed to 1st parameter expecting 'nonnull'
                        if (strlen(optarg) > 3 && !strncmp(optarg, "hci", 3))
                            ^~~~~~~~~~~~~~
2 warnings generated.
tools/create-image.c:76:3: warning: Value stored to 'fd' is never read
                fd = -1;
                ^    ~~
tools/create-image.c:84:3: warning: Value stored to 'fd' is never read
                fd = -1;
                ^    ~~
tools/create-image.c:92:3: warning: Value stored to 'fd' is never read
                fd = -1;
                ^    ~~
tools/create-image.c:105:2: warning: Value stored to 'fd' is never read
        fd = -1;
        ^    ~~
4 warnings generated.
tools/btgatt-client.c:1597:2: warning: Value stored to 'argv' is never read
        argv += optind;
        ^       ~~~~~~
1 warning generated.
tools/btgatt-server.c:1212:2: warning: Value stored to 'argv' is never read
        argv -= optind;
        ^       ~~~~~~
1 warning generated.
tools/check-selftest.c:42:3: warning: Value stored to 'ptr' is never read
                ptr = fgets(result, sizeof(result), fp);
                ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
tools/gatt-service.c:294:2: warning: 2nd function call argument is an uninitialized value
        chr_write(chr, value, len);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
tools/obex-server-tool.c:133:13: warning: Null pointer passed to 1st parameter expecting 'nonnull'
        data->fd = open(name, O_WRONLY | O_CREAT | O_NOCTTY, 0600);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/obex-server-tool.c:192:13: warning: Null pointer passed to 1st parameter expecting 'nonnull'
        data->fd = open(name, O_RDONLY | O_NOCTTY, 0);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
tools/test-runner.c:945:2: warning: 2nd function call argument is an uninitialized value
        printf("Running command %s\n", cmdname ? cmdname : argv[0]);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
tools/btpclientctl.c:402:3: warning: Value stored to 'bit' is never read
                bit = 0;
                ^     ~
tools/btpclientctl.c:1655:2: warning: Null pointer passed to 2nd parameter expecting 'nonnull'
        memcpy(cp->data, ad_data, ad_len);
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
src/sdpd-request.c:211:13: warning: Result of 'malloc' is converted to a pointer of type 'char', which is incompatible with sizeof operand type 'uint16_t'
                                pElem = malloc(sizeof(uint16_t));
                                        ^~~~~~ ~~~~~~~~~~~~~~~~
src/sdpd-request.c:239:13: warning: Result of 'malloc' is converted to a pointer of type 'char', which is incompatible with sizeof operand type 'uint32_t'
                                pElem = malloc(sizeof(uint32_t));
                                        ^~~~~~ ~~~~~~~~~~~~~~~~
2 warnings generated.
android/avrcp-lib.c:1968:3: warning: 1st function call argument is an uninitialized value
                g_free(text[i]);
                ^~~~~~~~~~~~~~~
1 warning generated.
profiles/health/hdp.c:644:3: warning: Use of memory after it is freed
                hdp_tmp_dc_data_unref(dc_data);
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
profiles/health/hdp.c:800:19: warning: Use of memory after it is freed
                path = g_strdup(chan->path);
                                ^~~~~~~~~~
profiles/health/hdp.c:1779:6: warning: Use of memory after it is freed
                                        hdp_tmp_dc_data_ref(hdp_conn),
                                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
profiles/health/hdp.c:1836:30: warning: Use of memory after it is freed
        reply = g_dbus_create_error(data->msg, ERROR_INTERFACE ".HealthError",
                                    ^~~~~~~~~
4 warnings generated.
profiles/health/hdp_util.c:1053:2: warning: Use of memory after it is freed
        conn_data->func(conn_data->data, gerr);
        ^~~~~~~~~~~~~~~
1 warning generated.
profiles/audio/bap.c: In function ‘pac_found’:
profiles/audio/bap.c:1255:8: error: implicit declaration of function ‘bt_bap_select’; did you mean ‘bt_bap_detach’? [-Werror=implicit-function-declaration]
 1255 |   if (!bt_bap_select(lpac, rpac, select_cb, ep))
      |        ^~~~~~~~~~~~~
      |        bt_bap_detach
cc1: all warnings being treated as errors
make[1]: *** [Makefile:10576: profiles/audio/bluetoothd-bap.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:4677: all] Error 2


---
Regards,
Linux Bluetooth
Luiz Augusto von Dentz Nov. 15, 2023, 12:22 a.m. UTC | #2
Hi Pauli,

On Sat, Oct 28, 2023 at 10:42 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Change unicast BAP configuration to proceed as:
>
> 0. SelectProperties(endpoint)
> 1. ASCS Config Codec
> 2. ASCS Server notifies ASE
> 3. SelectQoS(configuration)  [optional]
> 4. ASCS Config QoS
> 5. SetConfiguration(transport)
>
> Previously, SelectProperties had to return also the QoS
> configuration. However, it is impossible for it to provide it properly,
> because the values supported by the server are known only after ASCS
> Config Codec.
>
> This resolves the issue by adding a new method call, which is supposed
> to return suitable QoS values.
>
> Remove the QoS input parameter from the SelectProperties() call, as the
> server supported QoS settings may depend on the codec configuration, and
> are not known yet at that point.
>
> For convenience, e.g. when mandatory QoS presets are used, the endpoint
> does not need to implement SelectQoS(). In this case the QoS values
> returned by SelectProperties are used.

Id leave the endpoint the option to return the QoS on
SelectProperties, if it doesn't then we call SelectQoS, that said this
also means we need to wait until the endpoint respond with the QoS in
order to create the socket or we have to create the socket without any
QoS which afaik is required to bind.

> ---
>
> Notes:
>     Alternative to this is calling SelectProperties() twice at the
>     two different stages of the ASE setup steps.
>
>     However, if the second SelectProperties() call returns a different
>     Capabilities configuration, we'd need to either (i) do Config Codec again
>     or (ii) fail the configuration.
>
>     Doing Config Codec again introduces a chance of getting stuck looping,
>     if client is not behaving correctly, which doesn't sound like good
>     design. Failing the configuration raises question why have the
>     Capabilities as return parameters at all. So instead, make it a separate
>     method.

Well it is also possible that the server returns non-optiomal
preferences compared to a different codec configuration but we can
only know that once the codec is configured, so we need to be prepared
for the headset to misbehave like many do in case of A2DP, but in case
of BAP they can messed up in both Codec configuration and QoS stages.

>
>     ***
>
>     If two methods is too much, we could in principle get rid of the
>     SelectProperties() call and leave only SelectQoS.
>
>     Instead, the sound server would call SetConfiguration() on a remote
>     endpoint it chooses, and provide the configuration parameters there.
>     IIUC, this is how it is supposed to work for BAP Broadcast currently.
>     This might need some sort of "Ready" property on the Device1 DBus object
>     or elsewhere (e.g. the endpoints), so that it's simple for the sound
>     server to wait until all endpoints have been exposed in DBus.
>
>     This might also be preferable way to do it, since only the component
>     closer to the user i.e. the sound server knows which endpoint the user
>     wanted to use, and when BlueZ guesses wrong it avoids needing to tear
>     down the old configuration and reconfigure (which we have to do for
>     A2DP).

In principle Im nore in favor of keeping the logic of bluetoothd
calling into the endpoint object when it is necessary, we also may
apply the same logic as for A2DP that use the last configuration when
reconnecting, so there is no guessing but more of a policy of using
the last configuration to make the reconnections as quick as possible.
If the user wants to change the configuration he can do via
SetConfiguration, but this normally requires a user interaction, also
if the idea would be to have the reconnection policy on top of
bluetoothd Id say it is probably not recommended as that would
probably be slower and it may not know all the endpoints registered in
order to properly infer what is the best configuration to use.

>     ***
>
>     This series was tested also vs. this
>     https://gitlab.freedesktop.org/pvir/pipewire/-/commits/bap-selectqos
>
>  doc/org.bluez.MediaEndpoint.rst | 66 +++++++++++++++++++++++++++++----
>  1 file changed, 58 insertions(+), 8 deletions(-)
>
> diff --git a/doc/org.bluez.MediaEndpoint.rst b/doc/org.bluez.MediaEndpoint.rst
> index 6754d6e3b..4ffe6951c 100644
> --- a/doc/org.bluez.MediaEndpoint.rst
> +++ b/doc/org.bluez.MediaEndpoint.rst
> @@ -66,6 +66,8 @@ array{byte} SelectConfiguration(array{byte} capabilities)
>         Note: There is no need to cache the selected configuration since on
>         success the configuration is send back as parameter of SetConfiguration.
>
> +.. _SelectProperties:
> +
>  dict SelectProperties(dict capabilities)
>  ````````````````````````````````````````
>
> @@ -79,8 +81,58 @@ dict SelectProperties(dict capabilities)
>
>         :uint32 Locations:
>
> +       See `MediaEndpoint Properties`_ for their possible values.
> +
> +       Returns a configuration which can be used to setup a transport:
> +
> +       :array{byte} Capabilities:
> +
> +               See **org.bluez.MediaTransport(5)**.
> +
> +       :array{byte} Metadata [optional]:
> +
> +               See **org.bluez.MediaTransport(5)**.
> +
> +       :dict QoS:
> +
> +               See **org.bluez.MediaTransport(5)**.
> +
> +               The following fields shall be provided:
> +
> +               :byte TargetLatency:
> +               :byte PHY:
> +
> +               If `SelectQoS`_ is not implemented, then values for
> +               all other ``QoS`` fields are also determined by the
> +               value returned here.
> +
> +       Note: There is no need to cache the selected properties since
> +       on success the configuration is sent back as parameter of
> +       `SetConfiguration`_ and `SelectQoS`_.
> +
> +.. _SelectQoS:
> +
> +dict SelectQoS(dict configuration)
> +``````````````````````````````````
> +
> +       Select BAP unicast QoS to be used for a transport, based on
> +       server capabilities and selected configuration.
> +
> +       :object Endpoint:
> +
> +       :array{byte} Capabilities:
> +
> +               The configuration, as returned by `SelectProperties`_.
> +
> +       :array{byte} Metadata [optional]:
> +
> +               The metadata, as returned by `SelectProperties`_.
> +
>         :dict QoS:
>
> +               Server endpoint supported and preferred values.  See
> +               `MediaEndpoint Properties`_ for their possible values.
> +
>                 :byte Framing:
>                 :byte PHY:
>                 :uint16 MaximumLatency:
> @@ -89,18 +141,16 @@ dict SelectProperties(dict capabilities)
>                 :uint32 PreferredMinimumDelay:
>                 :uint32 PreferredMaximumDelay:
>
> -       See `MediaEndpoint Properties`_ for their possible values.
> +       Returns a QoS configuration which can be used to setup a transport:
>
> -       Returns a configuration which can be used to setup a transport:
> -
> -       :array{byte} Capabilities:
> -       :array{byte} Metadata [optional]:
>         :dict QoS:
>
> -       See `SetConfiguration`_ for their possible values.
> +               See **org.bluez.MediaTransport(5)** QoS property for
> +               possible values.
>
> -       Note: There is no need to cache the selected properties since on
> -       success the configuration is send back as parameter of SetConfiguration.
> +       Note: There is no need to cache the selected properties since
> +       on success the configuration is sent back as parameter of
> +       `SetConfiguration`_.
>
>  void ClearConfiguration(object transport)
>  `````````````````````````````````````````
> --
> 2.41.0
>
diff mbox series

Patch

diff --git a/doc/org.bluez.MediaEndpoint.rst b/doc/org.bluez.MediaEndpoint.rst
index 6754d6e3b..4ffe6951c 100644
--- a/doc/org.bluez.MediaEndpoint.rst
+++ b/doc/org.bluez.MediaEndpoint.rst
@@ -66,6 +66,8 @@  array{byte} SelectConfiguration(array{byte} capabilities)
 	Note: There is no need to cache the selected configuration since on
 	success the configuration is send back as parameter of SetConfiguration.
 
+.. _SelectProperties:
+
 dict SelectProperties(dict capabilities)
 ````````````````````````````````````````
 
@@ -79,8 +81,58 @@  dict SelectProperties(dict capabilities)
 
 	:uint32 Locations:
 
+	See `MediaEndpoint Properties`_ for their possible values.
+
+	Returns a configuration which can be used to setup a transport:
+
+	:array{byte} Capabilities:
+
+		See **org.bluez.MediaTransport(5)**.
+
+	:array{byte} Metadata [optional]:
+
+		See **org.bluez.MediaTransport(5)**.
+
+	:dict QoS:
+
+		See **org.bluez.MediaTransport(5)**.
+
+		The following fields shall be provided:
+
+		:byte TargetLatency:
+		:byte PHY:
+
+		If `SelectQoS`_ is not implemented, then values for
+		all other ``QoS`` fields are also determined by the
+		value returned here.
+
+	Note: There is no need to cache the selected properties since
+	on success the configuration is sent back as parameter of
+	`SetConfiguration`_ and `SelectQoS`_.
+
+.. _SelectQoS:
+
+dict SelectQoS(dict configuration)
+``````````````````````````````````
+
+	Select BAP unicast QoS to be used for a transport, based on
+	server capabilities and selected configuration.
+
+	:object Endpoint:
+
+	:array{byte} Capabilities:
+
+		The configuration, as returned by `SelectProperties`_.
+
+	:array{byte} Metadata [optional]:
+
+		The metadata, as returned by `SelectProperties`_.
+
 	:dict QoS:
 
+		Server endpoint supported and preferred values.	 See
+		`MediaEndpoint Properties`_ for their possible values.
+
 		:byte Framing:
 		:byte PHY:
 		:uint16 MaximumLatency:
@@ -89,18 +141,16 @@  dict SelectProperties(dict capabilities)
 		:uint32 PreferredMinimumDelay:
 		:uint32 PreferredMaximumDelay:
 
-	See `MediaEndpoint Properties`_ for their possible values.
+	Returns a QoS configuration which can be used to setup a transport:
 
-	Returns a configuration which can be used to setup a transport:
-
-	:array{byte} Capabilities:
-	:array{byte} Metadata [optional]:
 	:dict QoS:
 
-	See `SetConfiguration`_ for their possible values.
+		See **org.bluez.MediaTransport(5)** QoS property for
+		possible values.
 
-	Note: There is no need to cache the selected properties since on
-	success the configuration is send back as parameter of SetConfiguration.
+	Note: There is no need to cache the selected properties since
+	on success the configuration is sent back as parameter of
+	`SetConfiguration`_.
 
 void ClearConfiguration(object transport)
 `````````````````````````````````````````