Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200321195404.fvyku5hmcuqxt7sg@pali> <20200413162513.2221-1-pali@kernel.org>
 <20200413162513.2221-2-pali@kernel.org> <CABBYNZKneybjWDEddrCXxtzJdGZSY__D7qtPuw25QMLDE9hg4Q@mail.gmail.com>
 <20200413165206.xluhkwi76cfpiv7z@pali> <CABBYNZJYj=i4+SemOAST18Jn20Nn03C_NyfB+aBLc5L9Yg11_A@mail.gmail.com>
 <20200413171722.ilawhisu67onrifn@pali>
In-Reply-To: <20200413171722.ilawhisu67onrifn@pali>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 13 Apr 2020 10:47:04 -0700
Message-ID: <CABBYNZKFGjsnCnBPyw0WFr_aJ2zaYAjLj81t04S=fXaXD9ac+g@mail.gmail.com>
Subject: Re: [PATCH 1/3] src/profile: Distinguish between zero-set HFP AG
 features and unset HFP AG features
To:     =?UTF-8?Q?Pali_Roh=C3=A1r?= <pali@kernel.org>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        David Heidelberg <david@ixit.cz>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Pali,

On Mon, Apr 13, 2020 at 10:17 AM Pali Roh=C3=A1r <pali@kernel.org> wrote:
>
> On Monday 13 April 2020 09:58:34 Luiz Augusto von Dentz wrote:
> > Hi Pali,
> >
> > On Mon, Apr 13, 2020 at 9:52 AM Pali Roh=C3=A1r <pali@kernel.org> wrote=
:
> > >
> > > On Monday 13 April 2020 09:44:14 Luiz Augusto von Dentz wrote:
> > > > On Mon, Apr 13, 2020 at 9:25 AM Pali Roh=C3=A1r <pali@kernel.org> w=
rote:
> > > > > @@ -1750,15 +1758,19 @@ static int ext_disconnect_dev(struct btd_=
service *service)
> > > > >  static char *get_hfp_hf_record(struct ext_profile *ext, struct e=
xt_io *l2cap,
> > > > >                                                         struct ex=
t_io *rfcomm)
> > > > >  {
> > > > > +       /* HFP 1.7.2: By default features bitfield is 0b000000 */
> > > > >         return g_strdup_printf(HFP_HF_RECORD, rfcomm->chan, ext->=
version,
> > > > > -                                               ext->name, ext->f=
eatures);
> > > > > +                               ext->name,
> > > > > +                               ext->have_features ? ext->feature=
s : 0x0);
> > > > >  }
> > > > >
> > > > >  static char *get_hfp_ag_record(struct ext_profile *ext, struct e=
xt_io *l2cap,
> > > > >                                                         struct ex=
t_io *rfcomm)
> > > > >  {
> > > > > +       /* HFP 1.7.2: By default features bitfield is 0b001001 */
> > > > >         return g_strdup_printf(HFP_AG_RECORD, rfcomm->chan, ext->=
version,
> > > > > -                                               ext->name, ext->f=
eatures);
> > > > > +                               ext->name,
> > > > > +                               ext->have_features ? ext->feature=
s : 0x9);
> > > >
> > > > I wonder why you didn't just initialize the features wiht 0x9 inste=
ad
> > > > of adding a flag to track it, btw add a define with value 0x09 like
> > > > HFP_DEFAULT_FEATURES that way it is clearer why we are doing this.
> > >
> > > This function get_hfp_ag_record() is for parsing local features. You =
are
> > > right that for local features we do not need a flag to track it.
> > >
> > > But flag for tracking is needed for parsing remote features. And to h=
ave
> > > unified code for storing local and remote features it is easier to ha=
ve
> > > always a flag for checking if features were provided or not.
> >
> > Im not following you about the remote features beinf different, I
> > though both would be could be initialized in the same way and then if
> > we read a different value from the SDP record we just overwrite it.
>
> But in this case we put these default remote features to HFP DBus agent
> like if they were specified in SDP. Default value is specific for
> profile version. And if e.g. new HFP version defines a new bit in
> features with its own default value then HFP DBus agent would not be
> able to distinguish between state when remote device did not specified
> any value (as bluez will put there some defaults) and when remote device
> specified same features as bluez has defined in its current default
> value.

Different version may have different defaults but we can initialize
them correctly.

> Before and also after this change remote features are not send to HFP
> DBus agent when they were not specified by remote device.
>
> After your suggestion HFP DBus agent would not be able to check if
> remote device provided features or not. And this is I think a
> regression.

I don't think we can consider a regression to always provide features,
besides what would be the reason for the agent to know if the features
were from the SDP record or they are auto initiliazed? I that was
really necessary Id just dump the whole record as a ServiceRecord
option and leave it up to the client to parse it.

> > > I can put these default values in profile-role specific macros e.g.:
> > >
> > > #define HFP_AG_DEFAULT_FEATURES 0x09
> > > #define HFP_HF_DEFAULT_FEATURES 0x00
> > > #define HSP_HS_DEFAULT_VOLCNTRL "false"
> >
> > Don't bother with default that are 0x00/false, we can assume this is
> > implicit when allocating the memory everything is set to 0 so these
> > defines wouldn't be needed in the first place.
> >
> > > Or do you prefer different names?
> >
> >
> >
> > --
> > Luiz Augusto von Dentz


--=20
Luiz Augusto von Dentz