| Message ID | 20230922192335.1060601-1-luiz.dentz@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/6] media-api: Update to reflect the last code changes | expand |
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=786884 ---Test result--- Test Summary: CheckPatch PASS 4.24 seconds GitLint FAIL 2.39 seconds BuildEll PASS 28.66 seconds BluezMake PASS 1020.76 seconds MakeCheck PASS 12.07 seconds MakeDistcheck PASS 163.66 seconds CheckValgrind PASS 265.29 seconds CheckSmatch PASS 358.10 seconds bluezmakeextell PASS 109.72 seconds IncrementalBuild PASS 5193.51 seconds ScanBuild PASS 1080.16 seconds Details ############################## Test: GitLint - FAIL Desc: Run gitlint Output: [v2,3/6] client: Make transport.show to print QoS property 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 10: B3 Line contains hard tab characters (\t): " UUID: 00002bcb-0000-1000-8000-00805f9b34fb" 11: B3 Line contains hard tab characters (\t): " Codec: 0x06 (6)" 12: B3 Line contains hard tab characters (\t): " Configuration:" 14: B3 Line contains hard tab characters (\t): " Device: /org/bluez/hci0/dev_00_AA_01_01_00_03" 15: B3 Line contains hard tab characters (\t): " State: idle" 16: B3 Line contains hard tab characters (\t): " Endpoint: /org/bluez/hci0/dev_00_AA_01_01_00_03/pac_sink0" 17: B3 Line contains hard tab characters (\t): " QoS Key: CIG" 18: B3 Line contains hard tab characters (\t): " QoS Value: 0x00 (0)" 19: B3 Line contains hard tab characters (\t): " QoS Key: CIS" 20: B3 Line contains hard tab characters (\t): " QoS Value: 0x00 (0)" 21: B3 Line contains hard tab characters (\t): " QoS Key: Framing" 22: B3 Line contains hard tab characters (\t): " QoS Value: 0x00 (0)" 23: B3 Line contains hard tab characters (\t): " QoS Key: PresentationDelay" 24: B3 Line contains hard tab characters (\t): " QoS Value: 0x00009c40 (40000)" 25: B3 Line contains hard tab characters (\t): " QoS Key: Interval" 26: B3 Line contains hard tab characters (\t): " QoS Value: 0x00002710 (10000)" 27: B3 Line contains hard tab characters (\t): " QoS Key: Latency" 28: B3 Line contains hard tab characters (\t): " QoS Value: 0x000a (10)" 29: B3 Line contains hard tab characters (\t): " QoS Key: SDU" 30: B3 Line contains hard tab characters (\t): " QoS Value: 0x0028 (40)" 31: B3 Line contains hard tab characters (\t): " QoS Key: PHY" 32: B3 Line contains hard tab characters (\t): " QoS Value: 0x02 (2)" 33: B3 Line contains hard tab characters (\t): " QoS Key: Retransmissions" 34: B3 Line contains hard tab characters (\t): " QoS Value: 0x02 (2)" 35: B3 Line contains hard tab characters (\t): " Location: 0x00000003 (3)" 36: B3 Line contains hard tab characters (\t): " Links: /org/bluez/hci0/dev_00_AA_01_01_00_03/pac_source0/fd0" --- Regards, Linux Bluetooth
Hi Luiz, pe, 2023-09-22 kello 12:23 -0700, Luiz Augusto von Dentz kirjoitti: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > This splits media-api.rst into org.bluez.Media<interface>.rst and > generate manpages for them. I made a pass looking at the implementations of the transport/endpoint and SetConfiguration/SelectProperties/SelectConfiguration and comparing to docs. Note that the SelectProperties API has a problem with BAP endpoint configuration. We maybe should redesign it while we still can? I think generally at least from Pipewire side there's no need to keep backward compatibility while this is experimental. We can target BlueZ master branch. SelectProperties API problem: As BAP client one first does Config Codec (ASCS §5.1) which takes Target_Latency, Target_PHY and Codec. After this, the ASE properties (ACSS Table 4.3) in Codec Configured state contain the server supported values for Max_Transport_Latency, Presentation_Delay_Min/Max etc. SelectProperties is called before Config Codec, so it cannot necessarily know the server supported values. In this case the sound server cannot fill in QoS correctly. The client-invoked SetConfiguration API also seems to have similar issue. Probably: SelectProperties should be called twice, once to get parameters for Config Codec, and then again to get parameters for Config QoS. Or, there should be a separate "SelectCodec" and "SelectQoS" calls. Calling "SelectProperties" twice could be simpler for everyone. In client-invoked "SetConfiguration" API, one probably should only pass in the parameters needed for Config Codec, and BlueZ should then make a SelectProperties call to get the QoS ones once the server-side values are known. I can take a look at this... MediaTransport: "Delay" field only exists for A2DP in code. "Volume" field only exists for A2DP in code. "Links" is ucast only in code. "QoS.TargetLatency" does not exist in code. Maybe it should be exposed for consistency, since it's expected as return from SelectProperties. SetConfiguration: SetConfiguration properties dict when called by BlueZ contains exactly the properties of the transport. The documentation probably should say that this is so. When called by client, the contents of the properties dict are expected to be different. QoS parameters are not packed in a "QoS" dict. Maybe they should be. The "PHY" key is also still a string here. The documented input parameters expected from client are wrong, the documentation explains fields of struct bt_bap_pac_qos, but what the code parses are that of struct bt_bap_qos. "MaximumLatency": no such field in code, seems to be called "Latency" everywhere. SelectProperties: In code, the return parameter expects QoS parameters (struct bt_bap_qos) to be packed in "QoS" dict. They are not packed in "QoS" dict in the input parameters. These are not exactly QoS, but instead struct bt_bap_pac_qos, so not clear if they should be packed. The input and return parameters are not documented. The input QoS contains fields of struct bt_bap_pac_qos. The return QoS has fields of struct bt_bap_qos. MediaEndpoint: The endpoints published by BlueZ only have the properties "UUID", "Codec", "Capabilities", "Device". The documentation lists various other things, but they are not implemented as server endpoint properties. As client endpoint properties: "RTN" is expected to be present in code, but is not documented. "MaximumLatency" "Location" "SupportedContext" "Context" are documented but not present in code.
Hi, la, 2023-09-23 kello 12:14 +0300, Pauli Virtanen kirjoitti: > Hi Luiz, > > pe, 2023-09-22 kello 12:23 -0700, Luiz Augusto von Dentz kirjoitti: > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > > > This splits media-api.rst into org.bluez.Media<interface>.rst and > > generate manpages for them. > > I made a pass looking at the implementations of the transport/endpoint > and SetConfiguration/SelectProperties/SelectConfiguration and comparing > to docs. > > Note that the SelectProperties API has a problem with BAP endpoint > configuration. We maybe should redesign it while we still can? > > I think generally at least from Pipewire side there's no need to keep > backward compatibility while this is experimental. We can target BlueZ > master branch. > > > SelectProperties API problem: > > As BAP client one first does Config Codec (ASCS §5.1) which takes > Target_Latency, Target_PHY and Codec. > > After this, the ASE properties (ACSS Table 4.3) in Codec Configured > state contain the server supported values for Max_Transport_Latency, > Presentation_Delay_Min/Max etc. > > SelectProperties is called before Config Codec, so it cannot > necessarily know the server supported values. In this case the sound > server cannot fill in QoS correctly. > > The client-invoked SetConfiguration API also seems to have similar > issue. > > Probably: SelectProperties should be called twice, once to get > parameters for Config Codec, and then again to get parameters for > Config QoS. Or, there should be a separate "SelectCodec" and > "SelectQoS" calls. Calling "SelectProperties" twice could be simpler > for everyone. > > In client-invoked "SetConfiguration" API, one probably should only pass > in the parameters needed for Config Codec, and BlueZ should then make a > SelectProperties call to get the QoS ones once the server-side values > are known. > > I can take a look at this... > > > MediaTransport: > > "Delay" field only exists for A2DP in code. > > "Volume" field only exists for A2DP in code. > > "Links" is ucast only in code. > > "QoS.TargetLatency" does not exist in code. Maybe it should be exposed > for consistency, since it's expected as return from SelectProperties. > > > SetConfiguration: > > SetConfiguration properties dict when called by BlueZ contains exactly > the properties of the transport. The documentation probably should say > that this is so. > > When called by client, the contents of the properties dict are expected > to be different. QoS parameters are not packed in a "QoS" dict. Maybe > they should be. The "PHY" key is also still a string here. Also, "Framing" is boolean here (elsewhere it's byte). > > The documented input parameters expected from client are wrong, the > documentation explains fields of struct bt_bap_pac_qos, but what the > code parses are that of struct bt_bap_qos. > > "MaximumLatency": no such field in code, seems to be called "Latency" > everywhere. > > > SelectProperties: > > In code, the return parameter expects QoS parameters (struct > bt_bap_qos) to be packed in "QoS" dict. > > They are not packed in "QoS" dict in the input parameters. These are > not exactly QoS, but instead struct bt_bap_pac_qos, so not clear if > they should be packed. > > The input and return parameters are not documented. The input QoS > contains fields of struct bt_bap_pac_qos. The return QoS has fields of > struct bt_bap_qos. > > > MediaEndpoint: > > The endpoints published by BlueZ only have the properties > "UUID", "Codec", "Capabilities", "Device". > > The documentation lists various other things, but they are not > implemented as server endpoint properties. > > As client endpoint properties: > > "RTN" is expected to be present in code, but is not documented. "RTN" is called "Retransmissions" elsewhere. > > "MaximumLatency" "Location" "SupportedContext" "Context" are documented > but not present in code. > >
Hi Pauli, On Sat, Sep 23, 2023 at 2:14 AM Pauli Virtanen <pav@iki.fi> wrote: > > Hi Luiz, > > pe, 2023-09-22 kello 12:23 -0700, Luiz Augusto von Dentz kirjoitti: > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > > > This splits media-api.rst into org.bluez.Media<interface>.rst and > > generate manpages for them. > > I made a pass looking at the implementations of the transport/endpoint > and SetConfiguration/SelectProperties/SelectConfiguration and comparing > to docs. > > Note that the SelectProperties API has a problem with BAP endpoint > configuration. We maybe should redesign it while we still can? > > I think generally at least from Pipewire side there's no need to keep > backward compatibility while this is experimental. We can target BlueZ > master branch. > > > SelectProperties API problem: > > As BAP client one first does Config Codec (ASCS §5.1) which takes > Target_Latency, Target_PHY and Codec. > > After this, the ASE properties (ACSS Table 4.3) in Codec Configured > state contain the server supported values for Max_Transport_Latency, > Presentation_Delay_Min/Max etc. > > SelectProperties is called before Config Codec, so it cannot > necessarily know the server supported values. In this case the sound > server cannot fill in QoS correctly. > > The client-invoked SetConfiguration API also seems to have similar > issue. > > Probably: SelectProperties should be called twice, once to get > parameters for Config Codec, and then again to get parameters for > Config QoS. Or, there should be a separate "SelectCodec" and > "SelectQoS" calls. Calling "SelectProperties" twice could be simpler > for everyone. > > In client-invoked "SetConfiguration" API, one probably should only pass > in the parameters needed for Config Codec, and BlueZ should then make a > SelectProperties call to get the QoS ones once the server-side values > are known. > > I can take a look at this... Sound like a good idea, it means the actual configuration is always done via SelectProperties, that said we need to make sure this works with A2DP as well or state that it only applies to BAP, we could perhaps introduce a property to indicate that the endpoint implements SelectProperties. > > MediaTransport: > > "Delay" field only exists for A2DP in code. Yeah, the PresentationDelay was moved into QoS property. > "Volume" field only exists for A2DP in code. That is subject to change though. > "Links" is ucast only in code. Will mark it as unicast only. > "QoS.TargetLatency" does not exist in code. Maybe it should be exposed > for consistency, since it's expected as return from SelectProperties. Yeah, it seems we never implemented it, but I consider it a gap not a inconsistency with the code. > > SetConfiguration: > > SetConfiguration properties dict when called by BlueZ contains exactly > the properties of the transport. The documentation probably should say > that this is so. > > When called by client, the contents of the properties dict are expected > to be different. QoS parameters are not packed in a "QoS" dict. Maybe > they should be. The "PHY" key is also still a string here. Is it? Or perhaps I had changed it later on when splitting the documentation on a per-interface, at least on org.bluez.MediaEndpoint.rst there is no entry were PHY is a string, or you are talking about the actual code here? > The documented input parameters expected from client are wrong, the > documentation explains fields of struct bt_bap_pac_qos, but what the > code parses are that of struct bt_bap_qos. > > "MaximumLatency": no such field in code, seems to be called "Latency" > everywhere. The way Im designing these properties are that MediaEndpoint actually carries the preferences and MediaTransport the configuration as far as BAP is concerned, perhaps I shall actually state it though. > > SelectProperties: > > In code, the return parameter expects QoS parameters (struct > bt_bap_qos) to be packed in "QoS" dict. > > They are not packed in "QoS" dict in the input parameters. These are > not exactly QoS, but instead struct bt_bap_pac_qos, so not clear if > they should be packed. Input shall be considered the remote preferences and the output the local configuration. > The input and return parameters are not documented. The input QoS > contains fields of struct bt_bap_pac_qos. The return QoS has fields of > struct bt_bap_qos. I will attempt to clarify that. > > MediaEndpoint: > > The endpoints published by BlueZ only have the properties > "UUID", "Codec", "Capabilities", "Device". > > The documentation lists various other things, but they are not > implemented as server endpoint properties. > > As client endpoint properties: > > "RTN" is expected to be present in code, but is not documented. > > "MaximumLatency" "Location" "SupportedContext" "Context" are documented > but not present in code. > > > -- > Pauli Virtanen
diff --git a/doc/media-api.rst b/doc/media-api.rst index 34bf44e8ffbb..b37ae8f01630 100644 --- a/doc/media-api.rst +++ b/doc/media-api.rst @@ -710,28 +710,45 @@ void SetConfiguration(object transport, dict properties) properties: :array{byte} Capabilities [Mandatory]: + + See Endpoint.Capabilities property. + :array{byte} Metadata [ISO only]: - :byte CIG [ISO only]: - :byte CIS [ISO only]: - :uint32 Interval [ISO only]: - :bool Framing [ISO only]: - :string PHY [ISO only]: - :uint16 SDU [ISO only]: - :byte Retransmissions [ISO only]: - :uint16 Latency [ISO only]: - :uint32 Delay [ISO only]: - :uint8 TargetLatency [ISO Latency]: - :byte BIG [ISO broadcast only]: - :byte BIS [ISO broadcast only]: - :byte SyncInterval [ISO broadcast only]: - :byte Encryption [ISO broadcast only]: - :byte Options [ISO broadcast only]: - :uint16 Skip [ISO broadcast only]: - :uint16 SyncTimeout [ISO broadcast only]: - :byte SyncCteType [ISO broadcast only]: - :byte MSE [ISO broadcast only]: - :uint16 Timeout [ISO broadcast only]: - :array{byte} BroadcastCode [ISO broadcast only]: + + See Endpoint.Metadata property. + + :uint32 Location [ISO only]: + + See Endpoint.Location property. + + :byte Framing [ISO only]: + + See Endpoint.Framing property. + + :byte PHY [ISO only]: + + See Endpoint.PHY property. + + :uint16 MaximumLatency [ISO only]: + + See Endpoint.MaximumLatency property. + + :uint32 MinimumDelay [ISO only]: + + See Endpoint.MinimumDelay property. + + :uint32 MaximumDelay [ISO only]: + + See Endpoint.MaximumDelay property. + + :uint32 PreferredMinimumDelay [ISO only]: + + See Endpoint.PreferredMinimumDelay property. + + :uint32 PreferredMaximumDelay [ISO only]: + + See Endpoint.PreferredMaximumDelay property. + array{byte} SelectConfiguration(array{byte} capabilities) ````````````````````````````````````````````````````````` @@ -984,33 +1001,65 @@ dict QoS [readonly, optional, ISO only, experimental] Indicates configured CIG. + Possible values: + + :0x00 - 0xef: + + Valid ID range. + + :0xff: + + Auto allocate. + :byte CIS: Indicates configured CIS. - :uint32 Interval: + Possible values: - Indicates configured ISO interval. + :0x00 - 0xef: - :boolean Framing: + Valid ID range. + + :0xff: + + Auto allocate. + + :byte Framing: Indicates configured framing. - :byte PHY: + Possible values: - Indicates configured PHY. + :0x00: - :uint16 SDU: + Unframed. - Indicates configured SDU. + :0x01: - :byte Retransmissions: + Framed. - Indicates configured retransmissions. + :uint32 PresentationDelay: - :uint16 Latency: + Indicates configured transport presentation delay (us). - Indicates configured transport latency. + :byte TargetLatency: + + Indicates the requested target latency. + + Possible values: + + :0x01: + + Low Latency. + + :0x02: + + Balanced Latency/Reliability. + + :0x03: + + High Reliability. Possible values for Broadcast: @@ -1022,26 +1071,74 @@ dict QoS [readonly, optional, ISO only, experimental] Indicates configured BIS. - :uint32 SyncFactor: + :byte SyncFactor: - Indicates configured sync factor. + Indicates configured broadcast sync factor. - :uint32 Interval: + :byte Packing: - Indicates configured ISO interval. + Indicates configured packing. - :byte PHY: + :byte Framing: - Indicates configured PHY. + Indicates configured framing. - :uint16 SDU: + :byte Options: - Indicates configured maximum SDU. + Indicates configured broadcast options. + + :uint16 Skip: + + Indicates configured broadcast skip. :byte SyncTimeout: Indicates configured broadcast sync timeout. + :byte SyncType: + + Indicates configured broadcast sync CTE type. + + :byte MSE: + + Indicates configured broadcast MSE. + + :uint16 Timeout: + + Indicates configured broadcast timeout. + + Possible values for both Unicast and Broadcast: + + :uint32 Interval: + + Indicates configured ISO interval (us). + :uint16 Latency: - Indicates configured transport latency. + Indicates configured transport latency (ms). + + :uint16 SDU: + + Indicates configured maximum SDU. + + :byte PHY: + + Indicates configured PHY. + + Possible values: + + :bit 0: + + LE 1M + + :bit 1: + + LE 2M + + :bit 2: + + LE Coded + + :byte Retransmissions: + + Indicates configured retransmissions.
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> This reflect the last code changes adding the missing Broadcast properties. --- doc/media-api.rst | 179 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 138 insertions(+), 41 deletions(-)