Hi Ravi,
On Fri, Nov 08, 2013, Ravi Kumar Veeramally wrote:
>>> +void ascii2hex(const uint8_t *ascii, int ascii_len, uint8_t *hex)
>>> +{
>>> + int i;
>>> +
>>> + if (!ascii || !hex)
>>> + return;
>>> +
>>> + for (i = 0; i < ascii_len / 2; i++)
>>> + sscanf((char *) &ascii[i * 2], "%02x",
>>> + (unsigned int *) &hex[i]);
>>> +
>>> +}
>
>> I'd just keep this static inside the HID HAL since that seems to be
>> the only user for it. Actually I'm not even convinced that it's worth
>> to have this as a separate function since you only have two users and
>> essentially the function is just two lines (the if-statement is
>> redundant).
>
> Ok, I will keep it as static. Do you want me to keep in HID HAL or HID
> daemon?
The daemon is where you were doing the conversion now as well, so no
need to change that (sorry about being unclear when I said "HID HAL").
>> Also, the function name isn't quite right. You're converting from hex
>> to binary
>
> We are converting to hex right? correct me If I am wrong.
No, we're converting from hex to binary.
>>. Not from ascii to hex (hex notation is already using
>>the ascii character set). Also, do consider the point from Jerzy about
>>the format specifier.
>>
> Ok, I will fix Jerzy's suggestion also.
>
> Apart from this patch, do you have any comments on rest of the
> patches?
Only the ones I already sent.
Johan
On 11/08/2013 10:48 AM, Johan Hedberg wrote:
> Hi Ravi,
>
> On Fri, Nov 08, 2013, Ravi Kumar Veeramally wrote:
>>>> +void ascii2hex(const uint8_t *ascii, int ascii_len, uint8_t *hex)
>>>> +{
>>>> + int i;
>>>> +
>>>> + if (!ascii || !hex)
>>>> + return;
>>>> +
>>>> + for (i = 0; i < ascii_len / 2; i++)
>>>> + sscanf((char *) &ascii[i * 2], "%02x",
>>>> + (unsigned int *) &hex[i]);
>>>> +
>>>> +}
>>> I'd just keep this static inside the HID HAL since that seems to be
>>> the only user for it. Actually I'm not even convinced that it's worth
>>> to have this as a separate function since you only have two users and
>>> essentially the function is just two lines (the if-statement is
>>> redundant).
>> Ok, I will keep it as static. Do you want me to keep in HID HAL or HID
>> daemon?
> The daemon is where you were doing the conversion now as well, so no
> need to change that (sorry about being unclear when I said "HID HAL").
>
>>> Also, the function name isn't quite right. You're converting from hex
>>> to binary
>> We are converting to hex right? correct me If I am wrong.
> No, we're converting from hex to binary.
>
>>> . Not from ascii to hex (hex notation is already using
>>> the ascii character set). Also, do consider the point from Jerzy about
>>> the format specifier.
>>>
>> Ok, I will fix Jerzy's suggestion also.
>>
>> Apart from this patch, do you have any comments on rest of the
>> patches?
> Only the ones I already sent.
>
> Johan
>
Ok, Patch_v2 6 & 7 are independent of ascii stuff, can be it possible to
apply if there are no comments? (just a request!!).
Thanks a lot,
Ravi.