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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ devkit-devel mailing list devkit-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/devkit-devel
