[RFC,v2,1/1] adapter - D-Bus API for querying the adapter's capabilities

Commit Message

Yu Liu Aug. 17, 2020, 9:25 p.m. UTC

From: Archie Pusaka <[e-mail]>

Initially this is introduced to query whether WBS is supported by the adapter,
the API is generic enough to be extended to support querying others in
the future.

Reviewed-by: [e-mail]

Signed-off-by: Yu Liu <[e-mail]>
---

Changes in v2:
- Return an array of strings instead of a dict

Changes in v1:
- Initial change

 doc/adapter-api.txt | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Luiz Augusto von Dentz Aug. 17, 2020, 11:16 p.m. UTC | #1

Hi Marcel,

On Mon, Aug 17, 2020 at 4:07 PM Yu Liu <[e-mail]> wrote:
>
> From: Archie Pusaka <[e-mail]>
>
> Initially this is introduced to query whether WBS is supported by the adapter,
> the API is generic enough to be extended to support querying others in
> the future.
>
> Reviewed-by: [e-mail]
>
> Signed-off-by: Yu Liu <[e-mail]>
> ---
>
> Changes in v2:
> - Return an array of strings instead of a dict
>
> Changes in v1:
> - Initial change
>
>  doc/adapter-api.txt | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
> index 1a7255750..8fbcadb54 100644
> --- a/doc/adapter-api.txt
> +++ b/doc/adapter-api.txt
> @@ -204,6 +204,18 @@ Methods            void StartDiscovery()
>                                          org.bluez.Error.NotReady
>                                          org.bluez.Error.Failed
>
> +               array{string} GetCapabilities()
> +
> +                       This method returns a list of supported
> +                       capabilities that is populated when the adapter
> +                       initiated.
> +
> +                       Possible values:
> +                               "wbs" - Wide band speech

Btw, should we stick to use wbs terminology here, or we should
actually use the HCI feature/command, because wbs has actually to be
implemented by the HFP afaik this is only indicating that the
controller is able to notify packets drops, etc, with use of erroneous
command. Perhaps we should actually use the term PLC (Packet Loss
Concealment) instead since that seems to be the real capability here,
afaik WBS does not actually require PLC.

> +
> +                       Possible errors: org.bluez.Error.NotReady
> +                                        org.bluez.Error.Failed
> +
>  Properties     string Address [readonly]
>
>                         The Bluetooth device address.
> --
> 2.28.0.220.ged08abb693-goog
>

Yu Liu Aug. 20, 2020, 5:20 p.m. UTC | #2

Friendly ping for comments from Marcel. Thanks.


On Mon, Aug 17, 2020 at 4:17 PM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Marcel,
>
> On Mon, Aug 17, 2020 at 4:07 PM Yu Liu <[e-mail]> wrote:
> >
> > From: Archie Pusaka <[e-mail]>
> >
> > Initially this is introduced to query whether WBS is supported by the adapter,
> > the API is generic enough to be extended to support querying others in
> > the future.
> >
> > Reviewed-by: [e-mail]
> >
> > Signed-off-by: Yu Liu <[e-mail]>
> > ---
> >
> > Changes in v2:
> > - Return an array of strings instead of a dict
> >
> > Changes in v1:
> > - Initial change
> >
> >  doc/adapter-api.txt | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
> > index 1a7255750..8fbcadb54 100644
> > --- a/doc/adapter-api.txt
> > +++ b/doc/adapter-api.txt
> > @@ -204,6 +204,18 @@ Methods            void StartDiscovery()
> >                                          org.bluez.Error.NotReady
> >                                          org.bluez.Error.Failed
> >
> > +               array{string} GetCapabilities()
> > +
> > +                       This method returns a list of supported
> > +                       capabilities that is populated when the adapter
> > +                       initiated.
> > +
> > +                       Possible values:
> > +                               "wbs" - Wide band speech
>
> Btw, should we stick to use wbs terminology here, or we should
> actually use the HCI feature/command, because wbs has actually to be
> implemented by the HFP afaik this is only indicating that the
> controller is able to notify packets drops, etc, with use of erroneous
> command. Perhaps we should actually use the term PLC (Packet Loss
> Concealment) instead since that seems to be the real capability here,
> afaik WBS does not actually require PLC.
>
> > +
> > +                       Possible errors: org.bluez.Error.NotReady
> > +                                        org.bluez.Error.Failed
> > +
> >  Properties     string Address [readonly]
> >
> >                         The Bluetooth device address.
> > --
> > 2.28.0.220.ged08abb693-goog
> >
>
>
> --
> Luiz Augusto von Dentz

Yu Liu Aug. 31, 2020, 9:44 p.m. UTC | #3

+Alain Michaud

Hi Marcel,

Can you please comment on the cl as well as Luiz's suggestion? Thanks.

On Thu, Aug 20, 2020 at 10:20 AM Yu Liu <[e-mail]> wrote:
>
> Friendly ping for comments from Marcel. Thanks.
>
>
> On Mon, Aug 17, 2020 at 4:17 PM Luiz Augusto von Dentz
> <[e-mail]> wrote:
> >
> > Hi Marcel,
> >
> > On Mon, Aug 17, 2020 at 4:07 PM Yu Liu <[e-mail]> wrote:
> > >
> > > From: Archie Pusaka <[e-mail]>
> > >
> > > Initially this is introduced to query whether WBS is supported by the adapter,
> > > the API is generic enough to be extended to support querying others in
> > > the future.
> > >
> > > Reviewed-by: [e-mail]
> > >
> > > Signed-off-by: Yu Liu <[e-mail]>
> > > ---
> > >
> > > Changes in v2:
> > > - Return an array of strings instead of a dict
> > >
> > > Changes in v1:
> > > - Initial change
> > >
> > >  doc/adapter-api.txt | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
> > > index 1a7255750..8fbcadb54 100644
> > > --- a/doc/adapter-api.txt
> > > +++ b/doc/adapter-api.txt
> > > @@ -204,6 +204,18 @@ Methods            void StartDiscovery()
> > >                                          org.bluez.Error.NotReady
> > >                                          org.bluez.Error.Failed
> > >
> > > +               array{string} GetCapabilities()
> > > +
> > > +                       This method returns a list of supported
> > > +                       capabilities that is populated when the adapter
> > > +                       initiated.
> > > +
> > > +                       Possible values:
> > > +                               "wbs" - Wide band speech
> >
> > Btw, should we stick to use wbs terminology here, or we should
> > actually use the HCI feature/command, because wbs has actually to be
> > implemented by the HFP afaik this is only indicating that the
> > controller is able to notify packets drops, etc, with use of erroneous
> > command. Perhaps we should actually use the term PLC (Packet Loss
> > Concealment) instead since that seems to be the real capability here,
> > afaik WBS does not actually require PLC.
> >
> > > +
> > > +                       Possible errors: org.bluez.Error.NotReady
> > > +                                        org.bluez.Error.Failed
> > > +
> > >  Properties     string Address [readonly]
> > >
> > >                         The Bluetooth device address.
> > > --
> > > 2.28.0.220.ged08abb693-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz

Luiz Augusto von Dentz Sept. 1, 2020, 5:47 p.m. UTC | #4

Hi Alain,

On Tue, Sep 1, 2020 at 8:43 AM Alain Michaud <[e-mail]> wrote:
>
> Hi Marcel/Luiz,
>
> I'd be particularly interested in seeing your opinion on whether this could be better described as some form of SCO socket property... even if this is indeed an adapter property.

Yep, but wasn't that supposed to be what BT_PKT_STATUS is for? I mean
one can just attempt to read it with getsockopt and in case of returns
an error it means the controller does not support it, that said it
doesn't look like we do check the adapter features when using
BT_PKT_STATUS, should that be checking if HCI_WIDEBAND_SPEECH_ENABLED
is set?

Also it doesn't seem we have updated userspace to support
BT_PKT_STATUS, we should probably have something to test it via
isotest and perhaps create a iso-tester.c to validate all the options.

> Thanks,
> Alain
>
> On Mon, Aug 31, 2020 at 5:44 PM Yu Liu <[e-mail]> wrote:
>>
>> +Alain Michaud
>>
>> Hi Marcel,
>>
>> Can you please comment on the cl as well as Luiz's suggestion? Thanks.
>>
>> On Thu, Aug 20, 2020 at 10:20 AM Yu Liu <[e-mail]> wrote:
>> >
>> > Friendly ping for comments from Marcel. Thanks.
>> >
>> >
>> > On Mon, Aug 17, 2020 at 4:17 PM Luiz Augusto von Dentz
>> > <[e-mail]> wrote:
>> > >
>> > > Hi Marcel,
>> > >
>> > > On Mon, Aug 17, 2020 at 4:07 PM Yu Liu <[e-mail]> wrote:
>> > > >
>> > > > From: Archie Pusaka <[e-mail]>
>> > > >
>> > > > Initially this is introduced to query whether WBS is supported by the adapter,
>> > > > the API is generic enough to be extended to support querying others in
>> > > > the future.
>> > > >
>> > > > Reviewed-by: [e-mail]
>> > > >
>> > > > Signed-off-by: Yu Liu <[e-mail]>
>> > > > ---
>> > > >
>> > > > Changes in v2:
>> > > > - Return an array of strings instead of a dict
>> > > >
>> > > > Changes in v1:
>> > > > - Initial change
>> > > >
>> > > >  doc/adapter-api.txt | 12 ++++++++++++
>> > > >  1 file changed, 12 insertions(+)
>> > > >
>> > > > diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
>> > > > index 1a7255750..8fbcadb54 100644
>> > > > --- a/doc/adapter-api.txt
>> > > > +++ b/doc/adapter-api.txt
>> > > > @@ -204,6 +204,18 @@ Methods            void StartDiscovery()
>> > > >                                          org.bluez.Error.NotReady
>> > > >                                          org.bluez.Error.Failed
>> > > >
>> > > > +               array{string} GetCapabilities()
>> > > > +
>> > > > +                       This method returns a list of supported
>> > > > +                       capabilities that is populated when the adapter
>> > > > +                       initiated.
>> > > > +
>> > > > +                       Possible values:
>> > > > +                               "wbs" - Wide band speech
>> > >
>> > > Btw, should we stick to use wbs terminology here, or we should
>> > > actually use the HCI feature/command, because wbs has actually to be
>> > > implemented by the HFP afaik this is only indicating that the
>> > > controller is able to notify packets drops, etc, with use of erroneous
>> > > command. Perhaps we should actually use the term PLC (Packet Loss
>> > > Concealment) instead since that seems to be the real capability here,
>> > > afaik WBS does not actually require PLC.
>> > >
>> > > > +
>> > > > +                       Possible errors: org.bluez.Error.NotReady
>> > > > +                                        org.bluez.Error.Failed
>> > > > +
>> > > >  Properties     string Address [readonly]
>> > > >
>> > > >                         The Bluetooth device address.
>> > > > --
>> > > > 2.28.0.220.ged08abb693-goog
>> > > >
>> > >
>> > >
>> > > --
>> > > Luiz Augusto von Dentz

Alain Michaud Sept. 1, 2020, 6:59 p.m. UTC | #5

Hi Luiz,


On Tue, Sep 1, 2020 at 1:47 PM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Alain,
>
> On Tue, Sep 1, 2020 at 8:43 AM Alain Michaud <[e-mail]> wrote:
> >
> > Hi Marcel/Luiz,
> >
> > I'd be particularly interested in seeing your opinion on whether this could be better described as some form of SCO socket property... even if this is indeed an adapter property.
>
> Yep, but wasn't that supposed to be what BT_PKT_STATUS is for? I mean
> one can just attempt to read it with getsockopt and in case of returns
> an error it means the controller does not support it, that said it
> doesn't look like we do check the adapter features when using
> BT_PKT_STATUS, should that be checking if HCI_WIDEBAND_SPEECH_ENABLED
> is set?
The problem here is that this will be after the connection is created
and a packet is exchanged.  In this context, we'd like to know if the
controller supports it ahead of creating a sco connection so we can
choose to use the headset at all.  For example, there are devices and
circumstances where using the device's built-in mic and A2DP will
yield a better experience for the user so the platform may choose that
as a default for the user rather than degrading down to narrow-band.

>
> Also it doesn't seem we have updated userspace to support
> BT_PKT_STATUS, we should probably have something to test it via
> isotest and perhaps create a iso-tester.c to validate all the options.
>
> > Thanks,
> > Alain
> >
> > On Mon, Aug 31, 2020 at 5:44 PM Yu Liu <[e-mail]> wrote:
> >>
> >> +Alain Michaud
> >>
> >> Hi Marcel,
> >>
> >> Can you please comment on the cl as well as Luiz's suggestion? Thanks.
> >>
> >> On Thu, Aug 20, 2020 at 10:20 AM Yu Liu <[e-mail]> wrote:
> >> >
> >> > Friendly ping for comments from Marcel. Thanks.
> >> >
> >> >
> >> > On Mon, Aug 17, 2020 at 4:17 PM Luiz Augusto von Dentz
> >> > <[e-mail]> wrote:
> >> > >
> >> > > Hi Marcel,
> >> > >
> >> > > On Mon, Aug 17, 2020 at 4:07 PM Yu Liu <[e-mail]> wrote:
> >> > > >
> >> > > > From: Archie Pusaka <[e-mail]>
> >> > > >
> >> > > > Initially this is introduced to query whether WBS is supported by the adapter,
> >> > > > the API is generic enough to be extended to support querying others in
> >> > > > the future.
> >> > > >
> >> > > > Reviewed-by: [e-mail]
> >> > > >
> >> > > > Signed-off-by: Yu Liu <[e-mail]>
> >> > > > ---
> >> > > >
> >> > > > Changes in v2:
> >> > > > - Return an array of strings instead of a dict
> >> > > >
> >> > > > Changes in v1:
> >> > > > - Initial change
> >> > > >
> >> > > >  doc/adapter-api.txt | 12 ++++++++++++
> >> > > >  1 file changed, 12 insertions(+)
> >> > > >
> >> > > > diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
> >> > > > index 1a7255750..8fbcadb54 100644
> >> > > > --- a/doc/adapter-api.txt
> >> > > > +++ b/doc/adapter-api.txt
> >> > > > @@ -204,6 +204,18 @@ Methods            void StartDiscovery()
> >> > > >                                          org.bluez.Error.NotReady
> >> > > >                                          org.bluez.Error.Failed
> >> > > >
> >> > > > +               array{string} GetCapabilities()
> >> > > > +
> >> > > > +                       This method returns a list of supported
> >> > > > +                       capabilities that is populated when the adapter
> >> > > > +                       initiated.
> >> > > > +
> >> > > > +                       Possible values:
> >> > > > +                               "wbs" - Wide band speech
> >> > >
> >> > > Btw, should we stick to use wbs terminology here, or we should
> >> > > actually use the HCI feature/command, because wbs has actually to be
> >> > > implemented by the HFP afaik this is only indicating that the
> >> > > controller is able to notify packets drops, etc, with use of erroneous
> >> > > command. Perhaps we should actually use the term PLC (Packet Loss
> >> > > Concealment) instead since that seems to be the real capability here,
> >> > > afaik WBS does not actually require PLC.
> >> > >
> >> > > > +
> >> > > > +                       Possible errors: org.bluez.Error.NotReady
> >> > > > +                                        org.bluez.Error.Failed
> >> > > > +
> >> > > >  Properties     string Address [readonly]
> >> > > >
> >> > > >                         The Bluetooth device address.
> >> > > > --
> >> > > > 2.28.0.220.ged08abb693-goog
> >> > > >
> >> > >
> >> > >
> >> > > --
> >> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

Luiz Augusto von Dentz Sept. 1, 2020, 8:47 p.m. UTC | #6

Hi Alain,

On Tue, Sep 1, 2020 at 12:00 PM Alain Michaud <[e-mail]> wrote:
>
> Hi Luiz,
>
>
> On Tue, Sep 1, 2020 at 1:47 PM Luiz Augusto von Dentz
> <[e-mail]> wrote:
> >
> > Hi Alain,
> >
> > On Tue, Sep 1, 2020 at 8:43 AM Alain Michaud <[e-mail]> wrote:
> > >
> > > Hi Marcel/Luiz,
> > >
> > > I'd be particularly interested in seeing your opinion on whether this could be better described as some form of SCO socket property... even if this is indeed an adapter property.
> >
> > Yep, but wasn't that supposed to be what BT_PKT_STATUS is for? I mean
> > one can just attempt to read it with getsockopt and in case of returns
> > an error it means the controller does not support it, that said it
> > doesn't look like we do check the adapter features when using
> > BT_PKT_STATUS, should that be checking if HCI_WIDEBAND_SPEECH_ENABLED
> > is set?
> The problem here is that this will be after the connection is created
> and a packet is exchanged.  In this context, we'd like to know if the
> controller supports it ahead of creating a sco connection so we can
> choose to use the headset at all.  For example, there are devices and
> circumstances where using the device's built-in mic and A2DP will
> yield a better experience for the user so the platform may choose that
> as a default for the user rather than degrading down to narrow-band.

You can use get/setsockopt after bind, so I wonder if that is really a
problem here, in fact BT_VOICE wouldn't work if we couldn't use socket
options before the connection is made see how it is used in ofono for
instance:

https://git.kernel.org/pub/scm/network/ofono/ofono.git/tree/src/handsfree-audio.c#n105

Also in order to use WBS one will need to use BT_VOICE anyway,
actually that all you need for WBS with mSBC while BT_PKT_STATUS will
indicate the possibility to do PLC, not sure why we are mixing these
things together or does HFP mandates mSBC to implement PLC nowadays?

> >
> > Also it doesn't seem we have updated userspace to support
> > BT_PKT_STATUS, we should probably have something to test it via
> > isotest and perhaps create a iso-tester.c to validate all the options.
> >
> > > Thanks,
> > > Alain
> > >
> > > On Mon, Aug 31, 2020 at 5:44 PM Yu Liu <[e-mail]> wrote:
> > >>
> > >> +Alain Michaud
> > >>
> > >> Hi Marcel,
> > >>
> > >> Can you please comment on the cl as well as Luiz's suggestion? Thanks.
> > >>
> > >> On Thu, Aug 20, 2020 at 10:20 AM Yu Liu <[e-mail]> wrote:
> > >> >
> > >> > Friendly ping for comments from Marcel. Thanks.
> > >> >
> > >> >
> > >> > On Mon, Aug 17, 2020 at 4:17 PM Luiz Augusto von Dentz
> > >> > <[e-mail]> wrote:
> > >> > >
> > >> > > Hi Marcel,
> > >> > >
> > >> > > On Mon, Aug 17, 2020 at 4:07 PM Yu Liu <[e-mail]> wrote:
> > >> > > >
> > >> > > > From: Archie Pusaka <[e-mail]>
> > >> > > >
> > >> > > > Initially this is introduced to query whether WBS is supported by the adapter,
> > >> > > > the API is generic enough to be extended to support querying others in
> > >> > > > the future.
> > >> > > >
> > >> > > > Reviewed-by: [e-mail]
> > >> > > >
> > >> > > > Signed-off-by: Yu Liu <[e-mail]>
> > >> > > > ---
> > >> > > >
> > >> > > > Changes in v2:
> > >> > > > - Return an array of strings instead of a dict
> > >> > > >
> > >> > > > Changes in v1:
> > >> > > > - Initial change
> > >> > > >
> > >> > > >  doc/adapter-api.txt | 12 ++++++++++++
> > >> > > >  1 file changed, 12 insertions(+)
> > >> > > >
> > >> > > > diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
> > >> > > > index 1a7255750..8fbcadb54 100644
> > >> > > > --- a/doc/adapter-api.txt
> > >> > > > +++ b/doc/adapter-api.txt
> > >> > > > @@ -204,6 +204,18 @@ Methods            void StartDiscovery()
> > >> > > >                                          org.bluez.Error.NotReady
> > >> > > >                                          org.bluez.Error.Failed
> > >> > > >
> > >> > > > +               array{string} GetCapabilities()
> > >> > > > +
> > >> > > > +                       This method returns a list of supported
> > >> > > > +                       capabilities that is populated when the adapter
> > >> > > > +                       initiated.
> > >> > > > +
> > >> > > > +                       Possible values:
> > >> > > > +                               "wbs" - Wide band speech
> > >> > >
> > >> > > Btw, should we stick to use wbs terminology here, or we should
> > >> > > actually use the HCI feature/command, because wbs has actually to be
> > >> > > implemented by the HFP afaik this is only indicating that the
> > >> > > controller is able to notify packets drops, etc, with use of erroneous
> > >> > > command. Perhaps we should actually use the term PLC (Packet Loss
> > >> > > Concealment) instead since that seems to be the real capability here,
> > >> > > afaik WBS does not actually require PLC.
> > >> > >
> > >> > > > +
> > >> > > > +                       Possible errors: org.bluez.Error.NotReady
> > >> > > > +                                        org.bluez.Error.Failed
> > >> > > > +
> > >> > > >  Properties     string Address [readonly]
> > >> > > >
> > >> > > >                         The Bluetooth device address.
> > >> > > > --
> > >> > > > 2.28.0.220.ged08abb693-goog
> > >> > > >
> > >> > >
> > >> > >
> > >> > > --
> > >> > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz

Alain Michaud Sept. 1, 2020, 8:55 p.m. UTC | #7

Hi Luiz,


On Tue, Sep 1, 2020 at 4:48 PM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Alain,
>
> On Tue, Sep 1, 2020 at 12:00 PM Alain Michaud <[e-mail]> wrote:
> >
> > Hi Luiz,
> >
> >
> > On Tue, Sep 1, 2020 at 1:47 PM Luiz Augusto von Dentz
> > <[e-mail]> wrote:
> > >
> > > Hi Alain,
> > >
> > > On Tue, Sep 1, 2020 at 8:43 AM Alain Michaud <[e-mail]> wrote:
> > > >
> > > > Hi Marcel/Luiz,
> > > >
> > > > I'd be particularly interested in seeing your opinion on whether this could be better described as some form of SCO socket property... even if this is indeed an adapter property.
> > >
> > > Yep, but wasn't that supposed to be what BT_PKT_STATUS is for? I mean
> > > one can just attempt to read it with getsockopt and in case of returns
> > > an error it means the controller does not support it, that said it
> > > doesn't look like we do check the adapter features when using
> > > BT_PKT_STATUS, should that be checking if HCI_WIDEBAND_SPEECH_ENABLED
> > > is set?
> > The problem here is that this will be after the connection is created
> > and a packet is exchanged.  In this context, we'd like to know if the
> > controller supports it ahead of creating a sco connection so we can
> > choose to use the headset at all.  For example, there are devices and
> > circumstances where using the device's built-in mic and A2DP will
> > yield a better experience for the user so the platform may choose that
> > as a default for the user rather than degrading down to narrow-band.
>
> You can use get/setsockopt after bind, so I wonder if that is really a
> problem here, in fact BT_VOICE wouldn't work if we couldn't use socket
> options before the connection is made see how it is used in ofono for
> instance:
>
> https://git.kernel.org/pub/scm/network/ofono/ofono.git/tree/src/handsfree-audio.c#n105
>
> Also in order to use WBS one will need to use BT_VOICE anyway,
> actually that all you need for WBS with mSBC while BT_PKT_STATUS will
> indicate the possibility to do PLC, not sure why we are mixing these
> things together or does HFP mandates mSBC to implement PLC nowadays?
This is a design choice, one that must be done before a device
connected on our platform.  Note ChromeOS doesn't use ofono.  It's not
just about choosing to do mSBC, but rather about which mic the overall
platform will use (onboard or from the headset/speaker).

>
> > >
> > > Also it doesn't seem we have updated userspace to support
> > > BT_PKT_STATUS, we should probably have something to test it via
> > > isotest and perhaps create a iso-tester.c to validate all the options.
> > >
> > > > Thanks,
> > > > Alain
> > > >
> > > > On Mon, Aug 31, 2020 at 5:44 PM Yu Liu <[e-mail]> wrote:
> > > >>
> > > >> +Alain Michaud
> > > >>
> > > >> Hi Marcel,
> > > >>
> > > >> Can you please comment on the cl as well as Luiz's suggestion? Thanks.
> > > >>
> > > >> On Thu, Aug 20, 2020 at 10:20 AM Yu Liu <[e-mail]> wrote:
> > > >> >
> > > >> > Friendly ping for comments from Marcel. Thanks.
> > > >> >
> > > >> >
> > > >> > On Mon, Aug 17, 2020 at 4:17 PM Luiz Augusto von Dentz
> > > >> > <[e-mail]> wrote:
> > > >> > >
> > > >> > > Hi Marcel,
> > > >> > >
> > > >> > > On Mon, Aug 17, 2020 at 4:07 PM Yu Liu <[e-mail]> wrote:
> > > >> > > >
> > > >> > > > From: Archie Pusaka <[e-mail]>
> > > >> > > >
> > > >> > > > Initially this is introduced to query whether WBS is supported by the adapter,
> > > >> > > > the API is generic enough to be extended to support querying others in
> > > >> > > > the future.
> > > >> > > >
> > > >> > > > Reviewed-by: [e-mail]
> > > >> > > >
> > > >> > > > Signed-off-by: Yu Liu <[e-mail]>
> > > >> > > > ---
> > > >> > > >
> > > >> > > > Changes in v2:
> > > >> > > > - Return an array of strings instead of a dict
> > > >> > > >
> > > >> > > > Changes in v1:
> > > >> > > > - Initial change
> > > >> > > >
> > > >> > > >  doc/adapter-api.txt | 12 ++++++++++++
> > > >> > > >  1 file changed, 12 insertions(+)
> > > >> > > >
> > > >> > > > diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
> > > >> > > > index 1a7255750..8fbcadb54 100644
> > > >> > > > --- a/doc/adapter-api.txt
> > > >> > > > +++ b/doc/adapter-api.txt
> > > >> > > > @@ -204,6 +204,18 @@ Methods            void StartDiscovery()
> > > >> > > >                                          org.bluez.Error.NotReady
> > > >> > > >                                          org.bluez.Error.Failed
> > > >> > > >
> > > >> > > > +               array{string} GetCapabilities()
> > > >> > > > +
> > > >> > > > +                       This method returns a list of supported
> > > >> > > > +                       capabilities that is populated when the adapter
> > > >> > > > +                       initiated.
> > > >> > > > +
> > > >> > > > +                       Possible values:
> > > >> > > > +                               "wbs" - Wide band speech
> > > >> > >
> > > >> > > Btw, should we stick to use wbs terminology here, or we should
> > > >> > > actually use the HCI feature/command, because wbs has actually to be
> > > >> > > implemented by the HFP afaik this is only indicating that the
> > > >> > > controller is able to notify packets drops, etc, with use of erroneous
> > > >> > > command. Perhaps we should actually use the term PLC (Packet Loss
> > > >> > > Concealment) instead since that seems to be the real capability here,
> > > >> > > afaik WBS does not actually require PLC.
> > > >> > >
> > > >> > > > +
> > > >> > > > +                       Possible errors: org.bluez.Error.NotReady
> > > >> > > > +                                        org.bluez.Error.Failed
> > > >> > > > +
> > > >> > > >  Properties     string Address [readonly]
> > > >> > > >
> > > >> > > >                         The Bluetooth device address.
> > > >> > > > --
> > > >> > > > 2.28.0.220.ged08abb693-goog
> > > >> > > >
> > > >> > >
> > > >> > >
> > > >> > > --
> > > >> > > Luiz Augusto von Dentz
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

Luiz Augusto von Dentz Sept. 1, 2020, 10:49 p.m. UTC | #8

Hi Alain,

On Tue, Sep 1, 2020 at 1:55 PM Alain Michaud <[e-mail]> wrote:
>
> Hi Luiz,
>
>
> On Tue, Sep 1, 2020 at 4:48 PM Luiz Augusto von Dentz
> <[e-mail]> wrote:
> >
> > Hi Alain,
> >
> > On Tue, Sep 1, 2020 at 12:00 PM Alain Michaud <[e-mail]> wrote:
> > >
> > > Hi Luiz,
> > >
> > >
> > > On Tue, Sep 1, 2020 at 1:47 PM Luiz Augusto von Dentz
> > > <[e-mail]> wrote:
> > > >
> > > > Hi Alain,
> > > >
> > > > On Tue, Sep 1, 2020 at 8:43 AM Alain Michaud <[e-mail]> wrote:
> > > > >
> > > > > Hi Marcel/Luiz,
> > > > >
> > > > > I'd be particularly interested in seeing your opinion on whether this could be better described as some form of SCO socket property... even if this is indeed an adapter property.
> > > >
> > > > Yep, but wasn't that supposed to be what BT_PKT_STATUS is for? I mean
> > > > one can just attempt to read it with getsockopt and in case of returns
> > > > an error it means the controller does not support it, that said it
> > > > doesn't look like we do check the adapter features when using
> > > > BT_PKT_STATUS, should that be checking if HCI_WIDEBAND_SPEECH_ENABLED
> > > > is set?
> > > The problem here is that this will be after the connection is created
> > > and a packet is exchanged.  In this context, we'd like to know if the
> > > controller supports it ahead of creating a sco connection so we can
> > > choose to use the headset at all.  For example, there are devices and
> > > circumstances where using the device's built-in mic and A2DP will
> > > yield a better experience for the user so the platform may choose that
> > > as a default for the user rather than degrading down to narrow-band.
> >
> > You can use get/setsockopt after bind, so I wonder if that is really a
> > problem here, in fact BT_VOICE wouldn't work if we couldn't use socket
> > options before the connection is made see how it is used in ofono for
> > instance:
> >
> > https://git.kernel.org/pub/scm/network/ofono/ofono.git/tree/src/handsfree-audio.c#n105
> >
> > Also in order to use WBS one will need to use BT_VOICE anyway,
> > actually that all you need for WBS with mSBC while BT_PKT_STATUS will
> > indicate the possibility to do PLC, not sure why we are mixing these
> > things together or does HFP mandates mSBC to implement PLC nowadays?
> This is a design choice, one that must be done before a device
> connected on our platform.  Note ChromeOS doesn't use ofono.  It's not
> just about choosing to do mSBC, but rather about which mic the overall
> platform will use (onboard or from the headset/speaker).

Got it, I just don't like the naming because while it is perfectly
fine for ChromeOS to choose to only do WBS when BT_PKT_STATUS is
available, other platforms my not required that, specially since BlueZ
may also be used in carkits/IVI systems which may have been built
without PLC support.

Regarding the use of oFono, I was using more as an example that you
can set socket options before any connection is made, anyway the
setting of the erroneous command seems to be done at startup
regardless if the userspace will actually require BT_PKT_STATUS or
not, so all I'm suggesting is to check if the command was actually
used when using BT_PKT_STATUS.

> >
> > > >
> > > > Also it doesn't seem we have updated userspace to support
> > > > BT_PKT_STATUS, we should probably have something to test it via
> > > > isotest and perhaps create a iso-tester.c to validate all the options.
> > > >
> > > > > Thanks,
> > > > > Alain
> > > > >
> > > > > On Mon, Aug 31, 2020 at 5:44 PM Yu Liu <[e-mail]> wrote:
> > > > >>
> > > > >> +Alain Michaud
> > > > >>
> > > > >> Hi Marcel,
> > > > >>
> > > > >> Can you please comment on the cl as well as Luiz's suggestion? Thanks.
> > > > >>
> > > > >> On Thu, Aug 20, 2020 at 10:20 AM Yu Liu <[e-mail]> wrote:
> > > > >> >
> > > > >> > Friendly ping for comments from Marcel. Thanks.
> > > > >> >
> > > > >> >
> > > > >> > On Mon, Aug 17, 2020 at 4:17 PM Luiz Augusto von Dentz
> > > > >> > <[e-mail]> wrote:
> > > > >> > >
> > > > >> > > Hi Marcel,
> > > > >> > >
> > > > >> > > On Mon, Aug 17, 2020 at 4:07 PM Yu Liu <[e-mail]> wrote:
> > > > >> > > >
> > > > >> > > > From: Archie Pusaka <[e-mail]>
> > > > >> > > >
> > > > >> > > > Initially this is introduced to query whether WBS is supported by the adapter,
> > > > >> > > > the API is generic enough to be extended to support querying others in
> > > > >> > > > the future.
> > > > >> > > >
> > > > >> > > > Reviewed-by: [e-mail]
> > > > >> > > >
> > > > >> > > > Signed-off-by: Yu Liu <[e-mail]>
> > > > >> > > > ---
> > > > >> > > >
> > > > >> > > > Changes in v2:
> > > > >> > > > - Return an array of strings instead of a dict
> > > > >> > > >
> > > > >> > > > Changes in v1:
> > > > >> > > > - Initial change
> > > > >> > > >
> > > > >> > > >  doc/adapter-api.txt | 12 ++++++++++++
> > > > >> > > >  1 file changed, 12 insertions(+)
> > > > >> > > >
> > > > >> > > > diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
> > > > >> > > > index 1a7255750..8fbcadb54 100644
> > > > >> > > > --- a/doc/adapter-api.txt
> > > > >> > > > +++ b/doc/adapter-api.txt
> > > > >> > > > @@ -204,6 +204,18 @@ Methods            void StartDiscovery()
> > > > >> > > >                                          org.bluez.Error.NotReady
> > > > >> > > >                                          org.bluez.Error.Failed
> > > > >> > > >
> > > > >> > > > +               array{string} GetCapabilities()
> > > > >> > > > +
> > > > >> > > > +                       This method returns a list of supported
> > > > >> > > > +                       capabilities that is populated when the adapter
> > > > >> > > > +                       initiated.
> > > > >> > > > +
> > > > >> > > > +                       Possible values:
> > > > >> > > > +                               "wbs" - Wide band speech
> > > > >> > >
> > > > >> > > Btw, should we stick to use wbs terminology here, or we should
> > > > >> > > actually use the HCI feature/command, because wbs has actually to be
> > > > >> > > implemented by the HFP afaik this is only indicating that the
> > > > >> > > controller is able to notify packets drops, etc, with use of erroneous
> > > > >> > > command. Perhaps we should actually use the term PLC (Packet Loss
> > > > >> > > Concealment) instead since that seems to be the real capability here,
> > > > >> > > afaik WBS does not actually require PLC.
> > > > >> > >
> > > > >> > > > +
> > > > >> > > > +                       Possible errors: org.bluez.Error.NotReady
> > > > >> > > > +                                        org.bluez.Error.Failed
> > > > >> > > > +
> > > > >> > > >  Properties     string Address [readonly]
> > > > >> > > >
> > > > >> > > >                         The Bluetooth device address.
> > > > >> > > > --
> > > > >> > > > 2.28.0.220.ged08abb693-goog
> > > > >> > > >
> > > > >> > >
> > > > >> > >
> > > > >> > > --
> > > > >> > > Luiz Augusto von Dentz
> > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz

Patch

diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
index 1a7255750..8fbcadb54 100644
--- a/doc/adapter-api.txt
+++ b/doc/adapter-api.txt
@@ -204,6 +204,18 @@  Methods		void StartDiscovery()
 					 org.bluez.Error.NotReady
 					 org.bluez.Error.Failed
 
+		array{string} GetCapabilities()
+
+			This method returns a list of supported
+			capabilities that is populated when the adapter
+			initiated.
+
+			Possible values:
+				"wbs" - Wide band speech
+
+			Possible errors: org.bluez.Error.NotReady
+					 org.bluez.Error.Failed
+
 Properties	string Address [readonly]
 
 			The Bluetooth device address.