diff mbox series

[Bluez,v1,1/2] doc:adding a WidebandSpeechEnabled Api

Message ID 20200403153331.101846-1-alainm@chromium.org (mailing list archive)
State New, archived
Headers show
Series [Bluez,v1,1/2] doc:adding a WidebandSpeechEnabled Api | expand

Commit Message

Alain Michaud April 3, 2020, 3:33 p.m. UTC
This change adds an adapter Api to report the controller's
widebandspeech enabled state.

---

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

Comments

Alain Michaud April 16, 2020, 1:56 p.m. UTC | #1
Friendly ping on this series.


On Fri, Apr 3, 2020 at 11:33 AM Alain Michaud <alainm@chromium.org> wrote:
>
> This change adds an adapter Api to report the controller's
> widebandspeech enabled state.
>
> ---
>
>  doc/adapter-api.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
> index acae032d9..d8865e795 100644
> --- a/doc/adapter-api.txt
> +++ b/doc/adapter-api.txt
> @@ -326,3 +326,11 @@ Properties string Address [readonly]
>
>                         Local Device ID information in modalias format
>                         used by the kernel and udev.
> +
> +               boolean WidebandSpeechEnabled [readonly]
> +
> +                       Returns true if the adapter's wideband speech feature is
> +                       supported and enabled.
> +
> +
> +
> --
> 2.26.0.292.g33ef6b2f38-goog
>
Luiz Augusto von Dentz April 17, 2020, 12:29 a.m. UTC | #2
Hi Alain,

On Thu, Apr 16, 2020 at 1:32 PM Alain Michaud <alainmichaud@google.com> wrote:
>
> Friendly ping on this series.
>
>
> On Fri, Apr 3, 2020 at 11:33 AM Alain Michaud <alainm@chromium.org> wrote:
> >
> > This change adds an adapter Api to report the controller's
> > widebandspeech enabled state.

I wonder if this shouldn't be queried over SCO socket, or simple fail
with BT_VOICE when using BT_VOICE_TRANSPARENT which is how what is
normally used when using mSBC.

> > ---
> >
> >  doc/adapter-api.txt | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
> > index acae032d9..d8865e795 100644
> > --- a/doc/adapter-api.txt
> > +++ b/doc/adapter-api.txt
> > @@ -326,3 +326,11 @@ Properties string Address [readonly]
> >
> >                         Local Device ID information in modalias format
> >                         used by the kernel and udev.
> > +
> > +               boolean WidebandSpeechEnabled [readonly]
> > +
> > +                       Returns true if the adapter's wideband speech feature is
> > +                       supported and enabled.

There seems to be some extra empty lines here.

> > +
> > +
> > +
> > --
> > 2.26.0.292.g33ef6b2f38-goog
> >
Luiz Augusto von Dentz April 17, 2020, 6:16 p.m. UTC | #3
Hi Alain,

On Thu, Apr 16, 2020 at 5:34 PM Alain Michaud <alainmichaud@google.com> wrote:
>
> HI Luiz,
>
> On Thu, Apr 16, 2020 at 8:29 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>
>> Hi Alain,
>>
>> On Thu, Apr 16, 2020 at 1:32 PM Alain Michaud <alainmichaud@google.com> wrote:
>> >
>> > Friendly ping on this series.
>> >
>> >
>> > On Fri, Apr 3, 2020 at 11:33 AM Alain Michaud <alainm@chromium.org> wrote:
>> > >
>> > > This change adds an adapter Api to report the controller's
>> > > widebandspeech enabled state.
>>
>> I wonder if this shouldn't be queried over SCO socket, or simple fail
>> with BT_VOICE when using BT_VOICE_TRANSPARENT which is how what is
>> normally used when using mSBC.
>
> I think there is value in both.

Can you expand on that? I think this might generate confusion if the
property indicates support for it but HFP implementation don't support
it, since the later is usually implemented as a external profile so we
don't have the features it may support, or perhaps the intention here
is to actually indicate when it is in use?

>>
>>
>> > > ---
>> > >
>> > >  doc/adapter-api.txt | 8 ++++++++
>> > >  1 file changed, 8 insertions(+)
>> > >
>> > > diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
>> > > index acae032d9..d8865e795 100644
>> > > --- a/doc/adapter-api.txt
>> > > +++ b/doc/adapter-api.txt
>> > > @@ -326,3 +326,11 @@ Properties string Address [readonly]
>> > >
>> > >                         Local Device ID information in modalias format
>> > >                         used by the kernel and udev.
>> > > +
>> > > +               boolean WidebandSpeechEnabled [readonly]
>> > > +
>> > > +                       Returns true if the adapter's wideband speech feature is
>> > > +                       supported and enabled.
>>
>> There seems to be some extra empty lines here.
>
> ACK, will fix.
>>
>>
>> > > +
>> > > +
>> > > +
>> > > --
>> > > 2.26.0.292.g33ef6b2f38-goog
>> > >
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
Alain Michaud April 17, 2020, 6:21 p.m. UTC | #4
On Fri, Apr 17, 2020 at 2:16 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Alain,
>
> On Thu, Apr 16, 2020 at 5:34 PM Alain Michaud <alainmichaud@google.com> wrote:
> >
> > HI Luiz,
> >
> > On Thu, Apr 16, 2020 at 8:29 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> >>
> >> Hi Alain,
> >>
> >> On Thu, Apr 16, 2020 at 1:32 PM Alain Michaud <alainmichaud@google.com> wrote:
> >> >
> >> > Friendly ping on this series.
> >> >
> >> >
> >> > On Fri, Apr 3, 2020 at 11:33 AM Alain Michaud <alainm@chromium.org> wrote:
> >> > >
> >> > > This change adds an adapter Api to report the controller's
> >> > > widebandspeech enabled state.
> >>
> >> I wonder if this shouldn't be queried over SCO socket, or simple fail
> >> with BT_VOICE when using BT_VOICE_TRANSPARENT which is how what is
> >> normally used when using mSBC.
> >
> > I think there is value in both.
>
> Can you expand on that? I think this might generate confusion if the
> property indicates support for it but HFP implementation don't support
> it, since the later is usually implemented as a external profile so we
> don't have the features it may support, or perhaps the intention here
> is to actually indicate when it is in use?

This is a signal that the adapter supports it and has everything
enabled to support it.   Driver indicated it supports it and erroneous
data reporting was enabled.  The profile has it's own state which may
indicate if msbc will be used, but this will be on a per connection
basis and is independent from this adapter property.

The value in this property is to support diagnostic UX about
controller capabilities/state and also allow profiles that are
implemented outside of bluetoothd to see which codec it can attempt to
negotiate with the device.

>
>
> >>
> >>
> >> > > ---
> >> > >
> >> > >  doc/adapter-api.txt | 8 ++++++++
> >> > >  1 file changed, 8 insertions(+)
> >> > >
> >> > > diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
> >> > > index acae032d9..d8865e795 100644
> >> > > --- a/doc/adapter-api.txt
> >> > > +++ b/doc/adapter-api.txt
> >> > > @@ -326,3 +326,11 @@ Properties string Address [readonly]
> >> > >
> >> > >                         Local Device ID information in modalias format
> >> > >                         used by the kernel and udev.
> >> > > +
> >> > > +               boolean WidebandSpeechEnabled [readonly]
> >> > > +
> >> > > +                       Returns true if the adapter's wideband speech feature is
> >> > > +                       supported and enabled.
> >>
> >> There seems to be some extra empty lines here.
> >
> > ACK, will fix.
> >>
> >>
> >> > > +
> >> > > +
> >> > > +
> >> > > --
> >> > > 2.26.0.292.g33ef6b2f38-goog
> >> > >
> >>
> >>
> >>
> >> --
> >> Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz
Luiz Augusto von Dentz April 17, 2020, 8:58 p.m. UTC | #5
Hi Alain,

On Fri, Apr 17, 2020 at 11:22 AM Alain Michaud <alainmichaud@google.com> wrote:
>
> On Fri, Apr 17, 2020 at 2:16 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Alain,
> >
> > On Thu, Apr 16, 2020 at 5:34 PM Alain Michaud <alainmichaud@google.com> wrote:
> > >
> > > HI Luiz,
> > >
> > > On Thu, Apr 16, 2020 at 8:29 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> > >>
> > >> Hi Alain,
> > >>
> > >> On Thu, Apr 16, 2020 at 1:32 PM Alain Michaud <alainmichaud@google.com> wrote:
> > >> >
> > >> > Friendly ping on this series.
> > >> >
> > >> >
> > >> > On Fri, Apr 3, 2020 at 11:33 AM Alain Michaud <alainm@chromium.org> wrote:
> > >> > >
> > >> > > This change adds an adapter Api to report the controller's
> > >> > > widebandspeech enabled state.
> > >>
> > >> I wonder if this shouldn't be queried over SCO socket, or simple fail
> > >> with BT_VOICE when using BT_VOICE_TRANSPARENT which is how what is
> > >> normally used when using mSBC.
> > >
> > > I think there is value in both.
> >
> > Can you expand on that? I think this might generate confusion if the
> > property indicates support for it but HFP implementation don't support
> > it, since the later is usually implemented as a external profile so we
> > don't have the features it may support, or perhaps the intention here
> > is to actually indicate when it is in use?
>
> This is a signal that the adapter supports it and has everything
> enabled to support it.   Driver indicated it supports it and erroneous
> data reporting was enabled.  The profile has it's own state which may
> indicate if msbc will be used, but this will be on a per connection
> basis and is independent from this adapter property.
>
> The value in this property is to support diagnostic UX about
> controller capabilities/state and also allow profiles that are
> implemented outside of bluetoothd to see which codec it can attempt to
> negotiate with the device.

For diagnosic I think we would be better off with some dedicated
interface to query this, as for the later the information we are
giving does not actually tell anything about the codec support, which
was part of my original argument that for the likes of HFP and other
profiles using it it might not be enough and they still need to use
BT_VOICE in order to enable the use of custom codecs, if you take
ofono for example it does implement support for wideband speech
already and it would completely disregard this property which can give
the false impression that wideband speech cannot be activated when in
fact it can, it just don't have erroneous data reporting enable, so
perhaps we should indicate the actual adapter feature (e.g.
ErrnoneousDataReporting) not the profile feature here, so the profile
implementation can check weather this would disable use of wideband
speech or not, futhermore we should probably report the errors back to
the SCO socket or is that just for diagnostic and cannot be used to
adjust the streaming?

> >
> >
> > >>
> > >>
> > >> > > ---
> > >> > >
> > >> > >  doc/adapter-api.txt | 8 ++++++++
> > >> > >  1 file changed, 8 insertions(+)
> > >> > >
> > >> > > diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
> > >> > > index acae032d9..d8865e795 100644
> > >> > > --- a/doc/adapter-api.txt
> > >> > > +++ b/doc/adapter-api.txt
> > >> > > @@ -326,3 +326,11 @@ Properties string Address [readonly]
> > >> > >
> > >> > >                         Local Device ID information in modalias format
> > >> > >                         used by the kernel and udev.
> > >> > > +
> > >> > > +               boolean WidebandSpeechEnabled [readonly]
> > >> > > +
> > >> > > +                       Returns true if the adapter's wideband speech feature is
> > >> > > +                       supported and enabled.
> > >>
> > >> There seems to be some extra empty lines here.
> > >
> > > ACK, will fix.
> > >>
> > >>
> > >> > > +
> > >> > > +
> > >> > > +
> > >> > > --
> > >> > > 2.26.0.292.g33ef6b2f38-goog
> > >> > >
> > >>
> > >>
> > >>
> > >> --
> > >> Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
Alain Michaud April 17, 2020, 9:03 p.m. UTC | #6
On Fri, Apr 17, 2020 at 4:58 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Alain,
>
> On Fri, Apr 17, 2020 at 11:22 AM Alain Michaud <alainmichaud@google.com> wrote:
> >
> > On Fri, Apr 17, 2020 at 2:16 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Alain,
> > >
> > > On Thu, Apr 16, 2020 at 5:34 PM Alain Michaud <alainmichaud@google.com> wrote:
> > > >
> > > > HI Luiz,
> > > >
> > > > On Thu, Apr 16, 2020 at 8:29 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> > > >>
> > > >> Hi Alain,
> > > >>
> > > >> On Thu, Apr 16, 2020 at 1:32 PM Alain Michaud <alainmichaud@google.com> wrote:
> > > >> >
> > > >> > Friendly ping on this series.
> > > >> >
> > > >> >
> > > >> > On Fri, Apr 3, 2020 at 11:33 AM Alain Michaud <alainm@chromium.org> wrote:
> > > >> > >
> > > >> > > This change adds an adapter Api to report the controller's
> > > >> > > widebandspeech enabled state.
> > > >>
> > > >> I wonder if this shouldn't be queried over SCO socket, or simple fail
> > > >> with BT_VOICE when using BT_VOICE_TRANSPARENT which is how what is
> > > >> normally used when using mSBC.
> > > >
> > > > I think there is value in both.
> > >
> > > Can you expand on that? I think this might generate confusion if the
> > > property indicates support for it but HFP implementation don't support
> > > it, since the later is usually implemented as a external profile so we
> > > don't have the features it may support, or perhaps the intention here
> > > is to actually indicate when it is in use?
> >
> > This is a signal that the adapter supports it and has everything
> > enabled to support it.   Driver indicated it supports it and erroneous
> > data reporting was enabled.  The profile has it's own state which may
> > indicate if msbc will be used, but this will be on a per connection
> > basis and is independent from this adapter property.
> >
> > The value in this property is to support diagnostic UX about
> > controller capabilities/state and also allow profiles that are
> > implemented outside of bluetoothd to see which codec it can attempt to
> > negotiate with the device.
>
> For diagnosic I think we would be better off with some dedicated
> interface to query this, as for the later the information we are
> giving does not actually tell anything about the codec support, which
> was part of my original argument that for the likes of HFP and other
> profiles using it it might not be enough and they still need to use
> BT_VOICE in order to enable the use of custom codecs, if you take
> ofono for example it does implement support for wideband speech
> already and it would completely disregard this property which can give
> the false impression that wideband speech cannot be activated when in
> fact it can, it just don't have erroneous data reporting enable, so
> perhaps we should indicate the actual adapter feature (e.g.
> ErrnoneousDataReporting) not the profile feature here, so the profile
> implementation can check weather this would disable use of wideband
> speech or not, futhermore we should probably report the errors back to
> the SCO socket or is that just for diagnostic and cannot be used to
> adjust the streaming?

My original patch actually had this MGMT feature called erroneous data
reporting and Marcel argued against it.  If you both agree, then I'm
happy to rename all of this to erroneous data reporting.  We'd still
need some way for the driver support to be messaged some other way
though. see:

if (id->driver_info & BTUSB_WIDEBAND_SPEECH)
        set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, &hdev->quirks);


> > >
> > >
> > > >>
> > > >>
> > > >> > > ---
> > > >> > >
> > > >> > >  doc/adapter-api.txt | 8 ++++++++
> > > >> > >  1 file changed, 8 insertions(+)
> > > >> > >
> > > >> > > diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
> > > >> > > index acae032d9..d8865e795 100644
> > > >> > > --- a/doc/adapter-api.txt
> > > >> > > +++ b/doc/adapter-api.txt
> > > >> > > @@ -326,3 +326,11 @@ Properties string Address [readonly]
> > > >> > >
> > > >> > >                         Local Device ID information in modalias format
> > > >> > >                         used by the kernel and udev.
> > > >> > > +
> > > >> > > +               boolean WidebandSpeechEnabled [readonly]
> > > >> > > +
> > > >> > > +                       Returns true if the adapter's wideband speech feature is
> > > >> > > +                       supported and enabled.
> > > >>
> > > >> There seems to be some extra empty lines here.
> > > >
> > > > ACK, will fix.
> > > >>
> > > >>
> > > >> > > +
> > > >> > > +
> > > >> > > +
> > > >> > > --
> > > >> > > 2.26.0.292.g33ef6b2f38-goog
> > > >> > >
> > > >>
> > > >>
> > > >>
> > > >> --
> > > >> Luiz Augusto von Dentz
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz
Alain Michaud May 5, 2020, 3:47 p.m. UTC | #7
Friendly ping on this series.


On Tue, May 5, 2020 at 11:46 AM Alain Michaud <alainmichaud@google.com> wrote:
>
> Friendly ping on this series.
>
> On Fri, Apr 17, 2020 at 5:03 PM Alain Michaud <alainmichaud@google.com> wrote:
>>
>> On Fri, Apr 17, 2020 at 4:58 PM Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>> >
>> > Hi Alain,
>> >
>> > On Fri, Apr 17, 2020 at 11:22 AM Alain Michaud <alainmichaud@google.com> wrote:
>> > >
>> > > On Fri, Apr 17, 2020 at 2:16 PM Luiz Augusto von Dentz
>> > > <luiz.dentz@gmail.com> wrote:
>> > > >
>> > > > Hi Alain,
>> > > >
>> > > > On Thu, Apr 16, 2020 at 5:34 PM Alain Michaud <alainmichaud@google.com> wrote:
>> > > > >
>> > > > > HI Luiz,
>> > > > >
>> > > > > On Thu, Apr 16, 2020 at 8:29 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>> > > > >>
>> > > > >> Hi Alain,
>> > > > >>
>> > > > >> On Thu, Apr 16, 2020 at 1:32 PM Alain Michaud <alainmichaud@google.com> wrote:
>> > > > >> >
>> > > > >> > Friendly ping on this series.
>> > > > >> >
>> > > > >> >
>> > > > >> > On Fri, Apr 3, 2020 at 11:33 AM Alain Michaud <alainm@chromium.org> wrote:
>> > > > >> > >
>> > > > >> > > This change adds an adapter Api to report the controller's
>> > > > >> > > widebandspeech enabled state.
>> > > > >>
>> > > > >> I wonder if this shouldn't be queried over SCO socket, or simple fail
>> > > > >> with BT_VOICE when using BT_VOICE_TRANSPARENT which is how what is
>> > > > >> normally used when using mSBC.
>> > > > >
>> > > > > I think there is value in both.
>> > > >
>> > > > Can you expand on that? I think this might generate confusion if the
>> > > > property indicates support for it but HFP implementation don't support
>> > > > it, since the later is usually implemented as a external profile so we
>> > > > don't have the features it may support, or perhaps the intention here
>> > > > is to actually indicate when it is in use?
>> > >
>> > > This is a signal that the adapter supports it and has everything
>> > > enabled to support it.   Driver indicated it supports it and erroneous
>> > > data reporting was enabled.  The profile has it's own state which may
>> > > indicate if msbc will be used, but this will be on a per connection
>> > > basis and is independent from this adapter property.
>> > >
>> > > The value in this property is to support diagnostic UX about
>> > > controller capabilities/state and also allow profiles that are
>> > > implemented outside of bluetoothd to see which codec it can attempt to
>> > > negotiate with the device.
>> >
>> > For diagnosic I think we would be better off with some dedicated
>> > interface to query this, as for the later the information we are
>> > giving does not actually tell anything about the codec support, which
>> > was part of my original argument that for the likes of HFP and other
>> > profiles using it it might not be enough and they still need to use
>> > BT_VOICE in order to enable the use of custom codecs, if you take
>> > ofono for example it does implement support for wideband speech
>> > already and it would completely disregard this property which can give
>> > the false impression that wideband speech cannot be activated when in
>> > fact it can, it just don't have erroneous data reporting enable, so
>> > perhaps we should indicate the actual adapter feature (e.g.
>> > ErrnoneousDataReporting) not the profile feature here, so the profile
>> > implementation can check weather this would disable use of wideband
>> > speech or not, futhermore we should probably report the errors back to
>> > the SCO socket or is that just for diagnostic and cannot be used to
>> > adjust the streaming?
>>
>> My original patch actually had this MGMT feature called erroneous data
>> reporting and Marcel argued against it.  If you both agree, then I'm
>> happy to rename all of this to erroneous data reporting.  We'd still
>> need some way for the driver support to be messaged some other way
>> though. see:
>>
>> if (id->driver_info & BTUSB_WIDEBAND_SPEECH)
>>         set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, &hdev->quirks);
>>
>>
>> > > >
>> > > >
>> > > > >>
>> > > > >>
>> > > > >> > > ---
>> > > > >> > >
>> > > > >> > >  doc/adapter-api.txt | 8 ++++++++
>> > > > >> > >  1 file changed, 8 insertions(+)
>> > > > >> > >
>> > > > >> > > diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
>> > > > >> > > index acae032d9..d8865e795 100644
>> > > > >> > > --- a/doc/adapter-api.txt
>> > > > >> > > +++ b/doc/adapter-api.txt
>> > > > >> > > @@ -326,3 +326,11 @@ Properties string Address [readonly]
>> > > > >> > >
>> > > > >> > >                         Local Device ID information in modalias format
>> > > > >> > >                         used by the kernel and udev.
>> > > > >> > > +
>> > > > >> > > +               boolean WidebandSpeechEnabled [readonly]
>> > > > >> > > +
>> > > > >> > > +                       Returns true if the adapter's wideband speech feature is
>> > > > >> > > +                       supported and enabled.
>> > > > >>
>> > > > >> There seems to be some extra empty lines here.
>> > > > >
>> > > > > ACK, will fix.
>> > > > >>
>> > > > >>
>> > > > >> > > +
>> > > > >> > > +
>> > > > >> > > +
>> > > > >> > > --
>> > > > >> > > 2.26.0.292.g33ef6b2f38-goog
>> > > > >> > >
>> > > > >>
>> > > > >>
>> > > > >>
>> > > > >> --
>> > > > >> Luiz Augusto von Dentz
>> > > >
>> > > >
>> > > >
>> > > > --
>> > > > Luiz Augusto von Dentz
>> >
>> >
>> >
>> > --
>> > Luiz Augusto von Dentz
Luiz Augusto von Dentz May 5, 2020, 11:48 p.m. UTC | #8
Hi Marcel and Alain,

On Tue, May 5, 2020 at 8:47 AM Alain Michaud <alainmichaud@google.com> wrote:
>
> Friendly ping on this series.
>
>
> On Tue, May 5, 2020 at 11:46 AM Alain Michaud <alainmichaud@google.com> wrote:
> >
> > Friendly ping on this series.
> >
> > On Fri, Apr 17, 2020 at 5:03 PM Alain Michaud <alainmichaud@google.com> wrote:
> >>
> >> On Fri, Apr 17, 2020 at 4:58 PM Luiz Augusto von Dentz
> >> <luiz.dentz@gmail.com> wrote:
> >> >
> >> > Hi Alain,
> >> >
> >> > On Fri, Apr 17, 2020 at 11:22 AM Alain Michaud <alainmichaud@google.com> wrote:
> >> > >
> >> > > On Fri, Apr 17, 2020 at 2:16 PM Luiz Augusto von Dentz
> >> > > <luiz.dentz@gmail.com> wrote:
> >> > > >
> >> > > > Hi Alain,
> >> > > >
> >> > > > On Thu, Apr 16, 2020 at 5:34 PM Alain Michaud <alainmichaud@google.com> wrote:
> >> > > > >
> >> > > > > HI Luiz,
> >> > > > >
> >> > > > > On Thu, Apr 16, 2020 at 8:29 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> >> > > > >>
> >> > > > >> Hi Alain,
> >> > > > >>
> >> > > > >> On Thu, Apr 16, 2020 at 1:32 PM Alain Michaud <alainmichaud@google.com> wrote:
> >> > > > >> >
> >> > > > >> > Friendly ping on this series.
> >> > > > >> >
> >> > > > >> >
> >> > > > >> > On Fri, Apr 3, 2020 at 11:33 AM Alain Michaud <alainm@chromium.org> wrote:
> >> > > > >> > >
> >> > > > >> > > This change adds an adapter Api to report the controller's
> >> > > > >> > > widebandspeech enabled state.
> >> > > > >>
> >> > > > >> I wonder if this shouldn't be queried over SCO socket, or simple fail
> >> > > > >> with BT_VOICE when using BT_VOICE_TRANSPARENT which is how what is
> >> > > > >> normally used when using mSBC.
> >> > > > >
> >> > > > > I think there is value in both.
> >> > > >
> >> > > > Can you expand on that? I think this might generate confusion if the
> >> > > > property indicates support for it but HFP implementation don't support
> >> > > > it, since the later is usually implemented as a external profile so we
> >> > > > don't have the features it may support, or perhaps the intention here
> >> > > > is to actually indicate when it is in use?
> >> > >
> >> > > This is a signal that the adapter supports it and has everything
> >> > > enabled to support it.   Driver indicated it supports it and erroneous
> >> > > data reporting was enabled.  The profile has it's own state which may
> >> > > indicate if msbc will be used, but this will be on a per connection
> >> > > basis and is independent from this adapter property.
> >> > >
> >> > > The value in this property is to support diagnostic UX about
> >> > > controller capabilities/state and also allow profiles that are
> >> > > implemented outside of bluetoothd to see which codec it can attempt to
> >> > > negotiate with the device.
> >> >
> >> > For diagnosic I think we would be better off with some dedicated
> >> > interface to query this, as for the later the information we are
> >> > giving does not actually tell anything about the codec support, which
> >> > was part of my original argument that for the likes of HFP and other
> >> > profiles using it it might not be enough and they still need to use
> >> > BT_VOICE in order to enable the use of custom codecs, if you take
> >> > ofono for example it does implement support for wideband speech
> >> > already and it would completely disregard this property which can give
> >> > the false impression that wideband speech cannot be activated when in
> >> > fact it can, it just don't have erroneous data reporting enable, so
> >> > perhaps we should indicate the actual adapter feature (e.g.
> >> > ErrnoneousDataReporting) not the profile feature here, so the profile
> >> > implementation can check weather this would disable use of wideband
> >> > speech or not, futhermore we should probably report the errors back to
> >> > the SCO socket or is that just for diagnostic and cannot be used to
> >> > adjust the streaming?
> >>
> >> My original patch actually had this MGMT feature called erroneous data
> >> reporting and Marcel argued against it.  If you both agree, then I'm
> >> happy to rename all of this to erroneous data reporting.  We'd still
> >> need some way for the driver support to be messaged some other way
> >> though. see:
> >>
> >> if (id->driver_info & BTUSB_WIDEBAND_SPEECH)
> >>         set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, &hdev->quirks);

So this will get a little bit confusing if we don't have a HFP
implementation that actually does implement the profile parts to
enable wideband speech so I wonder if we should rename it to the
underline feature it enables, that said this sort of feature is
probably easier to be queried over the socket itself rather than over
D-Bus so it can be used in conjunction with the likes of BT_VOICE,
also regarding the erroneous data should that be also enabled by the
HFP profile, because depending on the platform it may not support
wideband speech so enabling erroneous data automatically may result in
artifacs, specially in cases where the codecs don't have error
correction or data loss concealment to handle data with possible error
and lost data respectively, btw we don't seem to be parsing the
SCO/ESCO flags and even if we do this properly we need to find a way
to notify them over the socket so things like plc works properly.

> >>
> >>
> >> > > >
> >> > > >
> >> > > > >>
> >> > > > >>
> >> > > > >> > > ---
> >> > > > >> > >
> >> > > > >> > >  doc/adapter-api.txt | 8 ++++++++
> >> > > > >> > >  1 file changed, 8 insertions(+)
> >> > > > >> > >
> >> > > > >> > > diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
> >> > > > >> > > index acae032d9..d8865e795 100644
> >> > > > >> > > --- a/doc/adapter-api.txt
> >> > > > >> > > +++ b/doc/adapter-api.txt
> >> > > > >> > > @@ -326,3 +326,11 @@ Properties string Address [readonly]
> >> > > > >> > >
> >> > > > >> > >                         Local Device ID information in modalias format
> >> > > > >> > >                         used by the kernel and udev.
> >> > > > >> > > +
> >> > > > >> > > +               boolean WidebandSpeechEnabled [readonly]
> >> > > > >> > > +
> >> > > > >> > > +                       Returns true if the adapter's wideband speech feature is
> >> > > > >> > > +                       supported and enabled.
> >> > > > >>
> >> > > > >> There seems to be some extra empty lines here.
> >> > > > >
> >> > > > > ACK, will fix.
> >> > > > >>
> >> > > > >>
> >> > > > >> > > +
> >> > > > >> > > +
> >> > > > >> > > +
> >> > > > >> > > --
> >> > > > >> > > 2.26.0.292.g33ef6b2f38-goog
> >> > > > >> > >
> >> > > > >>
> >> > > > >>
> >> > > > >>
> >> > > > >> --
> >> > > > >> Luiz Augusto von Dentz
> >> > > >
> >> > > >
> >> > > >
> >> > > > --
> >> > > > Luiz Augusto von Dentz
> >> >
> >> >
> >> >
> >> > --
> >> > Luiz Augusto von Dentz
Alain Michaud May 6, 2020, 2:10 p.m. UTC | #9
Hi Luiz,


On Tue, May 5, 2020 at 7:48 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Marcel and Alain,
>
> On Tue, May 5, 2020 at 8:47 AM Alain Michaud <alainmichaud@google.com> wrote:
> >
> > Friendly ping on this series.
> >
> >
> > On Tue, May 5, 2020 at 11:46 AM Alain Michaud <alainmichaud@google.com> wrote:
> > >
> > > Friendly ping on this series.
> > >
> > > On Fri, Apr 17, 2020 at 5:03 PM Alain Michaud <alainmichaud@google.com> wrote:
> > >>
> > >> On Fri, Apr 17, 2020 at 4:58 PM Luiz Augusto von Dentz
> > >> <luiz.dentz@gmail.com> wrote:
> > >> >
> > >> > Hi Alain,
> > >> >
> > >> > On Fri, Apr 17, 2020 at 11:22 AM Alain Michaud <alainmichaud@google.com> wrote:
> > >> > >
> > >> > > On Fri, Apr 17, 2020 at 2:16 PM Luiz Augusto von Dentz
> > >> > > <luiz.dentz@gmail.com> wrote:
> > >> > > >
> > >> > > > Hi Alain,
> > >> > > >
> > >> > > > On Thu, Apr 16, 2020 at 5:34 PM Alain Michaud <alainmichaud@google.com> wrote:
> > >> > > > >
> > >> > > > > HI Luiz,
> > >> > > > >
> > >> > > > > On Thu, Apr 16, 2020 at 8:29 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> > >> > > > >>
> > >> > > > >> Hi Alain,
> > >> > > > >>
> > >> > > > >> On Thu, Apr 16, 2020 at 1:32 PM Alain Michaud <alainmichaud@google.com> wrote:
> > >> > > > >> >
> > >> > > > >> > Friendly ping on this series.
> > >> > > > >> >
> > >> > > > >> >
> > >> > > > >> > On Fri, Apr 3, 2020 at 11:33 AM Alain Michaud <alainm@chromium.org> wrote:
> > >> > > > >> > >
> > >> > > > >> > > This change adds an adapter Api to report the controller's
> > >> > > > >> > > widebandspeech enabled state.
> > >> > > > >>
> > >> > > > >> I wonder if this shouldn't be queried over SCO socket, or simple fail
> > >> > > > >> with BT_VOICE when using BT_VOICE_TRANSPARENT which is how what is
> > >> > > > >> normally used when using mSBC.
> > >> > > > >
> > >> > > > > I think there is value in both.
> > >> > > >
> > >> > > > Can you expand on that? I think this might generate confusion if the
> > >> > > > property indicates support for it but HFP implementation don't support
> > >> > > > it, since the later is usually implemented as a external profile so we
> > >> > > > don't have the features it may support, or perhaps the intention here
> > >> > > > is to actually indicate when it is in use?
> > >> > >
> > >> > > This is a signal that the adapter supports it and has everything
> > >> > > enabled to support it.   Driver indicated it supports it and erroneous
> > >> > > data reporting was enabled.  The profile has it's own state which may
> > >> > > indicate if msbc will be used, but this will be on a per connection
> > >> > > basis and is independent from this adapter property.
> > >> > >
> > >> > > The value in this property is to support diagnostic UX about
> > >> > > controller capabilities/state and also allow profiles that are
> > >> > > implemented outside of bluetoothd to see which codec it can attempt to
> > >> > > negotiate with the device.
> > >> >
> > >> > For diagnosic I think we would be better off with some dedicated
> > >> > interface to query this, as for the later the information we are
> > >> > giving does not actually tell anything about the codec support, which
> > >> > was part of my original argument that for the likes of HFP and other
> > >> > profiles using it it might not be enough and they still need to use
> > >> > BT_VOICE in order to enable the use of custom codecs, if you take
> > >> > ofono for example it does implement support for wideband speech
> > >> > already and it would completely disregard this property which can give
> > >> > the false impression that wideband speech cannot be activated when in
> > >> > fact it can, it just don't have erroneous data reporting enable, so
> > >> > perhaps we should indicate the actual adapter feature (e.g.
> > >> > ErrnoneousDataReporting) not the profile feature here, so the profile
> > >> > implementation can check weather this would disable use of wideband
> > >> > speech or not, futhermore we should probably report the errors back to
> > >> > the SCO socket or is that just for diagnostic and cannot be used to
> > >> > adjust the streaming?
> > >>
> > >> My original patch actually had this MGMT feature called erroneous data
> > >> reporting and Marcel argued against it.  If you both agree, then I'm
> > >> happy to rename all of this to erroneous data reporting.  We'd still
> > >> need some way for the driver support to be messaged some other way
> > >> though. see:
> > >>
> > >> if (id->driver_info & BTUSB_WIDEBAND_SPEECH)
> > >>         set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, &hdev->quirks);
>
> So this will get a little bit confusing if we don't have a HFP
> implementation that actually does implement the profile parts to
> enable wideband speech so I wonder if we should rename it to the
> underline feature it enables, that said this sort of feature is
> probably easier to be queried over the socket itself rather than over
> D-Bus so it can be used in conjunction with the likes of BT_VOICE,
> also regarding the erroneous data should that be also enabled by the
> HFP profile, because depending on the platform it may not support
> wideband speech so enabling erroneous data automatically may result in
> artifacs, specially in cases where the codecs don't have error
> correction or data loss concealment to handle data with possible error
> and lost data respectively, btw we don't seem to be parsing the
> SCO/ESCO flags and even if we do this properly we need to find a way
> to notify them over the socket so things like plc works properly.

This indeed makes sense, but we'll need to decide how erroneous data
reporting gets enabled.  The SCO socket is a per connection scope
thing while erroneous data reporting is a controller level thing.


>
> > >>
> > >>
> > >> > > >
> > >> > > >
> > >> > > > >>
> > >> > > > >>
> > >> > > > >> > > ---
> > >> > > > >> > >
> > >> > > > >> > >  doc/adapter-api.txt | 8 ++++++++
> > >> > > > >> > >  1 file changed, 8 insertions(+)
> > >> > > > >> > >
> > >> > > > >> > > diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
> > >> > > > >> > > index acae032d9..d8865e795 100644
> > >> > > > >> > > --- a/doc/adapter-api.txt
> > >> > > > >> > > +++ b/doc/adapter-api.txt
> > >> > > > >> > > @@ -326,3 +326,11 @@ Properties string Address [readonly]
> > >> > > > >> > >
> > >> > > > >> > >                         Local Device ID information in modalias format
> > >> > > > >> > >                         used by the kernel and udev.
> > >> > > > >> > > +
> > >> > > > >> > > +               boolean WidebandSpeechEnabled [readonly]
> > >> > > > >> > > +
> > >> > > > >> > > +                       Returns true if the adapter's wideband speech feature is
> > >> > > > >> > > +                       supported and enabled.
> > >> > > > >>
> > >> > > > >> There seems to be some extra empty lines here.
> > >> > > > >
> > >> > > > > ACK, will fix.
> > >> > > > >>
> > >> > > > >>
> > >> > > > >> > > +
> > >> > > > >> > > +
> > >> > > > >> > > +
> > >> > > > >> > > --
> > >> > > > >> > > 2.26.0.292.g33ef6b2f38-goog
> > >> > > > >> > >
> > >> > > > >>
> > >> > > > >>
> > >> > > > >>
> > >> > > > >> --
> > >> > > > >> Luiz Augusto von Dentz
> > >> > > >
> > >> > > >
> > >> > > >
> > >> > > > --
> > >> > > > Luiz Augusto von Dentz
> > >> >
> > >> >
> > >> >
> > >> > --
> > >> > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz
Luiz Augusto von Dentz May 6, 2020, 4:24 p.m. UTC | #10
Hi Alain,

On Wed, May 6, 2020 at 7:10 AM Alain Michaud <alainmichaud@google.com> wrote:
>
> Hi Luiz,
>
>
> On Tue, May 5, 2020 at 7:48 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Marcel and Alain,
> >
> > On Tue, May 5, 2020 at 8:47 AM Alain Michaud <alainmichaud@google.com> wrote:
> > >
> > > Friendly ping on this series.
> > >
> > >
> > > On Tue, May 5, 2020 at 11:46 AM Alain Michaud <alainmichaud@google.com> wrote:
> > > >
> > > > Friendly ping on this series.
> > > >
> > > > On Fri, Apr 17, 2020 at 5:03 PM Alain Michaud <alainmichaud@google.com> wrote:
> > > >>
> > > >> On Fri, Apr 17, 2020 at 4:58 PM Luiz Augusto von Dentz
> > > >> <luiz.dentz@gmail.com> wrote:
> > > >> >
> > > >> > Hi Alain,
> > > >> >
> > > >> > On Fri, Apr 17, 2020 at 11:22 AM Alain Michaud <alainmichaud@google.com> wrote:
> > > >> > >
> > > >> > > On Fri, Apr 17, 2020 at 2:16 PM Luiz Augusto von Dentz
> > > >> > > <luiz.dentz@gmail.com> wrote:
> > > >> > > >
> > > >> > > > Hi Alain,
> > > >> > > >
> > > >> > > > On Thu, Apr 16, 2020 at 5:34 PM Alain Michaud <alainmichaud@google.com> wrote:
> > > >> > > > >
> > > >> > > > > HI Luiz,
> > > >> > > > >
> > > >> > > > > On Thu, Apr 16, 2020 at 8:29 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> > > >> > > > >>
> > > >> > > > >> Hi Alain,
> > > >> > > > >>
> > > >> > > > >> On Thu, Apr 16, 2020 at 1:32 PM Alain Michaud <alainmichaud@google.com> wrote:
> > > >> > > > >> >
> > > >> > > > >> > Friendly ping on this series.
> > > >> > > > >> >
> > > >> > > > >> >
> > > >> > > > >> > On Fri, Apr 3, 2020 at 11:33 AM Alain Michaud <alainm@chromium.org> wrote:
> > > >> > > > >> > >
> > > >> > > > >> > > This change adds an adapter Api to report the controller's
> > > >> > > > >> > > widebandspeech enabled state.
> > > >> > > > >>
> > > >> > > > >> I wonder if this shouldn't be queried over SCO socket, or simple fail
> > > >> > > > >> with BT_VOICE when using BT_VOICE_TRANSPARENT which is how what is
> > > >> > > > >> normally used when using mSBC.
> > > >> > > > >
> > > >> > > > > I think there is value in both.
> > > >> > > >
> > > >> > > > Can you expand on that? I think this might generate confusion if the
> > > >> > > > property indicates support for it but HFP implementation don't support
> > > >> > > > it, since the later is usually implemented as a external profile so we
> > > >> > > > don't have the features it may support, or perhaps the intention here
> > > >> > > > is to actually indicate when it is in use?
> > > >> > >
> > > >> > > This is a signal that the adapter supports it and has everything
> > > >> > > enabled to support it.   Driver indicated it supports it and erroneous
> > > >> > > data reporting was enabled.  The profile has it's own state which may
> > > >> > > indicate if msbc will be used, but this will be on a per connection
> > > >> > > basis and is independent from this adapter property.
> > > >> > >
> > > >> > > The value in this property is to support diagnostic UX about
> > > >> > > controller capabilities/state and also allow profiles that are
> > > >> > > implemented outside of bluetoothd to see which codec it can attempt to
> > > >> > > negotiate with the device.
> > > >> >
> > > >> > For diagnosic I think we would be better off with some dedicated
> > > >> > interface to query this, as for the later the information we are
> > > >> > giving does not actually tell anything about the codec support, which
> > > >> > was part of my original argument that for the likes of HFP and other
> > > >> > profiles using it it might not be enough and they still need to use
> > > >> > BT_VOICE in order to enable the use of custom codecs, if you take
> > > >> > ofono for example it does implement support for wideband speech
> > > >> > already and it would completely disregard this property which can give
> > > >> > the false impression that wideband speech cannot be activated when in
> > > >> > fact it can, it just don't have erroneous data reporting enable, so
> > > >> > perhaps we should indicate the actual adapter feature (e.g.
> > > >> > ErrnoneousDataReporting) not the profile feature here, so the profile
> > > >> > implementation can check weather this would disable use of wideband
> > > >> > speech or not, futhermore we should probably report the errors back to
> > > >> > the SCO socket or is that just for diagnostic and cannot be used to
> > > >> > adjust the streaming?
> > > >>
> > > >> My original patch actually had this MGMT feature called erroneous data
> > > >> reporting and Marcel argued against it.  If you both agree, then I'm
> > > >> happy to rename all of this to erroneous data reporting.  We'd still
> > > >> need some way for the driver support to be messaged some other way
> > > >> though. see:
> > > >>
> > > >> if (id->driver_info & BTUSB_WIDEBAND_SPEECH)
> > > >>         set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, &hdev->quirks);
> >
> > So this will get a little bit confusing if we don't have a HFP
> > implementation that actually does implement the profile parts to
> > enable wideband speech so I wonder if we should rename it to the
> > underline feature it enables, that said this sort of feature is
> > probably easier to be queried over the socket itself rather than over
> > D-Bus so it can be used in conjunction with the likes of BT_VOICE,
> > also regarding the erroneous data should that be also enabled by the
> > HFP profile, because depending on the platform it may not support
> > wideband speech so enabling erroneous data automatically may result in
> > artifacs, specially in cases where the codecs don't have error
> > correction or data loss concealment to handle data with possible error
> > and lost data respectively, btw we don't seem to be parsing the
> > SCO/ESCO flags and even if we do this properly we need to find a way
> > to notify them over the socket so things like plc works properly.
>
> This indeed makes sense, but we'll need to decide how erroneous data
> reporting gets enabled.  The SCO socket is a per connection scope
> thing while erroneous data reporting is a controller level thing.

The way I would have done this is to make use of recvmsg and then add
the packet flags as msg_flags:

   recvmsg()
       The recvmsg() call uses a msghdr structure to minimize the
number of directly supplied arguments.  This structure is defined as
follows in <sys/socket.h>:

           struct iovec {                    /* Scatter/gather array items */
               void  *iov_base;              /* Starting address */
               size_t iov_len;               /* Number of bytes to transfer */
           };

           struct msghdr {
               void         *msg_name;       /* Optional address */
               socklen_t     msg_namelen;    /* Size of address */
               struct iovec *msg_iov;        /* Scatter/gather array */
               size_t        msg_iovlen;     /* # elements in msg_iov */
               void         *msg_control;    /* Ancillary data, see below */
               size_t        msg_controllen; /* Ancillary data buffer len */
               int           msg_flags;      /* Flags on received message */
           };

But to make this backward compatible I would recommend adding a
socketopt that enables this new behavior and in case there is more
than one SCO socket those that did not use the sockopt should probably
drop packets with error or lost data so they work as without erroneous
data report since in that case the application will likely be using
regular reads it won't be able to detect the use of msg_flags.

>
>
> >
> > > >>
> > > >>
> > > >> > > >
> > > >> > > >
> > > >> > > > >>
> > > >> > > > >>
> > > >> > > > >> > > ---
> > > >> > > > >> > >
> > > >> > > > >> > >  doc/adapter-api.txt | 8 ++++++++
> > > >> > > > >> > >  1 file changed, 8 insertions(+)
> > > >> > > > >> > >
> > > >> > > > >> > > diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
> > > >> > > > >> > > index acae032d9..d8865e795 100644
> > > >> > > > >> > > --- a/doc/adapter-api.txt
> > > >> > > > >> > > +++ b/doc/adapter-api.txt
> > > >> > > > >> > > @@ -326,3 +326,11 @@ Properties string Address [readonly]
> > > >> > > > >> > >
> > > >> > > > >> > >                         Local Device ID information in modalias format
> > > >> > > > >> > >                         used by the kernel and udev.
> > > >> > > > >> > > +
> > > >> > > > >> > > +               boolean WidebandSpeechEnabled [readonly]
> > > >> > > > >> > > +
> > > >> > > > >> > > +                       Returns true if the adapter's wideband speech feature is
> > > >> > > > >> > > +                       supported and enabled.
> > > >> > > > >>
> > > >> > > > >> There seems to be some extra empty lines here.
> > > >> > > > >
> > > >> > > > > ACK, will fix.
> > > >> > > > >>
> > > >> > > > >>
> > > >> > > > >> > > +
> > > >> > > > >> > > +
> > > >> > > > >> > > +
> > > >> > > > >> > > --
> > > >> > > > >> > > 2.26.0.292.g33ef6b2f38-goog
> > > >> > > > >> > >
> > > >> > > > >>
> > > >> > > > >>
> > > >> > > > >>
> > > >> > > > >> --
> > > >> > > > >> Luiz Augusto von Dentz
> > > >> > > >
> > > >> > > >
> > > >> > > >
> > > >> > > > --
> > > >> > > > Luiz Augusto von Dentz
> > > >> >
> > > >> >
> > > >> >
> > > >> > --
> > > >> > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
Marcel Holtmann May 6, 2020, 4:43 p.m. UTC | #11
Hi Luiz,

>>>>>>>>>>>>> This change adds an adapter Api to report the controller's
>>>>>>>>>>>>> widebandspeech enabled state.
>>>>>>>>>>> 
>>>>>>>>>>> I wonder if this shouldn't be queried over SCO socket, or simple fail
>>>>>>>>>>> with BT_VOICE when using BT_VOICE_TRANSPARENT which is how what is
>>>>>>>>>>> normally used when using mSBC.
>>>>>>>>>> 
>>>>>>>>>> I think there is value in both.
>>>>>>>>> 
>>>>>>>>> Can you expand on that? I think this might generate confusion if the
>>>>>>>>> property indicates support for it but HFP implementation don't support
>>>>>>>>> it, since the later is usually implemented as a external profile so we
>>>>>>>>> don't have the features it may support, or perhaps the intention here
>>>>>>>>> is to actually indicate when it is in use?
>>>>>>>> 
>>>>>>>> This is a signal that the adapter supports it and has everything
>>>>>>>> enabled to support it.   Driver indicated it supports it and erroneous
>>>>>>>> data reporting was enabled.  The profile has it's own state which may
>>>>>>>> indicate if msbc will be used, but this will be on a per connection
>>>>>>>> basis and is independent from this adapter property.
>>>>>>>> 
>>>>>>>> The value in this property is to support diagnostic UX about
>>>>>>>> controller capabilities/state and also allow profiles that are
>>>>>>>> implemented outside of bluetoothd to see which codec it can attempt to
>>>>>>>> negotiate with the device.
>>>>>>> 
>>>>>>> For diagnosic I think we would be better off with some dedicated
>>>>>>> interface to query this, as for the later the information we are
>>>>>>> giving does not actually tell anything about the codec support, which
>>>>>>> was part of my original argument that for the likes of HFP and other
>>>>>>> profiles using it it might not be enough and they still need to use
>>>>>>> BT_VOICE in order to enable the use of custom codecs, if you take
>>>>>>> ofono for example it does implement support for wideband speech
>>>>>>> already and it would completely disregard this property which can give
>>>>>>> the false impression that wideband speech cannot be activated when in
>>>>>>> fact it can, it just don't have erroneous data reporting enable, so
>>>>>>> perhaps we should indicate the actual adapter feature (e.g.
>>>>>>> ErrnoneousDataReporting) not the profile feature here, so the profile
>>>>>>> implementation can check weather this would disable use of wideband
>>>>>>> speech or not, futhermore we should probably report the errors back to
>>>>>>> the SCO socket or is that just for diagnostic and cannot be used to
>>>>>>> adjust the streaming?
>>>>>> 
>>>>>> My original patch actually had this MGMT feature called erroneous data
>>>>>> reporting and Marcel argued against it.  If you both agree, then I'm
>>>>>> happy to rename all of this to erroneous data reporting.  We'd still
>>>>>> need some way for the driver support to be messaged some other way
>>>>>> though. see:
>>>>>> 
>>>>>> if (id->driver_info & BTUSB_WIDEBAND_SPEECH)
>>>>>>        set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, &hdev->quirks);
>>> 
>>> So this will get a little bit confusing if we don't have a HFP
>>> implementation that actually does implement the profile parts to
>>> enable wideband speech so I wonder if we should rename it to the
>>> underline feature it enables, that said this sort of feature is
>>> probably easier to be queried over the socket itself rather than over
>>> D-Bus so it can be used in conjunction with the likes of BT_VOICE,
>>> also regarding the erroneous data should that be also enabled by the
>>> HFP profile, because depending on the platform it may not support
>>> wideband speech so enabling erroneous data automatically may result in
>>> artifacs, specially in cases where the codecs don't have error
>>> correction or data loss concealment to handle data with possible error
>>> and lost data respectively, btw we don't seem to be parsing the
>>> SCO/ESCO flags and even if we do this properly we need to find a way
>>> to notify them over the socket so things like plc works properly.
>> 
>> This indeed makes sense, but we'll need to decide how erroneous data
>> reporting gets enabled.  The SCO socket is a per connection scope
>> thing while erroneous data reporting is a controller level thing.
> 
> The way I would have done this is to make use of recvmsg and then add
> the packet flags as msg_flags:
> 
>   recvmsg()
>       The recvmsg() call uses a msghdr structure to minimize the
> number of directly supplied arguments.  This structure is defined as
> follows in <sys/socket.h>:
> 
>           struct iovec {                    /* Scatter/gather array items */
>               void  *iov_base;              /* Starting address */
>               size_t iov_len;               /* Number of bytes to transfer */
>           };
> 
>           struct msghdr {
>               void         *msg_name;       /* Optional address */
>               socklen_t     msg_namelen;    /* Size of address */
>               struct iovec *msg_iov;        /* Scatter/gather array */
>               size_t        msg_iovlen;     /* # elements in msg_iov */
>               void         *msg_control;    /* Ancillary data, see below */
>               size_t        msg_controllen; /* Ancillary data buffer len */
>               int           msg_flags;      /* Flags on received message */
>           };
> 
> But to make this backward compatible I would recommend adding a
> socketopt that enables this new behavior and in case there is more
> than one SCO socket those that did not use the sockopt should probably
> drop packets with error or lost data so they work as without erroneous
> data report since in that case the application will likely be using
> regular reads it won't be able to detect the use of msg_flags.

I think that I already send an example on how to do that a while ago. Don’t remember if that was a private email or addressed to the mailing list, but I remember showing how this can be done. Or I confused this with something similar :(

Regards

Marcel
Alain Michaud May 6, 2020, 4:53 p.m. UTC | #12
Hi Marcel,

Indeed, we do have the packet_msg patch to carry the erroneous data
flag.  We will send results once validated.

However, the profile implementation will still need to know if
erroneous data reporting is enabled/supported and that the USB driver
is correctly using the alt setting on the USB isoch endpoint. As a
result, we still need to carry some form of notification that all of
these things are supported from kernel below.  Again, this is a
controller attribute, not a connection attribute so it may best be
served outside of the sco socket.

Thanks,
Alain

On Wed, May 6, 2020 at 12:43 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> >>>>>>>>>>>>> This change adds an adapter Api to report the controller's
> >>>>>>>>>>>>> widebandspeech enabled state.
> >>>>>>>>>>>
> >>>>>>>>>>> I wonder if this shouldn't be queried over SCO socket, or simple fail
> >>>>>>>>>>> with BT_VOICE when using BT_VOICE_TRANSPARENT which is how what is
> >>>>>>>>>>> normally used when using mSBC.
> >>>>>>>>>>
> >>>>>>>>>> I think there is value in both.
> >>>>>>>>>
> >>>>>>>>> Can you expand on that? I think this might generate confusion if the
> >>>>>>>>> property indicates support for it but HFP implementation don't support
> >>>>>>>>> it, since the later is usually implemented as a external profile so we
> >>>>>>>>> don't have the features it may support, or perhaps the intention here
> >>>>>>>>> is to actually indicate when it is in use?
> >>>>>>>>
> >>>>>>>> This is a signal that the adapter supports it and has everything
> >>>>>>>> enabled to support it.   Driver indicated it supports it and erroneous
> >>>>>>>> data reporting was enabled.  The profile has it's own state which may
> >>>>>>>> indicate if msbc will be used, but this will be on a per connection
> >>>>>>>> basis and is independent from this adapter property.
> >>>>>>>>
> >>>>>>>> The value in this property is to support diagnostic UX about
> >>>>>>>> controller capabilities/state and also allow profiles that are
> >>>>>>>> implemented outside of bluetoothd to see which codec it can attempt to
> >>>>>>>> negotiate with the device.
> >>>>>>>
> >>>>>>> For diagnosic I think we would be better off with some dedicated
> >>>>>>> interface to query this, as for the later the information we are
> >>>>>>> giving does not actually tell anything about the codec support, which
> >>>>>>> was part of my original argument that for the likes of HFP and other
> >>>>>>> profiles using it it might not be enough and they still need to use
> >>>>>>> BT_VOICE in order to enable the use of custom codecs, if you take
> >>>>>>> ofono for example it does implement support for wideband speech
> >>>>>>> already and it would completely disregard this property which can give
> >>>>>>> the false impression that wideband speech cannot be activated when in
> >>>>>>> fact it can, it just don't have erroneous data reporting enable, so
> >>>>>>> perhaps we should indicate the actual adapter feature (e.g.
> >>>>>>> ErrnoneousDataReporting) not the profile feature here, so the profile
> >>>>>>> implementation can check weather this would disable use of wideband
> >>>>>>> speech or not, futhermore we should probably report the errors back to
> >>>>>>> the SCO socket or is that just for diagnostic and cannot be used to
> >>>>>>> adjust the streaming?
> >>>>>>
> >>>>>> My original patch actually had this MGMT feature called erroneous data
> >>>>>> reporting and Marcel argued against it.  If you both agree, then I'm
> >>>>>> happy to rename all of this to erroneous data reporting.  We'd still
> >>>>>> need some way for the driver support to be messaged some other way
> >>>>>> though. see:
> >>>>>>
> >>>>>> if (id->driver_info & BTUSB_WIDEBAND_SPEECH)
> >>>>>>        set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, &hdev->quirks);
> >>>
> >>> So this will get a little bit confusing if we don't have a HFP
> >>> implementation that actually does implement the profile parts to
> >>> enable wideband speech so I wonder if we should rename it to the
> >>> underline feature it enables, that said this sort of feature is
> >>> probably easier to be queried over the socket itself rather than over
> >>> D-Bus so it can be used in conjunction with the likes of BT_VOICE,
> >>> also regarding the erroneous data should that be also enabled by the
> >>> HFP profile, because depending on the platform it may not support
> >>> wideband speech so enabling erroneous data automatically may result in
> >>> artifacs, specially in cases where the codecs don't have error
> >>> correction or data loss concealment to handle data with possible error
> >>> and lost data respectively, btw we don't seem to be parsing the
> >>> SCO/ESCO flags and even if we do this properly we need to find a way
> >>> to notify them over the socket so things like plc works properly.
> >>
> >> This indeed makes sense, but we'll need to decide how erroneous data
> >> reporting gets enabled.  The SCO socket is a per connection scope
> >> thing while erroneous data reporting is a controller level thing.
> >
> > The way I would have done this is to make use of recvmsg and then add
> > the packet flags as msg_flags:
> >
> >   recvmsg()
> >       The recvmsg() call uses a msghdr structure to minimize the
> > number of directly supplied arguments.  This structure is defined as
> > follows in <sys/socket.h>:
> >
> >           struct iovec {                    /* Scatter/gather array items */
> >               void  *iov_base;              /* Starting address */
> >               size_t iov_len;               /* Number of bytes to transfer */
> >           };
> >
> >           struct msghdr {
> >               void         *msg_name;       /* Optional address */
> >               socklen_t     msg_namelen;    /* Size of address */
> >               struct iovec *msg_iov;        /* Scatter/gather array */
> >               size_t        msg_iovlen;     /* # elements in msg_iov */
> >               void         *msg_control;    /* Ancillary data, see below */
> >               size_t        msg_controllen; /* Ancillary data buffer len */
> >               int           msg_flags;      /* Flags on received message */
> >           };
> >
> > But to make this backward compatible I would recommend adding a
> > socketopt that enables this new behavior and in case there is more
> > than one SCO socket those that did not use the sockopt should probably
> > drop packets with error or lost data so they work as without erroneous
> > data report since in that case the application will likely be using
> > regular reads it won't be able to detect the use of msg_flags.
>
> I think that I already send an example on how to do that a while ago. Don’t remember if that was a private email or addressed to the mailing list, but I remember showing how this can be done. Or I confused this with something similar :(
>
> Regards
>
> Marcel
>
diff mbox series

Patch

diff --git a/doc/adapter-api.txt b/doc/adapter-api.txt
index acae032d9..d8865e795 100644
--- a/doc/adapter-api.txt
+++ b/doc/adapter-api.txt
@@ -326,3 +326,11 @@  Properties	string Address [readonly]
 
 			Local Device ID information in modalias format
 			used by the kernel and udev.
+
+		boolean WidebandSpeechEnabled [readonly]
+
+			Returns true if the adapter's wideband speech feature is
+			supported and enabled.
+
+
+