K750 fixes in hidpp-rework branch

[Martin Pitt]

Hello Peter,

Peter Wu [2013-08-28 23:56 +0200]:
> For some reason I am at 20 commits now at the hidpp-rework branch. That's a 
> lot of changes. Can somebody review and merge once this issue has been sorted 
> out?

First, thanks a lot for doing this work! With these patches and the
structs and constants it is much easier to decipher the code than the
previous bit shifting/offset/numeric magic.

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?

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

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

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


 - 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));

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


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

 - 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).

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


Otherwise this looks good to me, thanks!

Martin
-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.freedesktop.org/archives/devkit-devel/attachments/20130902/960f28fa/attachment.pgp>