[PATCH_v3 0/5] Fixed and implemented set report and send data ifaces

[PATCH_v3 0/5] Fixed and implemented set report and send data ifaces

[Ravi kumar Veeramally]

v3: Fixed issues as per Johan's and Jerzy' comments and suggestions
    Implemented hex2bin as a static funtion and split the patches.

v2: Fixed issues as per Johan's comments and suggestions.
    This patch adds missing hid ipc documentation, hal headers,
    renaming and virtual unplug notification in hid HAL.
    Set report and send data interfaces are receiving data in ascii format
    but it should be in hex format. Provided utility for this and fixed
    set report data preparation. Implemented virtual unplug and send data
    methods in daemon.

v1: This patch adds missing hid ipc documentation, hal headers,
    renaming and virtual unplug notification in hid HAL.
    send RFC for ascii2hex utility.
    Unsupported methods are virtual unplug and send data in daemon.

Ravi kumar Veeramally (5):
  android/hid: Fix set seport ipc cmd preparation
  android/hid: Fix set report data format in daemon
  android/hid: Fill send data command struct in hal-hidhost
  android/hid: Add send data implemention in daemon
  android/hid: Add virtual unplug implemention in daemon

 android/hal-hidhost.c | 47 +++++++++++++++++-------
 android/hal-msg.h     |  2 +-
 android/hidhost.c     | 99 +++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 127 insertions(+), 21 deletions(-)

-- 
1.8.3.2

[PATCH_v3 1/5] android/hid: Fix set seport ipc cmd preparation

[Ravi kumar Veeramally]

Now report data is not fixed array. Allocate proper memory
and send ipc cmd.
---
 android/hal-hidhost.c | 29 ++++++++++++++++++++---------
 android/hal-msg.h     |  2 +-
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/android/hal-hidhost.c b/android/hal-hidhost.c
index 34f9f77..ce3dcd7 100644
--- a/android/hal-hidhost.c
+++ b/android/hal-hidhost.c
@@ -18,6 +18,7 @@
 #include <stdbool.h>
 #include <stddef.h>
 #include <string.h>
+#include <stdlib.h>
 
 #include "hal-log.h"
 #include "hal.h"
@@ -297,7 +298,8 @@ static bt_status_t set_report(bt_bdaddr_t *bd_addr,
 						bthh_report_type_t report_type,
 						char *report)
 {
-	struct hal_cmd_hidhost_set_report cmd;
+	struct hal_cmd_hidhost_set_report *cmd;
+	int cmd_len, status;
 
 	DBG("");
 
@@ -307,26 +309,35 @@ static bt_status_t set_report(bt_bdaddr_t *bd_addr,
 	if (!bd_addr || !report)
 		return BT_STATUS_PARM_INVALID;
 
-	memcpy(cmd.bdaddr, bd_addr, sizeof(cmd.bdaddr));
-	cmd.len = strlen(report);
-	memcpy(cmd.data, report, cmd.len);
+	cmd_len = sizeof(*cmd) + sizeof(struct hal_cmd_hidhost_set_report)
+							+ 1 + strlen(report);
+
+	cmd = malloc(cmd_len);
+	memset(cmd, 0, cmd_len);
+	memcpy(cmd->bdaddr, bd_addr, sizeof(cmd->bdaddr));
+	cmd->len = strlen(report);
+	memcpy(cmd->data, report, cmd->len);
 
 	switch (report_type) {
 	case BTHH_INPUT_REPORT:
-		cmd.type = HAL_HIDHOST_INPUT_REPORT;
+		cmd->type = HAL_HIDHOST_INPUT_REPORT;
 		break;
 	case BTHH_OUTPUT_REPORT:
-		cmd.type = HAL_HIDHOST_OUTPUT_REPORT;
+		cmd->type = HAL_HIDHOST_OUTPUT_REPORT;
 		break;
 	case BTHH_FEATURE_REPORT:
-		cmd.type = HAL_HIDHOST_FEATURE_REPORT;
+		cmd->type = HAL_HIDHOST_FEATURE_REPORT;
 		break;
 	default:
+		free(cmd);
 		return BT_STATUS_PARM_INVALID;
 	}
 
-	return hal_ipc_cmd(HAL_SERVICE_ID_HIDHOST, HAL_OP_HIDHOST_SET_REPORT,
-				sizeof(cmd), &cmd, 0, NULL, NULL);
+	status = hal_ipc_cmd(HAL_SERVICE_ID_HIDHOST, HAL_OP_HIDHOST_SET_REPORT,
+						cmd_len, cmd, 0, NULL, NULL);
+	free(cmd);
+
+	return status;
 }
 
 static bt_status_t send_data(bt_bdaddr_t *bd_addr, char *data)
diff --git a/android/hal-msg.h b/android/hal-msg.h
index 851875e..7ba2e00 100644
--- a/android/hal-msg.h
+++ b/android/hal-msg.h
@@ -293,7 +293,7 @@ struct hal_cmd_hidhost_set_report {
 	uint8_t  bdaddr[6];
 	uint8_t  type;
 	uint16_t len;
-	uint8_t  data[670];
+	uint8_t  data[0];
 } __attribute__((packed));
 
 #define HAL_OP_HIDHOST_SEND_DATA		0x09
-- 
1.8.3.2

[PATCH_v3 2/5] android/hid: Fix set report data format in daemon

[Ravi kumar Veeramally]

Report data coming to HAL is in ascii format, HAL sends
data in hex to daemon, so convert to binary.
---
 android/hidhost.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/android/hidhost.c b/android/hidhost.c
index c7b4114..9b4bb15 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -98,6 +98,14 @@ struct hid_device {
 	uint8_t		last_hid_msg;
 };
 
+static void hex2bin(const uint8_t *ascii, int ascii_len, uint8_t *hex)
+{
+	int i;
+
+	for (i = 0; i < ascii_len / 2; i++)
+		sscanf((char *) &ascii[i * 2], "%hhx", &hex[i]);
+}
+
 static int device_cmp(gconstpointer s, gconstpointer user_data)
 {
 	const struct hid_device *dev = s;
@@ -900,18 +908,20 @@ static uint8_t bt_hid_set_report(struct hal_cmd_hidhost_set_report *cmd,
 		return HAL_STATUS_FAILED;
 
 	dev = l->data;
-	req_size = 1 + cmd->len;
+	/* Report data coming to HAL is in ascii format, HAL sends
+	 * data in hex to daemon, so convert to binary. */
+	req_size = 1 + (cmd->len / 2);
 	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);
+	hex2bin(cmd->data, cmd->len, (req + 1));
 
 	fd = g_io_channel_unix_get_fd(dev->ctrl_io);
 
 	if (write(fd, req, req_size) < 0) {
-		error("error while querying device protocol");
+		error("error while sending report");
 		g_free(req);
 		return HAL_STATUS_FAILED;
 	}
-- 
1.8.3.2

[PATCH_v3 3/5] android/hid: Fill send data command struct in hal-hidhost

[Ravi kumar Veeramally]

---
 android/hal-hidhost.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/android/hal-hidhost.c b/android/hal-hidhost.c
index ce3dcd7..ec120ed 100644
--- a/android/hal-hidhost.c
+++ b/android/hal-hidhost.c
@@ -342,7 +342,8 @@ static bt_status_t set_report(bt_bdaddr_t *bd_addr,
 
 static bt_status_t send_data(bt_bdaddr_t *bd_addr, char *data)
 {
-	struct hal_cmd_hidhost_send_data cmd;
+	struct hal_cmd_hidhost_send_data *cmd;
+	int cmd_len, status;
 
 	DBG("");
 
@@ -352,10 +353,19 @@ static bt_status_t send_data(bt_bdaddr_t *bd_addr, char *data)
 	if (!bd_addr || !data)
 		return BT_STATUS_PARM_INVALID;
 
-	memcpy(cmd.bdaddr, bd_addr, sizeof(cmd.bdaddr));
+	cmd_len = sizeof(*cmd) + sizeof(struct hal_cmd_hidhost_send_data)
+							+ 1 + strlen(data);
 
-	return hal_ipc_cmd(HAL_SERVICE_ID_HIDHOST, HAL_OP_HIDHOST_SEND_DATA,
-					sizeof(cmd), &cmd, 0, NULL, NULL);
+	cmd = malloc(cmd_len);
+	memset(cmd, 0, cmd_len);
+	memcpy(cmd->bdaddr, bd_addr, sizeof(cmd->bdaddr));
+	cmd->len = strlen(data);
+	memcpy(cmd->data, data, cmd->len);
+
+	status = hal_ipc_cmd(HAL_SERVICE_ID_HIDHOST, HAL_OP_HIDHOST_SEND_DATA,
+						cmd_len, cmd, 0, NULL, NULL);
+	free(cmd);
+	return status;
 }
 
 static bt_status_t init(bthh_callbacks_t *callbacks)
-- 
1.8.3.2

[PATCH_v3 4/5] android/hid: Add send data implemention in daemon

[Ravi kumar Veeramally]

Send data on interrupt channel on request from hid host.
---
 android/hidhost.c | 38 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/android/hidhost.c b/android/hidhost.c
index 9b4bb15..612dff4 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -934,9 +934,43 @@ static uint8_t bt_hid_set_report(struct hal_cmd_hidhost_set_report *cmd,
 static uint8_t bt_hid_send_data(struct hal_cmd_hidhost_send_data *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 / 2);
+	req = g_try_malloc0(req_size);
+	if (!req)
+		return HAL_STATUS_NOMEM;
+
+	req[0] = HID_MSG_DATA | HID_DATA_TYPE_OUTPUT;
+	hex2bin(cmd->data, cmd->len, (req + 1));
+
+	fd = g_io_channel_unix_get_fd(dev->intr_io);
+
+	if (write(fd, req, req_size) < 0) {
+		error("error while sending data to device");
+		g_free(req);
+		return HAL_STATUS_FAILED;
+	}
+
+	g_free(req);
+	return HAL_STATUS_SUCCESS;
 }
 
 void bt_hid_handle_cmd(int sk, uint8_t opcode, void *buf, uint16_t len)
-- 
1.8.3.2

[PATCH_v3 5/5] android/hid: Add virtual unplug implemention in daemon

[Ravi kumar Veeramally]

Send virtual unplug command to hid device and disconnect and remove
hid device details.
---
 android/hidhost.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/android/hidhost.c b/android/hidhost.c
index 612dff4..ac6a3b9 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -55,6 +55,7 @@
 #define UHID_DEVICE_FILE	"/dev/uhid"
 
 /* HID message types */
+#define HID_MSG_CONTROL		0x10
 #define HID_MSG_GET_REPORT	0x40
 #define HID_MSG_SET_REPORT	0x50
 #define HID_MSG_GET_PROTOCOL	0x60
@@ -73,6 +74,9 @@
 /* HID GET REPORT Size Field */
 #define HID_GET_REPORT_SIZE_FIELD	0x08
 
+/* HID Virtual Cable Unplug */
+#define HID_VIRTUAL_CABLE_UNPLUG	0x05
+
 static int notification_sk = -1;
 static GIOChannel *ctrl_io = NULL;
 static GIOChannel *intr_io = NULL;
@@ -752,9 +756,46 @@ static uint8_t bt_hid_disconnect(struct hal_cmd_hidhost_disconnect *cmd,
 static uint8_t bt_hid_virtual_unplug(struct hal_cmd_hidhost_vp *cmd,
 								uint16_t len)
 {
-	DBG("Not Implemented");
+	struct hid_device *dev;
+	GSList *l;
+	bdaddr_t dst;
+	uint8_t hdr;
+	int fd;
 
-	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;
+	hdr = HID_MSG_CONTROL | HID_VIRTUAL_CABLE_UNPLUG;
+
+	if (!(dev->ctrl_io))
+		return HAL_STATUS_FAILED;
+
+	fd = g_io_channel_unix_get_fd(dev->ctrl_io);
+
+	if (write(fd, &hdr, sizeof(hdr)) < 0) {
+		error("error while virtual unplug command");
+		return HAL_STATUS_FAILED;
+	}
+
+	/* Wait either channels to HUP */
+	if (dev->intr_io)
+		g_io_channel_shutdown(dev->intr_io, TRUE, NULL);
+
+	if (dev->ctrl_io)
+		g_io_channel_shutdown(dev->ctrl_io, TRUE, NULL);
+
+	bt_hid_notify_state(dev, HAL_HIDHOST_STATE_DISCONNECTING);
+
+	return HAL_STATUS_SUCCESS;
 }
 
 static uint8_t bt_hid_info(struct hal_cmd_hidhost_set_info *cmd, uint16_t len)
-- 
1.8.3.2

Re: [PATCH_v3 2/5] android/hid: Fix set report data format in daemon

[Johan Hedberg]

Hi Ravi,

On Fri, Nov 08, 2013, Ravi kumar Veeramally wrote:
> Report data coming to HAL is in ascii format, HAL sends
> data in hex to daemon, so convert to binary.
> ---
>  android/hidhost.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/android/hidhost.c b/android/hidhost.c
> index c7b4114..9b4bb15 100644
> --- a/android/hidhost.c
> +++ b/android/hidhost.c
> @@ -98,6 +98,14 @@ struct hid_device {
>  	uint8_t		last_hid_msg;
>  };
>  
> +static void hex2bin(const uint8_t *ascii, int ascii_len, uint8_t *hex)
> +{
> +	int i;
> +
> +	for (i = 0; i < ascii_len / 2; i++)
> +		sscanf((char *) &ascii[i * 2], "%hhx", &hex[i]);
> +}

You're still calling the input parameter ascii and the output parameter
hex. Also, I'm still fine if you just drop this function altogether and
do the conversion inline in the two places that you need it.

>  	if (write(fd, req, req_size) < 0) {
> -		error("error while querying device protocol");
> +		error("error while sending report");

If you're fixing this error, how about fixing it to properly print the
exact error in the same go, i.e. using strerror?

Johan

Re: [PATCH_v3 1/5] android/hid: Fix set seport ipc cmd preparation

[Johan Hedberg]

Hi Ravi,

On Fri, Nov 08, 2013, Ravi kumar Veeramally wrote:
> Now report data is not fixed array. Allocate proper memory
> and send ipc cmd.
> ---
>  android/hal-hidhost.c | 29 ++++++++++++++++++++---------
>  android/hal-msg.h     |  2 +-
>  2 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/android/hal-hidhost.c b/android/hal-hidhost.c
> index 34f9f77..ce3dcd7 100644
> --- a/android/hal-hidhost.c
> +++ b/android/hal-hidhost.c
> @@ -18,6 +18,7 @@
>  #include <stdbool.h>
>  #include <stddef.h>
>  #include <string.h>
> +#include <stdlib.h>
>  
>  #include "hal-log.h"
>  #include "hal.h"
> @@ -297,7 +298,8 @@ static bt_status_t set_report(bt_bdaddr_t *bd_addr,
>  						bthh_report_type_t report_type,
>  						char *report)
>  {
> -	struct hal_cmd_hidhost_set_report cmd;
> +	struct hal_cmd_hidhost_set_report *cmd;
> +	int cmd_len, status;

The return type of this function is bt_status_t, not int (even though
the two are in practice compatible).

Are you sure you don't want to keep using a stack variable? You could
potentially just define a buffer with the max mtu size and use that.

Johan

Re: [PATCH_v3 2/5] android/hid: Fix set report data format in daemon

[Ravi Kumar Veeramally]

Hi Johan,

On 11/08/2013 01:05 PM, Johan Hedberg wrote:
> Hi Ravi,
>
> On Fri, Nov 08, 2013, Ravi kumar Veeramally wrote:
>> Report data coming to HAL is in ascii format, HAL sends
>> data in hex to daemon, so convert to binary.
>> ---
>>   android/hidhost.c | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/android/hidhost.c b/android/hidhost.c
>> index c7b4114..9b4bb15 100644
>> --- a/android/hidhost.c
>> +++ b/android/hidhost.c
>> @@ -98,6 +98,14 @@ struct hid_device {
>>   	uint8_t		last_hid_msg;
>>   };
>>   
>> +static void hex2bin(const uint8_t *ascii, int ascii_len, uint8_t *hex)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < ascii_len / 2; i++)
>> +		sscanf((char *) &ascii[i * 2], "%hhx", &hex[i]);
>> +}
> You're still calling the input parameter ascii and the output parameter
> hex. Also, I'm still fine if you just drop this function altogether and
> do the conversion inline in the two places that you need it.
   Ok.
>>   	if (write(fd, req, req_size) < 0) {
>> -		error("error while querying device protocol");
>> +		error("error while sending report");
> If you're fixing this error, how about fixing it to properly print the
> exact error in the same go, i.e. using strerror?
>
  Ok, I will send this in another patch, have to fix other debugs also.

Thanks,
Ravi.

Re: [PATCH_v3 1/5] android/hid: Fix set seport ipc cmd preparation

[Ravi Kumar Veeramally]

Hi Johan,

On 11/08/2013 01:07 PM, Johan Hedberg wrote:
> Hi Ravi,
>
> On Fri, Nov 08, 2013, Ravi kumar Veeramally wrote:
>> Now report data is not fixed array. Allocate proper memory
>> and send ipc cmd.
>> ---
>>   android/hal-hidhost.c | 29 ++++++++++++++++++++---------
>>   android/hal-msg.h     |  2 +-
>>   2 files changed, 21 insertions(+), 10 deletions(-)
>>
>> diff --git a/android/hal-hidhost.c b/android/hal-hidhost.c
>> index 34f9f77..ce3dcd7 100644
>> --- a/android/hal-hidhost.c
>> +++ b/android/hal-hidhost.c
>> @@ -18,6 +18,7 @@
>>   #include <stdbool.h>
>>   #include <stddef.h>
>>   #include <string.h>
>> +#include <stdlib.h>
>>   
>>   #include "hal-log.h"
>>   #include "hal.h"
>> @@ -297,7 +298,8 @@ static bt_status_t set_report(bt_bdaddr_t *bd_addr,
>>   						bthh_report_type_t report_type,
>>   						char *report)
>>   {
>> -	struct hal_cmd_hidhost_set_report cmd;
>> +	struct hal_cmd_hidhost_set_report *cmd;
>> +	int cmd_len, status;
> The return type of this function is bt_status_t, not int (even though
> the two are in practice compatible).
>
> Are you sure you don't want to keep using a stack variable? You could
> potentially just define a buffer with the max mtu size and use that.
>
> Johan
Ahh , I noticed your commit  49f051b6b62 removing
unnecessary allocations. I will fix that.

  Thanks,
  Ravi.