Hi Petri,
On Wed, Mar 12, 2014, Petri Gynther wrote:
> Enable HID protocol handling in userspace when uHIDP=true in input.conf.
> ---
> Makefile.plugins | 3 +-
> profiles/input/device.c | 804 +++++++++++++++++++++++++++++++++++++++++----
> profiles/input/device.h | 1 +
> profiles/input/hidp_defs.h | 77 +++++
> profiles/input/input.conf | 4 +
> profiles/input/manager.c | 22 +-
> 6 files changed, 837 insertions(+), 74 deletions(-)
> create mode 100644 profiles/input/hidp_defs.h
For such a large patch you'll really need a more thorough explanation of
the background. Also, if possible consider if there's any way the patch
could be split into smaller pieces.
Particularly, I'm curious why this feature is desirable over HIDP in the
kernel. Aren't you essentially introducing something that would cause
user space (bluetoothd) to be scheduled for every single packet, unlike
the kernel solution which lets user space stay idle most of the time?
> @@ -81,16 +85,30 @@ struct input_device {
> enum reconnect_mode_t reconnect_mode;
> guint reconnect_timer;
> uint32_t reconnect_attempt;
> + gboolean uhidp_enabled;
> + int uhid_fd;
> + guint uhid_watch;
> + gboolean uhid_created;
> + unsigned char report_req_pending;
To be consistent with the code base, please use uint8_t for unsigned
8-bit integers.
In our efforts to eventually get rid of the GLib dependency, it'd be
nice if you could try to use bool instead of gboolean whenever you're
not directly interfacing with a GLib API (which expects gboolean).
> +static gboolean hidp_send_message(GIOChannel *chan, const unsigned char hdr,
The extra const declaration for hdr seems unnecessary here (it's also
not consistent with the rest of the code base. Also, use uint8_t please.
> + msg[0] = hdr;
> + if (size > 0)
> + memcpy(&msg[1], data, size);
> +
> + msgptr = msg;
> + ++size;
> +
> + fd = g_io_channel_unix_get_fd(chan);
> +
> + while (size > 0) {
> + len = write(fd, msgptr, size);
> +
> + if (len < 0) {
> + error("BT socket write failed: %s (%d)",
> + strerror(errno), errno);
> + return FALSE;
> + }
> +
> + if (len == 0) {
> + error("BT socket write failed: wrote 0 bytes");
> + return FALSE;
> + }
> +
> + msgptr += len;
> + size -= len;
> + }
Since this is a SEQPACKET L2CAP socket I don't think you can get this
kind of partial writes, and hence the loop is unnecessary. Also, did
you considered using a scatter/gather vector to avoid this extra buffer
and memcpy in user space, i.e. use sendmsg() and keep hdr and data
separate?
> +static inline gboolean hidp_send_ctrl_message(struct input_device *idev,
> + const unsigned char hdr, const unsigned char *data, unsigned int size)
> +{
> + return hidp_send_message(idev->ctrl_io, hdr, data, size);
> +}
> +
> +static inline gboolean hidp_send_intr_message(struct input_device *idev,
> + const unsigned char hdr, const unsigned char *data, unsigned int size)
> +{
> + return hidp_send_message(idev->intr_io, hdr, data, size);
> +}
Again, no need for const with the non-pointer variables, and please use
uint8_t. I'd also consider renaming this to hidp_ctrl_send/hidp_intr_send
to make it easier to follow the coding style (you should indent the
split lines past the '(' of the first line. Also drop the inline
declaration as the compiler will optimize this anyway. I'd also consider
using size_t as a more appropriate type for the size, or alternatively
something that maps better to the limitations of the protocol such as
uint8_t or uint16_t.
> +static gboolean uhid_send_feature_answer(struct input_device *idev,
> + const unsigned char *data, unsigned int size,
> + const unsigned int id, const unsigned int err)
Same thing here with const and uint8_t.
> + len = write(idev->uhid_fd, &ev, sizeof(ev));
> +
> + if (len < 0) {
You can remove the empty line above to be consistent with the coding
style.
> +static gboolean uhid_send_input_report(struct input_device *idev,
> + const unsigned char *data, unsigned int size)
Same thing with the size variable type as mentioned earlier.
> + len = write(idev->uhid_fd, &ev, sizeof(ev));
> +
> + if (len < 0) {
You can remove the empty line above.
> +static gboolean hidp_recv_intr_data(GIOChannel *chan,
> + struct input_device *idev)
> +{
> + int fd;
> + ssize_t len;
> + unsigned char hdr;
uint8_t hdr;
> + unsigned char data[UHID_DATA_MAX + 1];
uint8_t data[...];
> +
> + fd = g_io_channel_unix_get_fd(chan);
> + len = read(fd, data, sizeof(data));
> +
> + if (len < 0) {
Remove the empty line.
> static gboolean intr_watch_cb(GIOChannel *chan, GIOCondition cond, gpointer data)
> {
> struct input_device *idev = data;
> char address[18];
>
> + if ((cond & G_IO_IN) && hidp_recv_intr_data(chan, idev) &&
> + (cond == G_IO_IN)) {
> + return TRUE;
> + }
This is cramming a bit too many things into the same if-statement.
Please split it up, e.g. something like:
if ((cond & G_IO_IN) && !hidp_recv_intr_data(chan, idev))
goto failed;
/* No exceptions just incoming data */
if (cond == G_IO_IN)
return TRUE;
> +static void hidp_recv_ctrl_handshake(struct input_device *idev,
> + unsigned char param)
uint8_t param
> + unsigned char pending_req_type;
uint8_t
> +static void hidp_recv_ctrl_hid_control(struct input_device *idev,
> + unsigned char param)
> +{
uint8_t param
> + DBG("");
> +
> + if (param == HIDP_CTRL_VIRTUAL_CABLE_UNPLUG)
> + connection_disconnect(idev, 0);
> +}
> +
> +static void hidp_recv_ctrl_data(struct input_device *idev, unsigned char param,
> + unsigned char *data, unsigned int len)
Considering how much you made use of const in the previous functions I'm
surprised you're not using it here. I suppose data should at least have
it? Also, again, please use size_t or something else more appropriate
than unsigned int for the len variable.
> +{
> + unsigned char pending_req_type;
> + unsigned char pending_req_param;
uint8_t
> + pending_req_type = idev->report_req_pending & HIDP_HEADER_TRANS_MASK;
> + pending_req_param = idev->report_req_pending & HIDP_HEADER_PARAM_MASK;
> +
> + if (pending_req_type != HIDP_TRANS_GET_REPORT) {
> + DBG("Spurious DATA on control channel");
> + return;
> + }
> +
> + if (pending_req_param != param) {
> + DBG("Received DATA RTYPE doesn't match pending request RTYPE");
> + return;
> + }
This might look cleaner (and more consistent with coding style) if you
do the assignments right before checking the resulting value:
pending_req_type = ...
if (panding_req_type != ...) {
...
}
pending_req_param = ...
if (pending_req_param != ...) {
...
}
> +static gboolean hidp_recv_ctrl_message(GIOChannel *chan,
> + struct input_device *idev)
> +{
> + int fd;
> + ssize_t len;
> + unsigned char hdr, type, param;
> + unsigned char data[UHID_DATA_MAX + 1];
uint8_t
> +
> + fd = g_io_channel_unix_get_fd(chan);
> + len = read(fd, data, sizeof(data));
> +
> + if (len < 0) {
You can remove the empty line above.
> static gboolean ctrl_watch_cb(GIOChannel *chan, GIOCondition cond, gpointer data)
> {
> struct input_device *idev = data;
> char address[18];
>
> + if ((cond & G_IO_IN) && hidp_recv_ctrl_message(chan, idev) &&
> + (cond == G_IO_IN)) {
> + return TRUE;
> + }
Same thing as earlier, please split this up a bit.
> +static gboolean hidp_report_req_timeout(gpointer data)
> +{
> + struct input_device *idev = data;
> + unsigned char pending_req_type;
> + char *req_type_str;
Should be const char * considering what you're later assigning to it.
> +static void hidp_send_set_report(struct input_device *idev,
> + struct uhid_event *ev)
> +{
> + unsigned char hdr;
uint8_t
> + gboolean sent;
bool
> +static void hidp_send_get_report(struct input_device *idev,
> + struct uhid_event *ev)
> +{
> + unsigned char hdr;
> + gboolean sent;
Same here.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
