From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <cyrus@holtmann.org>
MIME-Version: 1.0
In-Reply-To: <1269678511.22795.22.camel@localhost.localdomain>
References: <2c3916b71003160533s714e9ef3h6c803f88a79f27d2@mail.gmail.com>
	 <2a9506371003221436s1cf1b465r907f726797b4f251@mail.gmail.com>
	 <1269678511.22795.22.camel@localhost.localdomain>
Date: Sat, 27 Mar 2010 06:20:56 -0300
Message-ID: <6b53b1991003270220s3034f541s7b0c645dbb0ed79b@mail.gmail.com>
Subject: Re: [PATCH] Fix display of last device classes in hciconfig
From: "Gustavo F. Padovan" <gustavo@padovan.org>
To: Daniel Abraham <daniel.shrugged@gmail.com>
Cc: Vinicius Gomes <vinicius.gomes@openbossa.org>,
	linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset=ISO-8859-1
Sender: linux-bluetooth-owner@vger.kernel.org
List-ID: <linux-bluetooth.vger.kernel.org>

Hi Daniel,

On Sat, Mar 27, 2010 at 5:28 AM, Daniel Abraham
<daniel.shrugged@gmail.com> wrote:
>> > ---
>> >  tools/hciconfig.c |   30 ++++++++++++++++++++++++++----
>> >  1 files changed, 26 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/tools/hciconfig.c b/tools/hciconfig.c
>> > index 3f687e0..c97ce44 100644
>> > --- a/tools/hciconfig.c
>> > +++ b/tools/hciconfig.c
>> > @@ -645,8 +645,26 @@ static char *get_minor_device_name(int major, int minor)
>> >                        return "Game";
>> >                }
>> >                break;
>> > -       case 63:        /* uncategorised */
>> > -               return "";
>>
>> I think that the test for the "Uncategorised" (which seems to be
>> misspelled in many places) device class would be better staying here,
>> because the same code is used inside hcitool.c. Fixing there would be
>> nice too.
>>
>> Another thing, looks like this "case 63:" is wrong,  major device
>> class is a 5 bit number, and the uncategorized device class is defined
>> as 31. Looks like a legacy from ancient times ;-)
>
> That's exactly why I changed it: the original check below whether to
> enter this function ("if ((cls[1] & 0x1f) >= ...") meant <no. of
> categories + 1> where +1 meant "Unrecognized", but this is mistaken as
> you wrote.
>
> I don't mind fixing it in "hcitool.c" as well, but see followup question
> at the bottom.
>
>> > +       case 9: /* health */
>> > +               switch(minor) {
>> > +               case 0:
>> > +                       return "Undefined";
>> > +               case 1:
>> > +                       return "Blood Pressure Monitor";
>> > +               case 2:
>> > +                       return "Thermometer";
>> > +               case 3:
>> > +                       return "Weighing Scale";
>> > +               case 4:
>> > +                       return "Glucose Meter";
>> > +               case 5:
>> > +                       return "Pulse Oximeter";
>> > +               case 6:
>> > +                       return "Heart/Pulse Rate Monitor";
>> > +               case 7:
>> > +                       return "Health Data Display";
>> > +               }
>> > +               break;
>> >        }
>> >        return "Unknown (reserved) minor device class";
>> >  }
>> > @@ -668,7 +686,9 @@ static void cmd_class(int ctl, int hdev, char *opt)
>> >                                        "Audio/Video",
>> >                                        "Peripheral",
>> >                                        "Imaging",
>> > -                                       "Uncategorized" };
>> > +                                       "Wearable",
>> > +                                       "Toy",
>> > +                                       "Health" };
>> >        int s = hci_open_dev(hdev);
>> >
>> >        if (s < 0) {
>> > @@ -706,7 +726,9 @@ static void cmd_class(int ctl, int hdev, char *opt)
>> >                } else
>> >                        printf("Unspecified");
>> >                printf("\n\tDevice Class: ");
>> > -               if ((cls[1] & 0x1f) >= sizeof(major_devices) / sizeof(*major_devices))
>> > +               if (0x1f == cls[1])
>>
>> This test is inverted, in BlueZ code we use the form: (variable
>> operator constant)
>>
>> > +                       printf("Uncategorized\n");
>> > +               else if ((cls[1] & 0x1f) >= sizeof(major_devices) / sizeof(*major_devices))
>>
>> See above.
>
> Will fix both.
>
>> >                        printf("Invalid Device Class!\n");
>> >                else
>> >                        printf("%s, %s\n", major_devices[cls[1] & 0x1f],
>> > --
>> > 1.6.6.1
>> > --
>> >
>>
>>
>> Cheers,
>
> Thanks for the comments! I'll send a fixed patch.
>
> But here's a followup question: what's the convention for submitting a
> change to a patch - another patch on top of it, or a full replacement
> patch? As a reply to this thread, or as new message?

Send a full replacement inline patch as a reply to this thread.

>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Gustavo F. Padovan
http://padovan.org