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)
signature.asc
Description: Digital signature
_______________________________________________ devkit-devel mailing list devkit-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/devkit-devel
