Fixes to report size calculation and added tests

Author: Stephen

CMakeLists.txt

@@ -70,8 +70,8 @@ if(HIDAPI_ENABLE_ASAN)
     endif()
 endif()
 
-if(WIN32)
-    # so far only Windows has tests
+if(WIN32 OR HIDAPI_WITH_LIBUSB)
+    # so far only Windows and LibUSB have tests
     option(HIDAPI_WITH_TESTS "Build HIDAPI (unit-)tests" ${IS_DEBUG_BUILD})
 else()
     set(HIDAPI_WITH_TESTS OFF)

libusb/CMakeLists.txt

@@ -105,3 +105,7 @@ if(HIDAPI_INSTALL_TARGETS)
 endif()
 
 hidapi_configure_pc("${PROJECT_ROOT}/pc/hidapi-libusb.pc.in")
+
+if(HIDAPI_WITH_TESTS)
+     add_subdirectory(test)
+endif()

libusb/hid.c

@@ -279,27 +279,18 @@ static int get_usage(uint8_t *report_descriptor, size_t size,
 	return -1; /* failure */
 }
 
-/* Retrieves the largest input report size (in bytes) from the report descriptor.
-
-   Requires an opened device with *claimed interface*.
-
+/* Retrieves the largest input report size (in bytes) from the passed in report descriptor.
    The return value is the size on success and -1 on failure. */
-static size_t get_max_input_report_size(libusb_device_handle *handle, int interface_num, uint16_t expected_report_descriptor_size)
+static size_t get_max_input_report_size(uint8_t * report_descriptor, int desc_size)
 {
 	int i = 0;
 	int size_code;
 	int data_len, key_size;
 
-	int64_t report_size = 0, report_count = 0;
+	int64_t report_size = -1, report_count = -1;
+	ssize_t cur_size = 0;
 	ssize_t max_size = -1;
 
-	unsigned char report_descriptor[HID_API_MAX_REPORT_DESCRIPTOR_SIZE];
-
-	int desc_size = hid_get_report_descriptor_libusb(handle, interface_num, expected_report_descriptor_size, report_descriptor, sizeof(report_descriptor));
-	if (desc_size < 0) {
-		return -1;
-	}
-
 	while (i < desc_size) {
 		int key = report_descriptor[i];
 		int key_cmd = key & 0xfc;
@@ -325,26 +316,41 @@ static size_t get_max_input_report_size(libusb_device_handle *handle, int interf
 			key_size = 1;
 		}
 
-		if (key_cmd == 0x94) {
+		if (key_cmd == 0x94) { /* Report Count */
 			report_count = get_bytes(report_descriptor, desc_size, data_len, i);
 		}
-		if (key_cmd == 0x74) {
+		if (key_cmd == 0x74) { /* Report Size */
 			report_size = get_bytes(report_descriptor, desc_size, data_len, i);
 		}
-		if (key_cmd == 0x80) { // Input
-			/* report_size is in bits. Determine the total size (count * size),
-			   convert to bytes (rounded up), and add one byte for the report
-			   number. */
-			ssize_t size = (((report_count * report_size) + 7) / 8) + 1;
-			if (size > max_size)
-				max_size = size;
+		if (key_cmd == 0x80) { /* Input */
+			if (report_count < 0 || report_size < 0) {
+				/* We are missing size or count. That isn't good. */
+				return 0;
+			}
+			cur_size += (report_count * report_size);
+		}
+		if (key_cmd == 0x84) { /* Report ID */
+			if (cur_size > max_size) {
+				max_size = cur_size;
+			}
+			cur_size = 0;
 		}
 
 		/* Skip over this key and it's associated data */
 		i += data_len + key_size;
 	}
 
-	return max_size;
+	if (cur_size > max_size) {
+		max_size = cur_size;
+	}
+
+	if (max_size < 0) {
+		return -1;
+	}
+
+	/* report_size is in bits. Determine the total size convert to bytes
+	   (rounded up), and add one byte for the report number. */
+	return ((max_size + 7) / 8) + 1;
 }
 
 #if defined(__FreeBSD__) && __FreeBSD__ < 10
@@ -1294,7 +1300,15 @@ static int hidapi_initialize_device(hid_device *dev, const struct libusb_interfa
 	dev->interface = intf_desc->bInterfaceNumber;
 
 	dev->report_descriptor_size = get_report_descriptor_size_from_interface_descriptors(intf_desc);
-	dev->max_input_report_size = get_max_input_report_size(dev->device_handle, dev->interface, dev->report_descriptor_size);
+
+	unsigned char report_descriptor[HID_API_MAX_REPORT_DESCRIPTOR_SIZE];
+
+	int desc_size = hid_get_report_descriptor_libusb(dev->device_handle, dev->interface, dev->report_descriptor_size, report_descriptor, sizeof(report_descriptor));
+	if (desc_size > 0) {
+		dev->max_input_report_size = get_max_input_report_size(report_descriptor, desc_size);
+	} else {
+		dev->max_input_report_size = -1;
+	}
 
 	dev->input_endpoint = 0;
 	dev->input_ep_max_packet_size = 0;

libusb/test/CMakeLists.txt

@@ -0,0 +1,69 @@
+add_executable(max_input_report_size_test max_input_report_size_test.c)
+set_target_properties(max_input_report_size_test
+    PROPERTIES
+        C_STANDARD 11
+        C_STANDARD_REQUIRED TRUE
+)
+
+if(TARGET usb-1.0)
+    target_link_libraries(max_input_report_size_test PRIVATE usb-1.0)
+else()
+    include(FindPkgConfig)
+    pkg_check_modules(libusb REQUIRED IMPORTED_TARGET libusb-1.0>=1.0.9)
+    target_link_libraries(max_input_report_size_test PRIVATE PkgConfig::libusb)
+endif()
+
+target_link_libraries(max_input_report_size_test PUBLIC hidapi_include)
+
+# Each test case requires 2 files:
+# <name>.pp_data - textual representation of HIDP_PREPARSED_DATA;
+# <name>_expected.rpt_desc - reconstructed HID Report Descriptor out of .pp_data file;
+#
+# (Non-required by test):
+# <name>_real.dpt_desc - the original report rescriptor used to create a test case;
+set(HID_DESCRIPTOR_RECONSTRUCT_TEST_CASES
+     046D_C52F_0001_000C
+     046D_C52F_0001_FF00
+     046D_C52F_0002_0001
+     046D_C52F_0002_FF00
+     17CC_1130_0000_FF01
+     046D_0A37_0001_000C
+     046A_0011_0006_0001
+     046D_C077_0002_0001
+     046D_C283_0004_0001
+     046D_B010_0006_0001
+     046D_B010_0002_FF00
+     046D_B010_0002_0001
+     046D_B010_0001_FF00
+     046D_B010_0001_000C
+     046D_C534_0001_000C
+     046D_C534_0001_FF00
+     046D_C534_0002_0001
+     046D_C534_0002_FF00
+     046D_C534_0006_0001
+     046D_C534_0080_0001
+     047F_C056_0001_000C
+     047F_C056_0003_FFA0
+     047F_C056_0005_000B
+     045E_02FF_0005_0001
+     1532_00A3_0002_0001
+)
+
+set(CMAKE_VERSION_SUPPORTS_ENVIRONMENT_MODIFICATION "3.22")
+
+foreach(TEST_CASE ${HID_DESCRIPTOR_RECONSTRUCT_TEST_CASES})
+     set(TEST_PP_DATA "${CMAKE_CURRENT_LIST_DIR}/../../windows/test/data/${TEST_CASE}.pp_data")
+     if(NOT EXISTS "${TEST_PP_DATA}")
+          message(FATAL_ERROR "Missing '${TEST_PP_DATA}' file for '${TEST_CASE}' test case")
+     endif()
+     set(TEST_EXPECTED_DESCRIPTOR "${CMAKE_CURRENT_LIST_DIR}/../../windows/test/data/${TEST_CASE}_expected.rpt_desc")
+     if(NOT EXISTS "${TEST_EXPECTED_DESCRIPTOR}")
+          message(FATAL_ERROR "Missing '${TEST_EXPECTED_DESCRIPTOR}' file for '${TEST_CASE}' test case")
+     endif()
+
+     add_test(NAME "LibUsbHidMaxInputReportSizeTest_${TEST_CASE}"
+          COMMAND max_input_report_size_test "${TEST_PP_DATA}" "${TEST_EXPECTED_DESCRIPTOR}"
+          WORKING_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}"
+          #WORKING_DIRECTORY "$<TARGET_FILE_DIR:hidapi_winapi>"
+     )
+endforeach()

libusb/test/max_input_report_size_test.c

@@ -0,0 +1,115 @@
+#include <stddef.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <string.h>
+#include <errno.h>
+
+#include "../hid.c"
+
+static ssize_t parse_max_input_report_size(const char * filename)
+{
+	FILE* file = fopen(filename, "r");
+	if (file == NULL) {
+		fprintf(stderr, "ERROR: Couldn't open file '%s' for reading: %s\n", filename, strerror(errno));
+		return -1;
+	}
+
+	char line[256];
+	{
+		while (fgets(line, sizeof(line), file) != NULL) {
+			unsigned short temp_ushort;
+			if (sscanf(line, "pp_data->caps_info[0]->ReportByteLength   = %hu\n", &temp_ushort) == 1) {
+				fclose(file);
+				return (ssize_t)temp_ushort;
+			}
+		}
+	}
+
+	fprintf(stderr, "Unable to find pp_data->caps_info[0]->ReportByteLength in %s\n", filename);
+	fclose(file);
+
+	return -1;
+}
+
+static bool read_hex_data_from_text_file(const char *filename, unsigned char *data_out, size_t data_size, size_t *actual_read)
+{
+	size_t read_index = 0;
+	FILE* file = fopen(filename, "r");
+	if (file == NULL) {
+		fprintf(stderr, "ERROR: Couldn't open file '%s' for reading: %s\n", filename, strerror(errno));
+		return false;
+	}
+
+	bool result = true;
+	unsigned int val;
+	char buf[16];
+	while (fscanf(file, "%15s", buf) == 1) {
+		if (sscanf(buf, "0x%X", &val) != 1) {
+			fprintf(stderr, "Invalid HEX text ('%s') file, got %s\n", filename, buf);
+			result = false;
+			goto end;
+		}
+
+		if (read_index >= data_size) {
+			fprintf(stderr, "Buffer for file read is too small. Got only %zu bytes to read '%s'\n", data_size, filename);
+			result = false;
+			goto end;
+		}
+
+		if (val > (unsigned char)-1) {
+			fprintf(stderr, "Invalid HEX text ('%s') file, got a value of: %u\n", filename, val);
+			result = false;
+			goto end;
+		}
+
+		data_out[read_index] = (unsigned char) val;
+
+		read_index++;
+	}
+
+	if (!feof(file)) {
+		fprintf(stderr, "Invalid HEX text ('%s') file - failed to read all values\n", filename);
+		result = false;
+		goto end;
+	}
+
+	*actual_read = read_index;
+
+end:
+	fclose(file);
+	return result;
+}
+
+
+int main(int argc, char* argv[])
+{
+	if (argc != 3) {
+		fprintf(stderr, "Expected 2 arguments for the test ('<>.pp_data' and '<>_expected.rpt_desc'), got: %d\n", argc - 1);
+		return EXIT_FAILURE;
+	}
+
+	printf("Checking: '%s' / '%s'\n", argv[1], argv[2]);
+
+	unsigned char report_descriptor[HID_API_MAX_REPORT_DESCRIPTOR_SIZE];
+	size_t report_descriptor_size = 0;
+	if (!read_hex_data_from_text_file(argv[2], report_descriptor, sizeof(report_descriptor), &report_descriptor_size)) {
+		return EXIT_FAILURE;
+	}
+
+	ssize_t expected = parse_max_input_report_size(argv[1]);
+	if (expected < 0) {
+		fprintf(stderr, "Unable to expected max input report size from %s\n", argv[1]);
+		return EXIT_FAILURE;
+	}
+
+	ssize_t res = (ssize_t)get_max_input_report_size(report_descriptor, report_descriptor_size);
+
+	if (res != expected) {
+		fprintf(stderr, "Failed to properly compute size. Got %zd, expected %zd\n", res, expected);
+		return EXIT_FAILURE;
+	} else {
+		printf("Properly computed size: %zd\n", res);
+		return EXIT_SUCCESS;
+	}
+}