[PATCH_v3 01/04] android/hid: Implement hid get protocol in daemon

[PATCH_v3 01/04] android/hid: Implement hid get protocol in daemon

[Ravi kumar Veeramally]

This patch requests hid device protocol mode and reads reply
message and sends notification to hal.
---
v3: Changed hid_msg to last_hid_msg and variable type uint8_t type
---
 android/hid.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 104 insertions(+), 4 deletions(-)

diff --git a/android/hid.c b/android/hid.c
index c0c9aeb..97835d9 100644
--- a/android/hid.c
+++ b/android/hid.c
@@ -54,6 +54,14 @@
 #define L2CAP_PSM_HIDP_INTR	0x13
 #define UHID_DEVICE_FILE	"/dev/uhid"
 
+/* HID message types */
+#define HID_MSG_GET_PROTOCOL	0x60
+#define HID_MSG_DATA		0xa0
+
+/* HID protocol header parameters */
+#define HID_PROTO_BOOT		0x00
+#define HID_PROTO_REPORT	0x01
+
 static GIOChannel *notification_io = NULL;
 static GIOChannel *ctrl_io = NULL;
 static GIOChannel *intr_io = NULL;
@@ -76,6 +84,7 @@ struct hid_device {
 	guint		intr_watch;
 	int		uhid_fd;
 	guint		uhid_watch_id;
+	uint8_t		last_hid_msg;
 };
 
 static int device_cmp(gconstpointer s, gconstpointer user_data)
@@ -243,12 +252,74 @@ static gboolean intr_watch_cb(GIOChannel *chan, GIOCondition cond,
 	return FALSE;
 }
 
+static void bt_hid_notify_proto_mode(struct hid_device *dev, uint8_t *buf,
+									int len)
+{
+	struct hal_ev_hid_proto_mode ev;
+	char address[18];
+
+	ba2str(&dev->dst, address);
+	DBG("device %s", address);
+
+	memset(&ev, 0, sizeof(ev));
+	bdaddr2android(&dev->dst, ev.bdaddr);
+
+	if (buf[0] == HID_MSG_DATA) {
+		ev.status = HAL_HID_STATUS_OK;
+		if (buf[1] == HID_PROTO_REPORT)
+			ev.mode = HAL_HID_REPORT_PROTOCOL;
+		else if (buf[1] == HID_PROTO_BOOT)
+			ev.mode = HAL_HID_BOOT_PROTOCOL;
+		else
+			ev.mode = HAL_HID_UNSUPPORTED_PROTOCOL;
+
+	} else {
+		ev.status = buf[0];
+		ev.mode = HAL_HID_UNSUPPORTED_PROTOCOL;
+	}
+
+	ipc_send(notification_io, HAL_SERVICE_ID_HIDHOST,
+				HAL_EV_HID_PROTO_MODE, sizeof(ev), &ev, -1);
+}
+
+static gboolean ctrl_io_watch_cb(GIOChannel *chan, gpointer data)
+{
+	struct hid_device *dev = data;
+	int fd, bread;
+	uint8_t buf[UHID_DATA_MAX];
+
+	DBG("");
+
+	fd = g_io_channel_unix_get_fd(chan);
+	bread = read(fd, buf, sizeof(buf));
+	if (bread < 0) {
+		error("read: %s(%d)", strerror(errno), -errno);
+		return TRUE;
+	}
+
+	switch (dev->last_hid_msg) {
+	case HID_MSG_GET_PROTOCOL:
+		bt_hid_notify_proto_mode(dev, buf, bread);
+		break;
+	default:
+		DBG("unhandled hid msg type 0x%02x", dev->last_hid_msg);
+	}
+
+	/* reset msg type request */
+	dev->last_hid_msg = 0;
+
+	return TRUE;
+}
+
 static gboolean ctrl_watch_cb(GIOChannel *chan, GIOCondition cond,
 								gpointer data)
 {
 	struct hid_device *dev = data;
 	char address[18];
 
+	if (cond & G_IO_IN)
+		return ctrl_io_watch_cb(chan, data);
+
 	ba2str(&dev->dst, address);
 	bt_hid_notify_state(dev, HAL_HID_STATE_DISCONNECTED);
 
@@ -395,8 +466,8 @@ static void control_connect_cb(GIOChannel *chan, GError *conn_err,
 	}
 
 	dev->ctrl_watch = g_io_add_watch(dev->ctrl_io,
-					G_IO_HUP | G_IO_ERR | G_IO_NVAL,
-					ctrl_watch_cb, dev);
+				G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_NVAL,
+				ctrl_watch_cb, dev);
 
 	return;
 
@@ -591,9 +662,38 @@ static uint8_t bt_hid_info(struct hal_cmd_hid_set_info *cmd, uint16_t len)
 static uint8_t bt_hid_get_protocol(struct hal_cmd_hid_get_protocol *cmd,
 								uint16_t len)
 {
-	DBG("Not Implemented");
+	struct hid_device *dev;
+	GSList *l;
+	bdaddr_t dst;
+	int fd;
+	uint8_t hdr[1];
 
-	return HAL_STATUS_FAILED;
+	DBG("");
+
+	if (len < sizeof(*cmd))
+		return HAL_STATUS_INVALID;
+
+	android2bdaddr(&cmd->bdaddr, &dst);
+
+	l = g_slist_find_custom(devices, &dst, device_cmp);
+	if (!l)
+		return HAL_STATUS_FAILED;
+
+	dev = l->data;
+
+	if (dev->boot_dev)
+		return HAL_STATUS_UNSUPPORTED;
+
+	hdr[0] = HID_MSG_GET_PROTOCOL | cmd->mode;
+	fd = g_io_channel_unix_get_fd(dev->ctrl_io);
+
+	if (write(fd, hdr, sizeof(hdr)) < 0) {
+		error("error while querying device protocol");
+		return HAL_STATUS_FAILED;
+	}
+
+	dev->last_hid_msg = HID_MSG_GET_PROTOCOL;
+	return HAL_STATUS_SUCCESS;
 }
 
 static uint8_t bt_hid_set_protocol(struct hal_cmd_hid_set_protocol *cmd,
-- 
1.8.1.2

[PATCH_v3 02/04] android/hid: Implement hid set protocol in daemon

[Ravi kumar Veeramally]

This patch requests hid device to set protocol mode and reads
reply message and sends notification to hal.
---
 android/hid.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/android/hid.c b/android/hid.c
index 97835d9..d387d6d 100644
--- a/android/hid.c
+++ b/android/hid.c
@@ -56,6 +56,7 @@
 
 /* HID message types */
 #define HID_MSG_GET_PROTOCOL	0x60
+#define HID_MSG_SET_PROTOCOL	0x70
 #define HID_MSG_DATA		0xa0
 
 /* HID protocol header parameters */
@@ -299,6 +300,7 @@ static gboolean ctrl_io_watch_cb(GIOChannel *chan, gpointer data)
 
 	switch (dev->last_hid_msg) {
 	case HID_MSG_GET_PROTOCOL:
+	case HID_MSG_SET_PROTOCOL:
 		bt_hid_notify_proto_mode(dev, buf, bread);
 		break;
 	default:
@@ -699,9 +701,38 @@ static uint8_t bt_hid_get_protocol(struct hal_cmd_hid_get_protocol *cmd,
 static uint8_t bt_hid_set_protocol(struct hal_cmd_hid_set_protocol *cmd,
 								uint16_t len)
 {
-	DBG("Not Implemented");
+	struct hid_device *dev;
+	GSList *l;
+	bdaddr_t dst;
+	int fd;
+	uint8_t hdr[1];
 
-	return HAL_STATUS_FAILED;
+	DBG("");
+
+	if (len < sizeof(*cmd))
+		return HAL_STATUS_INVALID;
+
+	android2bdaddr(&cmd->bdaddr, &dst);
+
+	l = g_slist_find_custom(devices, &dst, device_cmp);
+	if (!l)
+		return HAL_STATUS_FAILED;
+
+	dev = l->data;
+
+	if (dev->boot_dev)
+		return HAL_STATUS_UNSUPPORTED;
+
+	hdr[0] = HID_MSG_SET_PROTOCOL | cmd->mode;
+	fd = g_io_channel_unix_get_fd(dev->ctrl_io);
+
+	if (write(fd, hdr, sizeof(hdr)) < 0) {
+		error("error while setting device protocol");
+		return HAL_STATUS_FAILED;
+	}
+
+	dev->last_hid_msg = HID_MSG_SET_PROTOCOL;
+	return HAL_STATUS_SUCCESS;
 }
 
 static uint8_t bt_hid_get_report(struct hal_cmd_hid_get_report *cmd,
-- 
1.8.1.2

[PATCH_v3 03/04] android/hid: Implement hid get report in daemon

[Ravi kumar Veeramally]

This patch requests hid device report and reads reply
message and sends notification to hal.
---
 android/hal-hidhost.c |   1 +
 android/hid.c         | 105 +++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 104 insertions(+), 2 deletions(-)

diff --git a/android/hal-hidhost.c b/android/hal-hidhost.c
index 13928e6..f554cb2 100644
--- a/android/hal-hidhost.c
+++ b/android/hal-hidhost.c
@@ -251,6 +251,7 @@ static bt_status_t hh_get_report(bt_bdaddr_t *bd_addr,
 
 	memcpy(cmd.bdaddr, bd_addr, sizeof(cmd.bdaddr));
 	cmd.id = reportId;
+	cmd.buf = bufferSize;
 
 	switch (reportType) {
 	case BTHH_INPUT_REPORT:
diff --git a/android/hid.c b/android/hid.c
index d387d6d..f1bf9e8 100644
--- a/android/hid.c
+++ b/android/hid.c
@@ -55,14 +55,23 @@
 #define UHID_DEVICE_FILE	"/dev/uhid"
 
 /* HID message types */
+#define HID_MSG_GET_REPORT	0x40
 #define HID_MSG_GET_PROTOCOL	0x60
 #define HID_MSG_SET_PROTOCOL	0x70
 #define HID_MSG_DATA		0xa0
 
+/* HID data types */
+#define HID_DATA_TYPE_INPUT	0x01
+#define HID_DATA_TYPE_OUTPUT	0x02
+#define HID_DATA_TYPE_FEATURE	0x03
+
 /* HID protocol header parameters */
 #define HID_PROTO_BOOT		0x00
 #define HID_PROTO_REPORT	0x01
 
+/* HID GET REPORT Size Field */
+#define HID_GET_REPORT_SIZE_FIELD	0x08
+
 static GIOChannel *notification_io = NULL;
 static GIOChannel *ctrl_io = NULL;
 static GIOChannel *intr_io = NULL;
@@ -283,6 +292,52 @@ static void bt_hid_notify_proto_mode(struct hid_device *dev, uint8_t *buf,
 				HAL_EV_HID_PROTO_MODE, sizeof(ev), &ev, -1);
 }
 
+static void bt_hid_notify_get_report(struct hid_device *dev, uint8_t *buf,
+									int len)
+{
+	struct hal_ev_hid_get_report *ev;
+	int ev_len;
+	char address[18];
+
+	ba2str(&dev->dst, address);
+	DBG("device %s", address);
+
+	ev_len = sizeof(*ev) + sizeof(struct hal_ev_hid_get_report) + 1;
+
+	if (!((buf[0] == (HID_MSG_DATA | HID_DATA_TYPE_INPUT)) ||
+			(buf[0] == (HID_MSG_DATA | HID_DATA_TYPE_OUTPUT)) ||
+			(buf[0]	== (HID_MSG_DATA | HID_DATA_TYPE_FEATURE)))) {
+		ev = g_malloc(len);
+		memset(ev, 0, ev_len);
+		ev->status = buf[0];
+		bdaddr2android(&dev->dst, ev->bdaddr);
+		goto send;
+	}
+
+	/* Report porotocol mode reply contains id after hdr, in boot
+	 * protocol mode id doesn't exist */
+	ev_len += (dev->boot_dev) ? (len - 1) : (len - 2);
+	ev = g_malloc(ev_len);
+	memset(ev, 0, ev_len);
+	ev->status = HAL_HID_STATUS_OK;
+	bdaddr2android(&dev->dst, ev->bdaddr);
+
+	/* Report porotocol mode reply contains id after hdr, in boot
+	 * protocol mode id doesn't exist */
+	if (dev->boot_dev) {
+		ev->len = len - 1;
+		memcpy(ev->data, buf + 1, ev->len);
+	} else {
+		ev->len = len - 2;
+		memcpy(ev->data, buf + 2, ev->len);
+	}
+
+send:
+	ipc_send(notification_io, HAL_SERVICE_ID_HIDHOST, HAL_EV_HID_GET_REPORT,
+						ev_len, ev, -1);
+	g_free(ev);
+}
+
 static gboolean ctrl_io_watch_cb(GIOChannel *chan, gpointer data)
 {
 	struct hid_device *dev = data;
@@ -303,6 +358,9 @@ static gboolean ctrl_io_watch_cb(GIOChannel *chan, gpointer data)
 	case HID_MSG_SET_PROTOCOL:
 		bt_hid_notify_proto_mode(dev, buf, bread);
 		break;
+	case HID_MSG_GET_REPORT:
+		bt_hid_notify_get_report(dev, buf, bread);
+		break;
 	default:
 		DBG("unhandled hid msg type 0x%02x", dev->last_hid_msg);
 	}
@@ -738,9 +796,51 @@ static uint8_t bt_hid_set_protocol(struct hal_cmd_hid_set_protocol *cmd,
 static uint8_t bt_hid_get_report(struct hal_cmd_hid_get_report *cmd,
 								uint16_t len)
 {
-	DBG("Not Implemented");
+	struct hid_device *dev;
+	GSList *l;
+	bdaddr_t dst;
+	int fd;
+	uint8_t *req;
+	uint8_t req_size;
 
-	return HAL_STATUS_FAILED;
+	DBG("");
+
+	if (len < sizeof(*cmd))
+		return HAL_STATUS_INVALID;
+
+	android2bdaddr(&cmd->bdaddr, &dst);
+
+	l = g_slist_find_custom(devices, &dst, device_cmp);
+	if (!l)
+		return HAL_STATUS_FAILED;
+
+	dev = l->data;
+	req_size = (cmd->buf > 0) ? 4 : 2;
+	req = g_try_malloc0(req_size);
+	if (!req)
+		return HAL_STATUS_NOMEM;
+
+	req[0] = HID_MSG_GET_REPORT | cmd->type;
+
+	if (cmd->buf > 0)
+		req[0] = req[0] | HID_GET_REPORT_SIZE_FIELD;
+
+	req[1] = cmd->id;
+
+	if (cmd->buf > 0)
+		bt_put_le16(cmd->buf, (req + 2));
+
+	fd = g_io_channel_unix_get_fd(dev->ctrl_io);
+
+	if (write(fd, req, req_size) < 0) {
+		error("error while querying device protocol");
+		g_free(req);
+		return HAL_STATUS_FAILED;
+	}
+
+	dev->last_hid_msg = HID_MSG_GET_REPORT;
+	g_free(req);
+	return HAL_STATUS_SUCCESS;
 }
 
 static uint8_t bt_hid_set_report(struct hal_cmd_hid_set_report *cmd,
-- 
1.8.1.2

[PATCH_v3 04/04] android/hid: Implement hid set report in daemon

[Ravi kumar Veeramally]

This patch requests hid device to set report.
---
 android/hal-hidhost.c |  2 ++
 android/hal-msg.h     |  6 ++++--
 android/hid.c         | 40 ++++++++++++++++++++++++++++++++++++++--
 3 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/android/hal-hidhost.c b/android/hal-hidhost.c
index f554cb2..29e3ee0 100644
--- a/android/hal-hidhost.c
+++ b/android/hal-hidhost.c
@@ -284,6 +284,8 @@ static bt_status_t hh_set_report(bt_bdaddr_t *bd_addr,
 		return BT_STATUS_PARM_INVALID;
 
 	memcpy(cmd.bdaddr, bd_addr, sizeof(cmd.bdaddr));
+	cmd.len = strlen(report);
+	memcpy(cmd.data, report, cmd.len);
 
 	switch (reportType) {
 	case BTHH_INPUT_REPORT:
diff --git a/android/hal-msg.h b/android/hal-msg.h
index bc7df6b..2c3067f 100644
--- a/android/hal-msg.h
+++ b/android/hal-msg.h
@@ -290,8 +290,10 @@ struct hal_cmd_hid_get_report {
 
 #define HAL_OP_HID_SET_REPORT		0x08
 struct hal_cmd_hid_set_report {
-	uint8_t bdaddr[6];
-	uint8_t type;
+	uint8_t  bdaddr[6];
+	uint8_t  type;
+	uint16_t len;
+	uint8_t  data[670];
 } __attribute__((packed));
 
 #define HAL_OP_HID_SEND_DATA		0x09
diff --git a/android/hid.c b/android/hid.c
index f1bf9e8..25e33fa 100644
--- a/android/hid.c
+++ b/android/hid.c
@@ -56,6 +56,7 @@
 
 /* HID message types */
 #define HID_MSG_GET_REPORT	0x40
+#define HID_MSG_SET_REPORT	0x50
 #define HID_MSG_GET_PROTOCOL	0x60
 #define HID_MSG_SET_PROTOCOL	0x70
 #define HID_MSG_DATA		0xa0
@@ -846,9 +847,44 @@ static uint8_t bt_hid_get_report(struct hal_cmd_hid_get_report *cmd,
 static uint8_t bt_hid_set_report(struct hal_cmd_hid_set_report *cmd,
 								uint16_t len)
 {
-	DBG("Not Implemented");
+	struct hid_device *dev;
+	GSList *l;
+	bdaddr_t dst;
+	int fd;
+	uint8_t *req;
+	uint8_t req_size;
 
-	return HAL_STATUS_FAILED;
+	DBG("");
+
+	if (len < sizeof(*cmd))
+		return HAL_STATUS_INVALID;
+
+	android2bdaddr(&cmd->bdaddr, &dst);
+
+	l = g_slist_find_custom(devices, &dst, device_cmp);
+	if (!l)
+		return HAL_STATUS_FAILED;
+
+	dev = l->data;
+	req_size = 1 + cmd->len;
+	req = g_try_malloc0(req_size);
+	if (!req)
+		return HAL_STATUS_NOMEM;
+
+	req[0] = HID_MSG_SET_REPORT | cmd->type;
+	memcpy(req + 1, cmd->data, req_size - 1);
+
+	fd = g_io_channel_unix_get_fd(dev->ctrl_io);
+
+	if (write(fd, req, req_size) < 0) {
+		error("error while querying device protocol");
+		g_free(req);
+		return HAL_STATUS_FAILED;
+	}
+
+	dev->last_hid_msg = HID_MSG_SET_REPORT;
+	g_free(req);
+	return HAL_STATUS_SUCCESS;
 }
 
 static uint8_t bt_hid_send_data(struct hal_cmd_hid_send_data *cmd,
-- 
1.8.1.2

Re: [PATCH_v3 01/04] android/hid: Implement hid get protocol in daemon

[Johan Hedberg]

Hi Ravi,

On Tue, Nov 05, 2013, Ravi kumar Veeramally wrote:
> +static gboolean ctrl_io_watch_cb(GIOChannel *chan, gpointer data)
> +{
> +	struct hid_device *dev = data;
> +	int fd, bread;
> +	uint8_t buf[UHID_DATA_MAX];
> +
> +	DBG("");
> +
> +	fd = g_io_channel_unix_get_fd(chan);
> +	bread = read(fd, buf, sizeof(buf));
> +	if (bread < 0) {
> +		error("read: %s(%d)", strerror(errno), -errno);
> +		return TRUE;
> +	}
> +
> +	switch (dev->last_hid_msg) {
> +	case HID_MSG_GET_PROTOCOL:
> +		bt_hid_notify_proto_mode(dev, buf, bread);
> +		break;
> +	default:
> +		DBG("unhandled hid msg type 0x%02x", dev->last_hid_msg);
> +	}

This doesn't really make sense to me. If you only set last_hid_msg when
you send a code that you do support, then why would the value of
last_hid_msg ever contain a type that you do not support? (assuming you
always add an entry to this switch statement in the same patch that you
add a corresponding write for the type).

Also, since you don't seem to be using last_hid_msg for anything else
than printing this debug message, I'm wondering is there really any
value for it? Previously (based on our IRC) discussion I understood that
it had some actual functional value that helped determine what to send
to the HAL, but now I'm not seeing it anywhere in the patch. I might
have missed it though (in which case please enlighten me :)

Johan

Re: [PATCH_v3 02/04] android/hid: Implement hid set protocol in daemon

[Johan Hedberg]

Hi Ravi,

On Tue, Nov 05, 2013, Ravi kumar Veeramally wrote:
>  static uint8_t bt_hid_set_protocol(struct hal_cmd_hid_set_protocol *cmd,
>  								uint16_t len)
>  {
> -	DBG("Not Implemented");
> +	struct hid_device *dev;
> +	GSList *l;
> +	bdaddr_t dst;
> +	int fd;
> +	uint8_t hdr[1];

If it's just one element there's no need for an array. Just use
"uint8_t hdr;" instead.

> +	hdr[0] = HID_MSG_SET_PROTOCOL | cmd->mode;

And then here hdr = ...;

> +	fd = g_io_channel_unix_get_fd(dev->ctrl_io);
> +
> +	if (write(fd, hdr, sizeof(hdr)) < 0) {

And here &hdr, sizeof(hdr)

Johan

Re: [PATCH_v3 03/04] android/hid: Implement hid get report in daemon

[Johan Hedberg]

Hi Ravi,

On Tue, Nov 05, 2013, Ravi kumar Veeramally wrote:
> +	if (!((buf[0] == (HID_MSG_DATA | HID_DATA_TYPE_INPUT)) ||
> +			(buf[0] == (HID_MSG_DATA | HID_DATA_TYPE_OUTPUT)) ||
> +			(buf[0]	== (HID_MSG_DATA | HID_DATA_TYPE_FEATURE)))) {
> +		ev = g_malloc(len);
> +		memset(ev, 0, ev_len);

Is it intentional that you allocate a different length than what you
memset to 0 here? If they should be the same just use g_malloc0, and if
not a code comment might be in order (to explain what the actual
intention is).

> +	ev = g_malloc(ev_len);
> +	memset(ev, 0, ev_len);

Here g_malloc0 makes more sense.

> +	ev->status = HAL_HID_STATUS_OK;
> +	bdaddr2android(&dev->dst, ev->bdaddr);
> +
> +	/* Report porotocol mode reply contains id after hdr, in boot
> +	 * protocol mode id doesn't exist */
> +	if (dev->boot_dev) {
> +		ev->len = len - 1;
> +		memcpy(ev->data, buf + 1, ev->len);
> +	} else {
> +		ev->len = len - 2;
> +		memcpy(ev->data, buf + 2, ev->len);
> +	}
> +
> +send:
> +	ipc_send(notification_io, HAL_SERVICE_ID_HIDHOST, HAL_EV_HID_GET_REPORT,
> +						ev_len, ev, -1);

This doesn't look right for your first allocation (you claim that the
length of ev is ev_len, but in fact you allocated len amount of bytes.

Johan

Re: [PATCH_v3 01/04] android/hid: Implement hid get protocol in daemon

[Ravi Kumar Veeramally]

Hi Johan,

On 11/05/2013 03:10 PM, Johan Hedberg wrote:
> Hi Ravi,
>
> On Tue, Nov 05, 2013, Ravi kumar Veeramally wrote:
>> +static gboolean ctrl_io_watch_cb(GIOChannel *chan, gpointer data)
>> +{
>> +	struct hid_device *dev = data;
>> +	int fd, bread;
>> +	uint8_t buf[UHID_DATA_MAX];
>> +
>> +	DBG("");
>> +
>> +	fd = g_io_channel_unix_get_fd(chan);
>> +	bread = read(fd, buf, sizeof(buf));
>> +	if (bread < 0) {
>> +		error("read: %s(%d)", strerror(errno), -errno);
>> +		return TRUE;
>> +	}
>> +
>> +	switch (dev->last_hid_msg) {
>> +	case HID_MSG_GET_PROTOCOL:
>> +		bt_hid_notify_proto_mode(dev, buf, bread);
>> +		break;
>> +	default:
>> +		DBG("unhandled hid msg type 0x%02x", dev->last_hid_msg);
>> +	}
> This doesn't really make sense to me. If you only set last_hid_msg when
> you send a code that you do support, then why would the value of
> last_hid_msg ever contain a type that you do not support? (assuming you
> always add an entry to this switch statement in the same patch that you
> add a corresponding write for the type).
>
> Also, since you don't seem to be using last_hid_msg for anything else
> than printing this debug message, I'm wondering is there really any
> value for it? Previously (based on our IRC) discussion I understood that
> it had some actual functional value that helped determine what to send
> to the HAL, but now I'm not seeing it anywhere in the patch. I might
> have missed it though (in which case please enlighten me :)
  I don't know if I understand you correctly, but at the end of all patches
  it looks like this.
          switch (dev->last_hid_msg) {
         case HID_MSG_GET_PROTOCOL:
         case HID_MSG_SET_PROTOCOL:
                 bt_hid_notify_proto_mode(dev, buf, bread);
                 break;
         case HID_MSG_GET_REPORT:
                 bt_hid_notify_get_report(dev, buf, bread);
                 break;
         default:
                 DBG("unhandled hid msg type 0x%02x", dev->last_hid_msg);
         }

based on last_hid_msg switch case, it will call respective function,
default statement is for debug purpose if we miss something from hid device.

Thanks,
Ravi.

Re: [PATCH_v3 02/04] android/hid: Implement hid set protocol in daemon

[Ravi Kumar Veeramally]

Hi Johan,

On 11/05/2013 03:12 PM, Johan Hedberg wrote:
> Hi Ravi,
>
> On Tue, Nov 05, 2013, Ravi kumar Veeramally wrote:
>>   static uint8_t bt_hid_set_protocol(struct hal_cmd_hid_set_protocol *cmd,
>>   								uint16_t len)
>>   {
>> -	DBG("Not Implemented");
>> +	struct hid_device *dev;
>> +	GSList *l;
>> +	bdaddr_t dst;
>> +	int fd;
>> +	uint8_t hdr[1];
> If it's just one element there's no need for an array. Just use
> "uint8_t hdr;" instead.
>
>> +	hdr[0] = HID_MSG_SET_PROTOCOL | cmd->mode;
> And then here hdr = ...;
>
>> +	fd = g_io_channel_unix_get_fd(dev->ctrl_io);
>> +
>> +	if (write(fd, hdr, sizeof(hdr)) < 0) {
> And here &hdr, sizeof(hdr)
  Ok.

Thanks,
Ravi.

Re: [PATCH_v3 03/04] android/hid: Implement hid get report in daemon

[Ravi Kumar Veeramally]

On 11/05/2013 03:16 PM, Johan Hedberg wrote:
> Hi Ravi,
>
> On Tue, Nov 05, 2013, Ravi kumar Veeramally wrote:
>> +	if (!((buf[0] == (HID_MSG_DATA | HID_DATA_TYPE_INPUT)) ||
>> +			(buf[0] == (HID_MSG_DATA | HID_DATA_TYPE_OUTPUT)) ||
>> +			(buf[0]	== (HID_MSG_DATA | HID_DATA_TYPE_FEATURE)))) {
>> +		ev = g_malloc(len);
>> +		memset(ev, 0, ev_len);
> Is it intentional that you allocate a different length than what you
> memset to 0 here? If they should be the same just use g_malloc0, and if
> not a code comment might be in order (to explain what the actual
> intention is).
  Ok, type, it should be ev_len.
>> +	ev = g_malloc(ev_len);
>> +	memset(ev, 0, ev_len);
> Here g_malloc0 makes more sense.
>
>> +	ev->status = HAL_HID_STATUS_OK;
>> +	bdaddr2android(&dev->dst, ev->bdaddr);
>> +
>> +	/* Report porotocol mode reply contains id after hdr, in boot
>> +	 * protocol mode id doesn't exist */
>> +	if (dev->boot_dev) {
>> +		ev->len = len - 1;
>> +		memcpy(ev->data, buf + 1, ev->len);
>> +	} else {
>> +		ev->len = len - 2;
>> +		memcpy(ev->data, buf + 2, ev->len);
>> +	}
>> +
>> +send:
>> +	ipc_send(notification_io, HAL_SERVICE_ID_HIDHOST, HAL_EV_HID_GET_REPORT,
>> +						ev_len, ev, -1);
> This doesn't look right for your first allocation (you claim that the
> length of ev is ev_len, but in fact you allocated len amount of bytes.
  Ok. I will fix it.

Thanks,
Ravi.

Re: [PATCH_v3 01/04] android/hid: Implement hid get protocol in daemon

[Johan Hedberg]

Hi Ravi,

On Tue, Nov 05, 2013, Ravi Kumar Veeramally wrote:
> >On Tue, Nov 05, 2013, Ravi kumar Veeramally wrote:
> >>+static gboolean ctrl_io_watch_cb(GIOChannel *chan, gpointer data)
> >>+{
> >>+	struct hid_device *dev = data;
> >>+	int fd, bread;
> >>+	uint8_t buf[UHID_DATA_MAX];
> >>+
> >>+	DBG("");
> >>+
> >>+	fd = g_io_channel_unix_get_fd(chan);
> >>+	bread = read(fd, buf, sizeof(buf));
> >>+	if (bread < 0) {
> >>+		error("read: %s(%d)", strerror(errno), -errno);
> >>+		return TRUE;
> >>+	}
> >>+
> >>+	switch (dev->last_hid_msg) {
> >>+	case HID_MSG_GET_PROTOCOL:
> >>+		bt_hid_notify_proto_mode(dev, buf, bread);
> >>+		break;
> >>+	default:
> >>+		DBG("unhandled hid msg type 0x%02x", dev->last_hid_msg);
> >>+	}
> >This doesn't really make sense to me. If you only set last_hid_msg when
> >you send a code that you do support, then why would the value of
> >last_hid_msg ever contain a type that you do not support? (assuming you
> >always add an entry to this switch statement in the same patch that you
> >add a corresponding write for the type).
> >
> >Also, since you don't seem to be using last_hid_msg for anything else
> >than printing this debug message, I'm wondering is there really any
> >value for it? Previously (based on our IRC) discussion I understood that
> >it had some actual functional value that helped determine what to send
> >to the HAL, but now I'm not seeing it anywhere in the patch. I might
> >have missed it though (in which case please enlighten me :)
>  I don't know if I understand you correctly, but at the end of all patches
>  it looks like this.
>          switch (dev->last_hid_msg) {
>         case HID_MSG_GET_PROTOCOL:
>         case HID_MSG_SET_PROTOCOL:
>                 bt_hid_notify_proto_mode(dev, buf, bread);
>                 break;
>         case HID_MSG_GET_REPORT:
>                 bt_hid_notify_get_report(dev, buf, bread);
>                 break;
>         default:
>                 DBG("unhandled hid msg type 0x%02x", dev->last_hid_msg);
>         }
> 
> based on last_hid_msg switch case, it will call respective function,
> default statement is for debug purpose if we miss something from hid device.

I only saw the first two case statements and didn't look deeper into the
patch set, sorry. In this case I do see the point of the variable and
you may keep it as is.

My earlier comment about the debug statement still holds though, i.e. if
the default entry gets triggered last_hid_msg will not contain the
unknown msg type. In this case this will simply be an unexpected message
whose type we do not know (and if there's a debug log that's what it
should say).

Johan

Re: [PATCH_v3 01/04] android/hid: Implement hid get protocol in daemon

[ravikumar.veeramally]

Hi Johan,

> Hi Ravi,
>
> On Tue, Nov 05, 2013, Ravi Kumar Veeramally wrote:
>> >On Tue, Nov 05, 2013, Ravi kumar Veeramally wrote:
>> >>+static gboolean ctrl_io_watch_cb(GIOChannel *chan, gpointer data)
>> >>+{
>> >>+	struct hid_device *dev = data;
>> >>+	int fd, bread;
>> >>+	uint8_t buf[UHID_DATA_MAX];
>> >>+
>> >>+	DBG("");
>> >>+
>> >>+	fd = g_io_channel_unix_get_fd(chan);
>> >>+	bread = read(fd, buf, sizeof(buf));
>> >>+	if (bread < 0) {
>> >>+		error("read: %s(%d)", strerror(errno), -errno);
>> >>+		return TRUE;
>> >>+	}
>> >>+
>> >>+	switch (dev->last_hid_msg) {
>> >>+	case HID_MSG_GET_PROTOCOL:
>> >>+		bt_hid_notify_proto_mode(dev, buf, bread);
>> >>+		break;
>> >>+	default:
>> >>+		DBG("unhandled hid msg type 0x%02x", dev->last_hid_msg);
>> >>+	}
>> >This doesn't really make sense to me. If you only set last_hid_msg when
>> >you send a code that you do support, then why would the value of
>> >last_hid_msg ever contain a type that you do not support? (assuming you
>> >always add an entry to this switch statement in the same patch that you
>> >add a corresponding write for the type).
>> >
>> >Also, since you don't seem to be using last_hid_msg for anything else
>> >than printing this debug message, I'm wondering is there really any
>> >value for it? Previously (based on our IRC) discussion I understood
>> that
>> >it had some actual functional value that helped determine what to send
>> >to the HAL, but now I'm not seeing it anywhere in the patch. I might
>> >have missed it though (in which case please enlighten me :)
>>  I don't know if I understand you correctly, but at the end of all
>> patches
>>  it looks like this.
>>          switch (dev->last_hid_msg) {
>>         case HID_MSG_GET_PROTOCOL:
>>         case HID_MSG_SET_PROTOCOL:
>>                 bt_hid_notify_proto_mode(dev, buf, bread);
>>                 break;
>>         case HID_MSG_GET_REPORT:
>>                 bt_hid_notify_get_report(dev, buf, bread);
>>                 break;
>>         default:
>>                 DBG("unhandled hid msg type 0x%02x", dev->last_hid_msg);
>>         }
>>
>> based on last_hid_msg switch case, it will call respective function,
>> default statement is for debug purpose if we miss something from hid
>> device.
>
> I only saw the first two case statements and didn't look deeper into the
> patch set, sorry. In this case I do see the point of the variable and
> you may keep it as is.
Ok, np.

>
> My earlier comment about the debug statement still holds though, i.e. if
> the default entry gets triggered last_hid_msg will not contain the
> unknown msg type. In this case this will simply be an unexpected message
> whose type we do not know (and if there's a debug log that's what it
> should say).

I will remove the debug statement which doesn't help much.

Thanks,
Ravi.