hidpp: refactor to introduce structure in protocol

Before this patch, there was no structure at all in the messages that
were passed around. Some issues:

 - (debug) Every message of length 7 was considered a request (and
   length 20 were seen as responses). This is not the case for HID++
   1.0.
 - The length of the message payload (ignoring the header) is fixed to 7
   or 20, this was not considered in the previous code.
 - The hidpp_device_cmd function contained special-case code for a HID++
   1.0 response on a HID++ 2.0 ping request.

After this patch, the protocol message structure should be more
explicit. To be done:

 - Test for HID++ 1.0 ping response (removed/broken by this patch).
 - Validate responses, retry read if needed.

Signed-off-by: Peter Wu <lekensteyn@gmail.com>

1 files changed, 154 insertions, 157 deletions

diff --git a/src/linux/hidpp-device.c b/src/linux/hidpp-device.c
index 1ce48b7..b53348f 100644
--- a/src/linux/hidpp-device.c
+++ b/src/linux/hidpp-device.c
@@ -37,12 +37,6 @@
 
 #define HIDPP_RECEIVER_ADDRESS					0xff
 
-#define HIDPP_RESPONSE_SHORT_LENGTH				7
-#define HIDPP_RESPONSE_LONG_LENGTH				20
-
-#define HIDPP_HEADER_REQUEST					0x10
-#define HIDPP_HEADER_RESPONSE					0x11
-
 /* HID++ 1.0 */
 #define HIDPP_READ_SHORT_REGISTER				0x81
 #define HIDPP_READ_SHORT_REGISTER_BATTERY			0x0d
@@ -108,6 +102,24 @@
 
 #define HIDPP_DEVICE_READ_RESPONSE_TIMEOUT			3000 /* miliseconds */
 
+typedef struct {
+#define HIDPP_MSG_TYPE_SHORT	0x10
+#define HIDPP_MSG_TYPE_LONG	0x11
+#define HIDPP_MSG_LENGTH(msg) ((msg)->type == HIDPP_MSG_TYPE_SHORT ? 7 : 20)
+	guchar			 type;
+	guchar			 device_idx;
+	guchar			 feature_idx;
+	guchar			 function_idx; /* funcId:software_id */
+	union {
+		struct {
+			gchar	 params[3];
+		} s; /* short */
+		struct {
+			gchar	 params[16];
+		} l; /* long */
+	};
+} HidppMessage;
+
 struct HidppDevicePrivate
 {
 	gboolean		 enable_debug;
@@ -197,48 +209,48 @@ hidpp_device_map_get_by_idx (HidppDevice *device, gint idx)
  * Pretty print the send/recieve buffer.
  **/
 static void
-hidpp_device_print_buffer (HidppDevice *device, const guint8 *buffer)
+hidpp_device_print_buffer (HidppDevice *device, const HidppMessage *msg)
 {
 	guint i;
 	const HidppDeviceMap *map;
 
 	if (!device->priv->enable_debug)
 		return;
-	for (i = 0; i < HIDPP_RESPONSE_LONG_LENGTH; i++)
-		g_print ("%02x ", buffer[i]);
+	for (i = 0; i < sizeof (*msg); i++)
+		g_print ("%02x ", ((const guchar*) msg)[i]);
 	g_print ("\n");
 
-	/* direction */
-	if (buffer[0] == HIDPP_HEADER_REQUEST)
-		g_print ("REQUEST\n");
-	else if (buffer[0] == HIDPP_HEADER_RESPONSE)
-		g_print ("RESPONSE\n");
+	/* direction/type */
+	if (msg->type == HIDPP_MSG_TYPE_SHORT)
+		g_print ("REQUEST/SHORT\n");
+	else if (msg->type == HIDPP_MSG_TYPE_LONG)
+		g_print ("RESPONSE/LONG\n");
 	else
 		g_print ("??\n");
 
 	/* dev index */
-	g_print ("device-idx=%02x ", buffer[1]);
-	if (buffer[1] == HIDPP_RECEIVER_ADDRESS) {
+	g_print ("device-idx=%02x ", msg->device_idx);
+	if (msg->device_idx == HIDPP_RECEIVER_ADDRESS) {
 		g_print ("[Receiver]\n");
-	} else if (device->priv->device_idx == buffer[1]) {
+	} else if (device->priv->device_idx == msg->device_idx) {
 		g_print ("[This Device]\n");
 	} else {
 		g_print ("[Random Device]\n");
 	}
 
 	/* feature index */
-	if (buffer[2] == HIDPP_READ_LONG_REGISTER) {
+	if (msg->feature_idx == HIDPP_READ_LONG_REGISTER) {
 		g_print ("feature-idx=%s [%02x]\n",
-			 "v1(ReadLongRegister)", buffer[2]);
+			 "v1(ReadLongRegister)", msg->feature_idx);
 	} else {
-		map = hidpp_device_map_get_by_idx (device, buffer[2]);
+		map = hidpp_device_map_get_by_idx (device, msg->feature_idx);
 		g_print ("feature-idx=v2(%s) [%02x]\n",
-			 map != NULL ? map->name : "unknown", buffer[2]);
+			 map != NULL ? map->name : "unknown", msg->feature_idx);
 	}
 
-	g_print ("function-id=%01x\n", buffer[3] & 0xf);
-	g_print ("software-id=%01x\n", buffer[3] >> 4);
-	g_print ("param[0]=%02x\n\n", buffer[4]);
+	g_print ("function-id=%01x\n", msg->function_idx & 0xf);
+	g_print ("software-id=%01x\n", msg->function_idx >> 4);
+	g_print ("param[0]=%02x\n\n", msg->s.params[0]);
 }
 
 /**
@@ -246,19 +258,14 @@ hidpp_device_print_buffer (HidppDevice *device, const guint8 *buffer)
  **/
 static gboolean
 hidpp_device_cmd (HidppDevice	*device,
-		  guint8	 device_idx,
-		  guint8	 feature_idx,
-		  guint8	 function_idx,
-		  guint8	*request_data,
-		  gsize		 request_len,
-		  guint8	*response_data,
-		  gsize		 response_len,
+		  const HidppMessage	*request,
+		  HidppMessage	*response,
 		  GError	**error)
 {
 	gboolean ret = TRUE;
 	gssize wrote;
-	guint8 buf[HIDPP_RESPONSE_LONG_LENGTH];
-	guint i;
+	HidppMessage read_msg = { 0 };
+	guint msg_len;
 	HidppDevicePrivate *priv = device->priv;
 	GPollFD poll[] = {
 		{
@@ -267,19 +274,16 @@ hidpp_device_cmd (HidppDevice	*device,
 		},
 	};
 
-	/* make the request packet */
-	memset (buf, 0x00, HIDPP_RESPONSE_LONG_LENGTH);
-	buf[0] = HIDPP_HEADER_REQUEST;
-	buf[1] = device_idx;
-	buf[2] = feature_idx;
-	buf[3] = function_idx;
-	for (i = 0; i < request_len; i++)
-		buf[4 + i] = request_data[i];
+	g_assert (request->type == HIDPP_MSG_TYPE_SHORT ||
+			request->type == HIDPP_MSG_TYPE_LONG);
+
+	hidpp_device_print_buffer (device, request);
+
+	msg_len = HIDPP_MSG_LENGTH(request);
 
 	/* write to the device */
-	hidpp_device_print_buffer (device, buf);
-	wrote = write (priv->fd, buf, 4 + request_len);
-	if ((gsize) wrote != 4 + request_len) {
+	wrote = write (priv->fd, (const char *)request, msg_len);
+	if ((gsize) wrote != msg_len) {
 		g_set_error (error, 1, 0,
 			     "Unable to write request to device: %" G_GSIZE_FORMAT,
 			     wrote);
@@ -288,6 +292,7 @@ hidpp_device_cmd (HidppDevice	*device,
 	}
 
 	/* read from the device */
+	// TODO: keep reading until wanted message is found
 	wrote = g_poll (poll, G_N_ELEMENTS(poll),
 		        HIDPP_DEVICE_READ_RESPONSE_TIMEOUT);
 	if (wrote <= 0) {
@@ -297,8 +302,8 @@ hidpp_device_cmd (HidppDevice	*device,
 		ret = FALSE;
 		goto out;
 	}
-	memset (buf, 0x00, HIDPP_RESPONSE_LONG_LENGTH);
-	wrote = read (priv->fd, buf, sizeof (buf));
+
+	wrote = read (priv->fd, &read_msg, sizeof (*response));
 	if (wrote <= 0) {
 		g_set_error (error, 1, 0,
 			     "Unable to read response from device: %" G_GSIZE_FORMAT,
@@ -307,20 +312,10 @@ hidpp_device_cmd (HidppDevice	*device,
 		goto out;
 	}
 
-	/* is device offline */
-	hidpp_device_print_buffer (device, buf);
-	if (buf[0] == HIDPP_HEADER_REQUEST &&
-	    buf[1] == device_idx &&
-	    buf[2] == HIDPP_ERR_INVALID_SUBID &&
-	    buf[3] == 0x00 &&
-	    buf[4] == HIDPP_FEATURE_ROOT_FN_PING) {
-		/* HID++ 1.0 ping reply, so fake success with version 1  */
-		if (priv->version < 2 && (buf[5] == HIDPP_ERROR_CODE_UNKNOWN
-					|| buf[5] == HIDPP_ERROR_CODE_UNSUPPORTED)) {
-			response_data[0] = 1;
-			goto out;
-		}
-	}
+	hidpp_device_print_buffer (device, response);
+
+	// TODO: test for invalid reply
+#if 0
 	if ((buf[0] != HIDPP_HEADER_REQUEST && buf[0] != HIDPP_HEADER_RESPONSE) ||
 	    buf[1] != device_idx ||
 	    buf[2] != feature_idx ||
@@ -331,9 +326,10 @@ hidpp_device_cmd (HidppDevice	*device,
 		ret = FALSE;
 		goto out;
 	}
-	for (i = 0; i < response_len; i++)
-		response_data[i] = buf[4 + i];
+#endif
 out:
+	/* allow caller to check for protocol errors */
+	memcpy (response, &read_msg, sizeof (read_msg));
 	return ret;
 }
 
@@ -350,21 +346,21 @@ hidpp_device_map_add (HidppDevice *device,
 {
 	gboolean ret;
 	GError *error = NULL;
-	guint8 buf[3];
+	HidppMessage msg;
 	HidppDeviceMap *map;
 	HidppDevicePrivate *priv = device->priv;
 
-	buf[0] = feature >> 8;
-	buf[1] = feature;
-	buf[2] = 0x00;
+	msg.type = HIDPP_MSG_TYPE_SHORT;
+	msg.device_idx = priv->device_idx;
+	msg.feature_idx = HIDPP_FEATURE_ROOT_INDEX;
+	msg.function_idx = HIDPP_FEATURE_ROOT_FN_GET_FEATURE;
+	msg.s.params[0] = feature >> 8;
+	msg.s.params[1] = feature;
+	msg.s.params[2] = 0x00;
 
 	g_debug ("Getting idx for feature %s [%02x]", name, feature);
 	ret = hidpp_device_cmd (device,
-				priv->device_idx,
-				HIDPP_FEATURE_ROOT_INDEX,
-				HIDPP_FEATURE_ROOT_FN_GET_FEATURE,
-				buf, sizeof (buf),
-				buf, sizeof (buf),
+				&msg, &msg,
 				&error);
 	if (!ret) {
 		g_warning ("Failed to get feature idx: %s", error->message);
@@ -373,7 +369,7 @@ hidpp_device_map_add (HidppDevice *device,
 	}
 
 	/* zero index */
-	if (buf[0] == 0x00) {
+	if (msg.s.params[0] == 0x00) {
 		ret = FALSE;
 		g_debug ("Feature not found");
 		goto out;
@@ -381,7 +377,7 @@ hidpp_device_map_add (HidppDevice *device,
 
 	/* add to map */
 	map = g_new0 (HidppDeviceMap, 1);
-	map->idx = buf[0];
+	map->idx = msg.s.params[0];
 	map->feature = feature;
 	map->name = g_strdup (name);
 	g_ptr_array_add (priv->feature_index, map);
@@ -485,7 +481,7 @@ hidpp_device_refresh (HidppDevice *device,
 	const HidppDeviceMap *map;
 	gboolean ret = TRUE;
 	GString *name = NULL;
-	guint8 buf[HIDPP_RESPONSE_LONG_LENGTH];
+	HidppMessage msg;
 	guint i;
 	guint len;
 	HidppDevicePrivate *priv = device->priv;
@@ -508,20 +504,21 @@ hidpp_device_refresh (HidppDevice *device,
 	if ((refresh_flags & HIDPP_REFRESH_FLAGS_VERSION) > 0) {
 		guint version_old = priv->version;
 
-		buf[0] = 0x00;
-		buf[1] = 0x00;
-		buf[2] = HIDPP_PING_DATA;
+		msg.type = HIDPP_MSG_TYPE_SHORT;
+		msg.device_idx = priv->device_idx;
+		msg.feature_idx = HIDPP_FEATURE_ROOT_INDEX;
+		msg.function_idx = HIDPP_FEATURE_ROOT_FN_PING;
+		msg.s.params[0] = 0x00;
+		msg.s.params[1] = 0x00;
+		msg.s.params[2] = HIDPP_PING_DATA;
 		ret = hidpp_device_cmd (device,
-					priv->device_idx,
-					HIDPP_FEATURE_ROOT_INDEX,
-					HIDPP_FEATURE_ROOT_FN_PING,
-					buf, 3,
-					buf, 4,
+					&msg, &msg,
 					error);
+		// TODO: on failure, test if hid error
 		if (!ret)
 			goto out;
 
-		priv->version = buf[0];
+		priv->version = msg.s.params[0];
 
 		if (version_old != priv->version)
 			g_debug("protocol for hid++ device changed from v%d to v%d",
@@ -559,19 +556,19 @@ hidpp_device_refresh (HidppDevice *device,
 	if ((refresh_flags & HIDPP_REFRESH_FLAGS_KIND) > 0) {
 
 		if (priv->version == 1) {
-			buf[0] = 0x20 | (priv->device_idx - 1);
-			buf[1] = 0x00;
-			buf[2] = 0x00;
+			msg.type = HIDPP_MSG_TYPE_SHORT;
+			msg.device_idx = HIDPP_RECEIVER_ADDRESS;
+			msg.feature_idx = HIDPP_READ_LONG_REGISTER;
+			msg.function_idx = 0xb5;
+			msg.s.params[0] = 0x20 | (priv->device_idx - 1);
+			msg.s.params[1] = 0x00;
+			msg.s.params[2] = 0x00;
 			ret = hidpp_device_cmd (device,
-						HIDPP_RECEIVER_ADDRESS,
-						HIDPP_READ_LONG_REGISTER,
-						0xb5,
-						buf, 3,
-						buf, 8,
+						&msg, &msg,
 						error);
 			if (!ret)
 				goto out;
-			switch (buf[7]) {
+			switch (msg.l.params[7]) {
 			case HIDPP_READ_LONG_REGISTER_DEVICE_TYPE_KEYBOARD:
 			case HIDPP_READ_LONG_REGISTER_DEVICE_TYPE_NUMPAD:
 			case HIDPP_READ_LONG_REGISTER_DEVICE_TYPE_REMOTE_CONTROL:
@@ -597,19 +594,19 @@ hidpp_device_refresh (HidppDevice *device,
 			/* send a BatteryLevelStatus report */
 			map = hidpp_device_map_get_by_feature (device, HIDPP_FEATURE_GET_DEVICE_NAME_TYPE);
 			if (map != NULL) {
-				buf[0] = 0x00;
-				buf[1] = 0x00;
-				buf[2] = 0x00;
+				msg.type = HIDPP_MSG_TYPE_SHORT;
+				msg.device_idx = priv->device_idx;
+				msg.feature_idx = map->idx;
+				msg.function_idx = HIDPP_FEATURE_GET_DEVICE_NAME_TYPE_FN_GET_TYPE;
+				msg.s.params[0] = 0x00;
+				msg.s.params[1] = 0x00;
+				msg.s.params[2] = 0x00;
 				ret = hidpp_device_cmd (device,
-							priv->device_idx,
-							map->idx,
-							HIDPP_FEATURE_GET_DEVICE_NAME_TYPE_FN_GET_TYPE,
-							buf, 3,
-							buf, 1,
+							&msg, &msg,
 							error);
 				if (!ret)
 					goto out;
-				switch (buf[0]) {
+				switch (msg.s.params[0]) {
 				case 0: /* keyboard */
 				case 2: /* numpad */
 					priv->kind = HIDPP_DEVICE_KIND_KEYBOARD;
@@ -632,56 +629,56 @@ hidpp_device_refresh (HidppDevice *device,
 	/* get device model string */
 	if ((refresh_flags & HIDPP_REFRESH_FLAGS_MODEL) > 0) {
 		if (priv->version == 1) {
-			buf[0] = 0x40 | (priv->device_idx - 1);
-			buf[1] = 0x00;
-			buf[2] = 0x00;
+			msg.type = HIDPP_MSG_TYPE_SHORT;
+			msg.device_idx = HIDPP_RECEIVER_ADDRESS;
+			msg.feature_idx = HIDPP_READ_LONG_REGISTER;
+			msg.function_idx = 0xb5;
+			msg.s.params[0] = 0x40 | (priv->device_idx - 1);
+			msg.s.params[1] = 0x00;
+			msg.s.params[2] = 0x00;
 
 			ret = hidpp_device_cmd (device,
-					HIDPP_RECEIVER_ADDRESS,
-					HIDPP_READ_LONG_REGISTER,
-					0xb5,
-					buf, 3,
-					buf, HIDPP_RESPONSE_LONG_LENGTH,
+					&msg, &msg,
 					error);
 			if (!ret)
 				goto out;
 
-			len = buf[1];
+			len = msg.l.params[1];
 			name = g_string_new ("");
-			g_string_append_len (name, (gchar *) buf+2, len);
+			g_string_append_len (name, msg.l.params + 2, len);
 			priv->model = g_strdup (name->str);
 		} else if (priv->version == 2) {
-			buf[0] = 0x00;
-			buf[1] = 0x00;
-			buf[2] = 0x00;
+			msg.type = HIDPP_MSG_TYPE_SHORT;
+			msg.device_idx = priv->device_idx;
+			msg.feature_idx = map->idx;
+			msg.function_idx = HIDPP_FEATURE_GET_DEVICE_NAME_TYPE_FN_GET_COUNT;
+			msg.s.params[0] = 0x00;
+			msg.s.params[1] = 0x00;
+			msg.s.params[2] = 0x00;
 			map = hidpp_device_map_get_by_feature (device, HIDPP_FEATURE_GET_DEVICE_NAME_TYPE);
 			if (map != NULL) {
 				ret = hidpp_device_cmd (device,
-						priv->device_idx,
-						map->idx,
-						HIDPP_FEATURE_GET_DEVICE_NAME_TYPE_FN_GET_COUNT,
-						buf, 3,
-						buf, 1,
+						&msg, &msg,
 						error);
 				if (!ret)
 					goto out;
 			}
-			len = buf[0];
+			len = msg.s.params[0];
 			name = g_string_new ("");
 			for (i = 0; i < len; i +=4 ) {
-				buf[0] = i;
-				buf[1] = 0x00;
-				buf[2] = 0x00;
+				msg.type = HIDPP_MSG_TYPE_SHORT;
+				msg.device_idx = priv->device_idx;
+				msg.feature_idx = map->idx;
+				msg.function_idx = HIDPP_FEATURE_GET_DEVICE_NAME_TYPE_FN_GET_NAME;
+				msg.s.params[0] = i;
+				msg.s.params[1] = 0x00;
+				msg.s.params[2] = 0x00;
 				ret = hidpp_device_cmd (device,
-						priv->device_idx,
-						map->idx,
-						HIDPP_FEATURE_GET_DEVICE_NAME_TYPE_FN_GET_NAME,
-						buf, 3,
-						buf, 4,
+						&msg, &msg,
 						error);
 				if (!ret)
 					goto out;
-				g_string_append_len (name, (gchar *) &buf[0], 4);
+				g_string_append_len (name, msg.s.params, 4);
 			}
 			priv->model = g_strdup (name->str);
 		}
@@ -690,59 +687,59 @@ hidpp_device_refresh (HidppDevice *device,
 	/* get battery status */
 	if ((refresh_flags & HIDPP_REFRESH_FLAGS_BATTERY) > 0) {
 		if (priv->version == 1) {
-			buf[0] = 0x00;
-			buf[1] = 0x00;
-			buf[2] = 0x00;
+			msg.type = HIDPP_MSG_TYPE_SHORT;
+			msg.device_idx = priv->device_idx;
+			msg.feature_idx = HIDPP_READ_SHORT_REGISTER;
+			msg.function_idx = HIDPP_READ_SHORT_REGISTER_BATTERY;
+			msg.s.params[0] = 0x00;
+			msg.s.params[1] = 0x00;
+			msg.s.params[2] = 0x00;
 			ret = hidpp_device_cmd (device,
-						priv->device_idx,
-						HIDPP_READ_SHORT_REGISTER,
-						HIDPP_READ_SHORT_REGISTER_BATTERY,
-						buf, 3,
-						buf, 1,
+						&msg, &msg,
 						error);
 			if (!ret)
 				goto out;
-			priv->batt_percentage = buf[0];
+			priv->batt_percentage = msg.s.params[0];
 			priv->batt_status = HIDPP_DEVICE_BATT_STATUS_DISCHARGING;
 		} else if (priv->version == 2) {
 
 			/* sent a SetLightMeasure report */
 			map = hidpp_device_map_get_by_feature (device, HIDPP_FEATURE_SOLAR_DASHBOARD);
 			if (map != NULL) {
-				buf[0] = 0x01; /* Max number of reports: number of report sent after function call */
-				buf[1] = 0x01; /* Report period: time between reports, in seconds */
+				msg.type = HIDPP_MSG_TYPE_SHORT;
+				msg.device_idx = priv->device_idx;
+				msg.feature_idx = map->idx;
+				msg.function_idx = HIDPP_FEATURE_SOLAR_DASHBOARD_FN_SET_LIGHT_MEASURE;
+				msg.s.params[0] = 0x01; /* Max number of reports: number of report sent after function call */
+				msg.s.params[1] = 0x01; /* Report period: time between reports, in seconds */
 				ret = hidpp_device_cmd (device,
-							priv->device_idx,
-							map->idx,
-							HIDPP_FEATURE_SOLAR_DASHBOARD_FN_SET_LIGHT_MEASURE,
-							buf, 2,
-							buf, 3,
+							&msg, &msg,
 							error);
 				if (!ret)
 					goto out;
-				priv->batt_percentage = buf[0];
+				priv->batt_percentage = msg.s.params[0];
 				priv->batt_status = HIDPP_DEVICE_BATT_STATUS_DISCHARGING;
 			}
 
 			/* send a BatteryLevelStatus report */
 			map = hidpp_device_map_get_by_feature (device, HIDPP_FEATURE_BATTERY_LEVEL_STATUS);
 			if (map != NULL) {
-				buf[0] = 0x00;
-				buf[1] = 0x00;
-				buf[2] = 0x00;
+				msg.type = HIDPP_MSG_TYPE_SHORT;
+				msg.device_idx = priv->device_idx;
+				msg.feature_idx = map->idx;
+				msg.function_idx = HIDPP_FEATURE_BATTERY_LEVEL_STATUS_FN_GET_STATUS;
+				msg.s.params[0] = 0x00;
+				msg.s.params[1] = 0x00;
+				msg.s.params[2] = 0x00;
 				ret = hidpp_device_cmd (device,
-							priv->device_idx,
-							map->idx,
-							HIDPP_FEATURE_BATTERY_LEVEL_STATUS_FN_GET_STATUS,
-							buf, 3,
-							buf, 3,
+							&msg, &msg,
 							error);
 				if (!ret)
 					goto out;
 
 				/* convert the HID++ v2 status into something
 				 * we can set on the device */
-				switch (buf[2]) {
+				switch (msg.s.params[2]) {
 				case 0: /* discharging */
 					priv->batt_status = HIDPP_DEVICE_BATT_STATUS_DISCHARGING;
 					break;
@@ -757,9 +754,9 @@ hidpp_device_refresh (HidppDevice *device,
 				default:
 					break;
 				}
-				priv->batt_percentage = buf[0];
+				priv->batt_percentage = msg.s.params[0];
 				g_debug ("level=%i%%, next-level=%i%%, battery-status=%i",
-					 buf[0], buf[1], buf[2]);
+					 msg.s.params[0], msg.s.params[1], msg.s.params[2]);
 			}
 		}
 	}