diff mbox series

[BlueZ,RFC] media-api: Make QoS a single property

Message ID 20230824212153.11050-1-luiz.dentz@gmail.com (mailing list archive)
State Superseded
Headers show
Series [BlueZ,RFC] media-api: Make QoS a single property | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch success CheckPatch PASS
tedd_an/GitLint success Gitlint PASS
tedd_an/BuildEll success Build ELL PASS
tedd_an/BluezMake success Bluez Make PASS
tedd_an/MakeCheck success Bluez Make Check PASS
tedd_an/MakeDistcheck success Make Distcheck PASS
tedd_an/CheckValgrind success Check Valgrind PASS
tedd_an/CheckSmatch success CheckSparse PASS
tedd_an/bluezmakeextell success Make External ELL PASS
tedd_an/IncrementalBuild success Incremental Build PASS
tedd_an/ScanBuild success Scan Build PASS

Commit Message

Luiz Augusto von Dentz Aug. 24, 2023, 9:21 p.m. UTC
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This moves QoS related propertis to a single dictionary.
---
 doc/media-api.txt | 44 +++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

Comments

Luiz Augusto von Dentz Aug. 24, 2023, 9:28 p.m. UTC | #1
Hi,

On Thu, Aug 24, 2023 at 2:21 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This moves QoS related propertis to a single dictionary.
> ---
>  doc/media-api.txt | 44 +++++++++++++++++++++-----------------------
>  1 file changed, 21 insertions(+), 23 deletions(-)
>
> diff --git a/doc/media-api.txt b/doc/media-api.txt
> index 3a0ec38e244d..9f2482e73ac1 100644
> --- a/doc/media-api.txt
> +++ b/doc/media-api.txt
> @@ -816,42 +816,40 @@ Properties        object Device [readonly]
>                         Linked transport objects which the transport is
>                         associated with.
>
> -               byte CIG [ISO only, optional, experimental]
> +               dict QoS [ISO only, optional, experimental]
>
> -                       Indicates configured QoS CIG.
>                         Only present when QoS is configured.
>
> -               byte CIS [ISO only, optional, experimental]
> +                       Possible values for Unicast:
>
> -                       Indicates configured QoS CIS.
> -                       Only present when QoS is configured.
> +                       byte CIG
>
> -               uint32 Interval [ISO only, optional, experimental]
> +                               Indicates configured QoS CIG.
>
> -                       Indicates configured QoS interval.
> -                       Only present when QoS is configured.
> +                       byte CIS
>
> -               boolean Framing [ISO only, optional, experimental]
> +                               Only present when QoS is configured.
>
> -                       Indicates configured QoS framing.
> -                       Only present when QoS is configured.
> +                       uint32 Interval
>
> -               byte PHY [ISO only, optional, experimental]
> +                               Indicates configured QoS interval.
>
> -                       Indicates configured QoS PHY.
> -                       Only present when QoS is configured.
> +                       boolean Framing
>
> -               uint16 SDU [ISO only, optional, experimental]
> +                               Indicates configured QoS framing.
>
> -                       Indicates configured QoS SDU.
> -                       Only present when QoS is configured.
> +                       byte PHY
>
> -               byte Retransmissions [ISO only, optional, experimental]
> +                               Indicates configured QoS PHY.
>
> -                       Indicates configured QoS retransmissions.
> -                       Only present when QoS is configured.
> +                       uint16 SDU
>
> -               uint16 Latency [ISO only, optional, experimental]
> +                               Indicates configured QoS SDU.
>
> -                       Indicates configured QoS latency.
> -                       Only present when QoS is configured.
> +                       byte Retransmissions
> +
> +                               Indicates configured QoS retransmissions.
> +
> +                       uint16 Latency
> +
> +                               Indicates configured QoS latency.
> --
> 2.41.0

Let me know if you have anything against doing these changes, this
will break backward compatibility but since they are still marked as
experimental we can still do these changes.
bluez.test.bot@gmail.com Aug. 24, 2023, 10:33 p.m. UTC | #2
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=779177

---Test result---

Test Summary:
CheckPatch                    PASS      0.39 seconds
GitLint                       PASS      0.29 seconds
BuildEll                      PASS      26.84 seconds
BluezMake                     PASS      780.95 seconds
MakeCheck                     PASS      12.05 seconds
MakeDistcheck                 PASS      156.04 seconds
CheckValgrind                 PASS      251.09 seconds
CheckSmatch                   PASS      340.10 seconds
bluezmakeextell               PASS      102.47 seconds
IncrementalBuild              PASS      656.04 seconds
ScanBuild                     PASS      1031.51 seconds



---
Regards,
Linux Bluetooth
Pauli Virtanen Aug. 25, 2023, 3:44 p.m. UTC | #3
Hi Luiz,

to, 2023-08-24 kello 14:28 -0700, Luiz Augusto von Dentz kirjoitti:
> Hi,
> 
> On Thu, Aug 24, 2023 at 2:21 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> > 
> > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > 
> > This moves QoS related propertis to a single dictionary.
> > ---
> >  doc/media-api.txt | 44 +++++++++++++++++++++-----------------------
> >  1 file changed, 21 insertions(+), 23 deletions(-)
> > 
> > diff --git a/doc/media-api.txt b/doc/media-api.txt
> > index 3a0ec38e244d..9f2482e73ac1 100644
> > --- a/doc/media-api.txt
> > +++ b/doc/media-api.txt
> > @@ -816,42 +816,40 @@ Properties        object Device [readonly]
> >                         Linked transport objects which the transport is
> >                         associated with.
> > 
> > -               byte CIG [ISO only, optional, experimental]
> > +               dict QoS [ISO only, optional, experimental]
> > 
> > -                       Indicates configured QoS CIG.
> >                         Only present when QoS is configured.
> > 
> > -               byte CIS [ISO only, optional, experimental]
> > +                       Possible values for Unicast:
> > 
> > -                       Indicates configured QoS CIS.
> > -                       Only present when QoS is configured.
> > +                       byte CIG
> > 
> > -               uint32 Interval [ISO only, optional, experimental]
> > +                               Indicates configured QoS CIG.
> > 
> > -                       Indicates configured QoS interval.
> > -                       Only present when QoS is configured.
> > +                       byte CIS
> > 
> > -               boolean Framing [ISO only, optional, experimental]
> > +                               Only present when QoS is configured.
> > 
> > -                       Indicates configured QoS framing.
> > -                       Only present when QoS is configured.
> > +                       uint32 Interval
> > 
> > -               byte PHY [ISO only, optional, experimental]
> > +                               Indicates configured QoS interval.
> > 
> > -                       Indicates configured QoS PHY.
> > -                       Only present when QoS is configured.
> > +                       boolean Framing

While we are breaking things, it would be useful to unify this API with
SelectProperties/SetConfiguration, where PHY is string "1M" or "2M" not
byte.

Can we make it byte everywhere? I think the enum ultimately comes from
Core spec, so not sure the string names are that useful.

> > 
> > -               uint16 SDU [ISO only, optional, experimental]
> > +                               Indicates configured QoS framing.
> > 
> > -                       Indicates configured QoS SDU.
> > -                       Only present when QoS is configured.
> > +                       byte PHY
> > 
> > -               byte Retransmissions [ISO only, optional, experimental]
> > +                               Indicates configured QoS PHY.
> > 
> > -                       Indicates configured QoS retransmissions.
> > -                       Only present when QoS is configured.
> > +                       uint16 SDU
> > 
> > -               uint16 Latency [ISO only, optional, experimental]
> > +                               Indicates configured QoS SDU.
> > 
> > -                       Indicates configured QoS latency.
> > -                       Only present when QoS is configured.
> > +                       byte Retransmissions
> > +
> > +                               Indicates configured QoS retransmissions.
> > +
> > +                       uint16 Latency
> > +
> > +                               Indicates configured QoS latency.
> > --
> > 2.41.0
> 
> Let me know if you have anything against doing these changes, this
> will break backward compatibility but since they are still marked as
> experimental we can still do these changes.

I think it's fine if Pipewire targets BlueZ master branch as long as
the feature is experimental.
Luiz Augusto von Dentz Aug. 25, 2023, 6:34 p.m. UTC | #4
Hi Pauli,

On Fri, Aug 25, 2023 at 8:44 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Hi Luiz,
>
> to, 2023-08-24 kello 14:28 -0700, Luiz Augusto von Dentz kirjoitti:
> > Hi,
> >
> > On Thu, Aug 24, 2023 at 2:21 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > >
> > > This moves QoS related propertis to a single dictionary.
> > > ---
> > >  doc/media-api.txt | 44 +++++++++++++++++++++-----------------------
> > >  1 file changed, 21 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/doc/media-api.txt b/doc/media-api.txt
> > > index 3a0ec38e244d..9f2482e73ac1 100644
> > > --- a/doc/media-api.txt
> > > +++ b/doc/media-api.txt
> > > @@ -816,42 +816,40 @@ Properties        object Device [readonly]
> > >                         Linked transport objects which the transport is
> > >                         associated with.
> > >
> > > -               byte CIG [ISO only, optional, experimental]
> > > +               dict QoS [ISO only, optional, experimental]
> > >
> > > -                       Indicates configured QoS CIG.
> > >                         Only present when QoS is configured.
> > >
> > > -               byte CIS [ISO only, optional, experimental]
> > > +                       Possible values for Unicast:
> > >
> > > -                       Indicates configured QoS CIS.
> > > -                       Only present when QoS is configured.
> > > +                       byte CIG
> > >
> > > -               uint32 Interval [ISO only, optional, experimental]
> > > +                               Indicates configured QoS CIG.
> > >
> > > -                       Indicates configured QoS interval.
> > > -                       Only present when QoS is configured.
> > > +                       byte CIS
> > >
> > > -               boolean Framing [ISO only, optional, experimental]
> > > +                               Only present when QoS is configured.
> > >
> > > -                       Indicates configured QoS framing.
> > > -                       Only present when QoS is configured.
> > > +                       uint32 Interval
> > >
> > > -               byte PHY [ISO only, optional, experimental]
> > > +                               Indicates configured QoS interval.
> > >
> > > -                       Indicates configured QoS PHY.
> > > -                       Only present when QoS is configured.
> > > +                       boolean Framing
>
> While we are breaking things, it would be useful to unify this API with
> SelectProperties/SetConfiguration, where PHY is string "1M" or "2M" not
> byte.
>
> Can we make it byte everywhere? I think the enum ultimately comes from
> Core spec, so not sure the string names are that useful.

Fair enough, there was already some feedback that it doesn't
correspond to the values when read to the socket, so yeah I will
change that was well.

> > >
> > > -               uint16 SDU [ISO only, optional, experimental]
> > > +                               Indicates configured QoS framing.
> > >
> > > -                       Indicates configured QoS SDU.
> > > -                       Only present when QoS is configured.
> > > +                       byte PHY
> > >
> > > -               byte Retransmissions [ISO only, optional, experimental]
> > > +                               Indicates configured QoS PHY.
> > >
> > > -                       Indicates configured QoS retransmissions.
> > > -                       Only present when QoS is configured.
> > > +                       uint16 SDU
> > >
> > > -               uint16 Latency [ISO only, optional, experimental]
> > > +                               Indicates configured QoS SDU.
> > >
> > > -                       Indicates configured QoS latency.
> > > -                       Only present when QoS is configured.
> > > +                       byte Retransmissions
> > > +
> > > +                               Indicates configured QoS retransmissions.
> > > +
> > > +                       uint16 Latency
> > > +
> > > +                               Indicates configured QoS latency.
> > > --
> > > 2.41.0
> >
> > Let me know if you have anything against doing these changes, this
> > will break backward compatibility but since they are still marked as
> > experimental we can still do these changes.
>
> I think it's fine if Pipewire targets BlueZ master branch as long as
> the feature is experimental.

We are planning to do a release first before starting to do these changes.

>
> --
> Pauli Virtanen
diff mbox series

Patch

diff --git a/doc/media-api.txt b/doc/media-api.txt
index 3a0ec38e244d..9f2482e73ac1 100644
--- a/doc/media-api.txt
+++ b/doc/media-api.txt
@@ -816,42 +816,40 @@  Properties	object Device [readonly]
 			Linked transport objects which the transport is
 			associated with.
 
-		byte CIG [ISO only, optional, experimental]
+		dict QoS [ISO only, optional, experimental]
 
-			Indicates configured QoS CIG.
 			Only present when QoS is configured.
 
-		byte CIS [ISO only, optional, experimental]
+			Possible values for Unicast:
 
-			Indicates configured QoS CIS.
-			Only present when QoS is configured.
+			byte CIG
 
-		uint32 Interval [ISO only, optional, experimental]
+				Indicates configured QoS CIG.
 
-			Indicates configured QoS interval.
-			Only present when QoS is configured.
+			byte CIS
 
-		boolean Framing [ISO only, optional, experimental]
+				Only present when QoS is configured.
 
-			Indicates configured QoS framing.
-			Only present when QoS is configured.
+			uint32 Interval
 
-		byte PHY [ISO only, optional, experimental]
+				Indicates configured QoS interval.
 
-			Indicates configured QoS PHY.
-			Only present when QoS is configured.
+			boolean Framing
 
-		uint16 SDU [ISO only, optional, experimental]
+				Indicates configured QoS framing.
 
-			Indicates configured QoS SDU.
-			Only present when QoS is configured.
+			byte PHY
 
-		byte Retransmissions [ISO only, optional, experimental]
+				Indicates configured QoS PHY.
 
-			Indicates configured QoS retransmissions.
-			Only present when QoS is configured.
+			uint16 SDU
 
-		uint16 Latency [ISO only, optional, experimental]
+				Indicates configured QoS SDU.
 
-			Indicates configured QoS latency.
-			Only present when QoS is configured.
+			byte Retransmissions
+
+				Indicates configured QoS retransmissions.
+
+			uint16 Latency
+
+				Indicates configured QoS latency.