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>
In-Reply-To: <20200413165206.xluhkwi76cfpiv7z@pali>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 13 Apr 2020 09:58:34 -0700
Message-ID: <CABBYNZJYj=i4+SemOAST18Jn20Nn03C_NyfB+aBLc5L9Yg11_A@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 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> wrote=
:
> > > @@ -1750,15 +1758,19 @@ static int ext_disconnect_dev(struct btd_serv=
ice *service)
> > >  static char *get_hfp_hf_record(struct ext_profile *ext, struct ext_i=
o *l2cap,
> > >                                                         struct ext_io=
 *rfcomm)
> > >  {
> > > +       /* HFP 1.7.2: By default features bitfield is 0b000000 */
> > >         return g_strdup_printf(HFP_HF_RECORD, rfcomm->chan, ext->vers=
ion,
> > > -                                               ext->name, ext->featu=
res);
> > > +                               ext->name,
> > > +                               ext->have_features ? ext->features : =
0x0);
> > >  }
> > >
> > >  static char *get_hfp_ag_record(struct ext_profile *ext, struct ext_i=
o *l2cap,
> > >                                                         struct ext_io=
 *rfcomm)
> > >  {
> > > +       /* HFP 1.7.2: By default features bitfield is 0b001001 */
> > >         return g_strdup_printf(HFP_AG_RECORD, rfcomm->chan, ext->vers=
ion,
> > > -                                               ext->name, ext->featu=
res);
> > > +                               ext->name,
> > > +                               ext->have_features ? ext->features : =
0x9);
> >
> > I wonder why you didn't just initialize the features wiht 0x9 instead
> > 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 have
> unified code for storing local and remote features it is easier to have
> 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.

> 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?


--=20
Luiz Augusto von Dentz