K750 fixes in hidpp-rework branch

[Peter Wu]

Hi Martin,

Thank you for your feedback, a general review of the structure is also helful.

On Monday 02 September 2013 07:00:48 Martin Pitt wrote:
> I'm afraid I can't help much with the actual protocol bits, but some
> general/logic things that caught my eye:
> 
>  - hidpp_device_print_buffer()
>    + This now always prints sizeof (*msg) bytes, which is a constant
>      (i. e. the larger part of the union for a long message). That
>      means for a short message the last 13 bytes will be only garbage.
>      Perhaps the byte dump should be moved into the if TYPE_SHORT/elif
>      TYPE_LONG / elif UNKNOWN right below it?

This code is only executed when running the hidpp-test program, what the 
output looks like should not really matter since most reports are padded 
anyway (and those padding can be ignored). I will fix this for short messages 
though.

>  - hidpp_device_cmd():
>    + Do we need to check for EINTR in the write()? Or are all
>      commands very tiny and thus the chance of EINTRing is negligible?

When trying to debug, I encountered a few times that write/read fails due to 
EINTR, therefore I added this check.

>    + In this part:
> 
>               /* validate response */
>               if (read_msg.type != HIDPP_MSG_TYPE_SHORT &&
>                       read_msg.type != HIDPP_MSG_TYPE_LONG) {
>                       /* ignore key presses, mouse motions, etc. */
>                       continue;
>               }
> 
>       This means we read an unknown message type and thus we probably
>       did a short or too long (if there are more messages queued)
>       read(), i. e. the next read() will be complete garbage as we
>       start reading in the middle of some message. Can it actually
>       happen that the next read() after a command write() is unrelated
>       to that write()? I. e. do messages actually arrive out of band?

The hidraw API guaruantees that message boundaries are preserved. See also the 
implementation of hidpp_discard_messages(), that only reads one byte and let 
the kernel take care of the remainder.

>    + After the "out:" label it unconditionally copies the read message
>      from the local "read_msg" into the output argument "response"
>      anyway. Any reason why it couldn't use "response" right away for
>      the read() call and evaluation, and drop "read_msg"?

Before "hidpp: split request read/write functions", the response pointer could 
be the same as the request. After that commit, I can now directly copy to 
read_msg. I will fix this.

> 
>  - hidpp_device_refresh():
>    + This construction
> 
>                 name = g_string_new ("");
>                 serialp = (guint32 *) &msg.l.params[1];
>                 g_string_printf (name, "%08X", g_ntohl(*serialp));
>                 priv->serial = g_strdup (name->str);
> 
>       looks overly complicated to me. new is freed at the end of the
>       function so it doesn't leak, but this would seem easier:
> 
>                 serialp = (guint32 *) &msg.l.params[1];
>                 priv->serial = g_strdup_printf ("%08X", g_ntohl(*serialp));

Yes, I followed the other example without considering alternatives. 
g_strdup_printf seems sufficient here.

>       Same for the other occurrence, which is even easier (it's just
>       setting it to a single string).

Not exactly the same, the other string does not have to be NUL terminated. 
Anyway, I am now using:

    g_strdup_printf ("%.*s", len, msg.l.params + 2);

> 
>    + Is "if (lux > 200) CHARGING else DISCHARGING" some kind of
>      heuristic, or actually defined somewhere?

It was found by experimentation of Julien Danjou[1].

>  - up_device_unifying_coldplug():
>    + I think it's better to keep the "GUdevDevice *parent_dev"
>      declaration within the if/else blocks, as it's going to be an
>      undefined non-NULL value outside of them (the dev got unreffed
>      already).

Should I name it 'tmp2' instead? No, you are right ;) The scope is limited to 
if which is probably a better place for it. I do not want to add a new commit 
for fixing such a tiny style issue, can I rebase it before merging?

>    + Is there some technical specification that this is a valid
>      assumption?
> 
>      /* give newly paired devices a chance to complete pairing */
>      g_usleep(30000);
> 
>      Or should this rather wait for some signal/uevent to happen?

I looked at the maximum "Default report interval" in the HID++ 1.0 spec (20 
ms) and added a few ms extra. It might not make any sense at all to use that 
value here, but it seems a safe boundary to allow the hidpp-logitech-dj driver 
to complete initialisation.

Regards,
Peter

 [1]: http://lists.freedesktop.org/archives/devkit-devel/2013-August/001460.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/devkit-devel/attachments/20130902/73926d98/attachment.pgp>