Re: [PATCH 1/3] src/profile: Distinguish between zero-set HFP AG features and unset HFP AG features

[Luiz Augusto von Dentz <luiz.dentz@gmail.com>]

Hi Pali,

On Mon, Apr 13, 2020 at 9:52 AM Pali Rohár <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ár <pali@kernel.org> wrote:
> > > @@ -1750,15 +1758,19 @@ static int ext_disconnect_dev(struct btd_service *service)
> > >  static char *get_hfp_hf_record(struct ext_profile *ext, struct ext_io *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->version,
> > > -                                               ext->name, ext->features);
> > > +                               ext->name,
> > > +                               ext->have_features ? ext->features : 0x0);
> > >  }
> > >
> > >  static char *get_hfp_ag_record(struct ext_profile *ext, struct ext_io *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->version,
> > > -                                               ext->name, ext->features);
> > > +                               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?



-- 
Luiz Augusto von Dentz