MSC, CDC, HID keyboard definitely working

dhalbert · dhalbert · commit abfb020d417f · 2021-04-29T17:41:43.000-04:00
diff --git a/main.c b/main.c
@@ -170,12 +170,6 @@ STATIC void start_mp(supervisor_allocation* heap) {
     #if CIRCUITPY_NETWORK
     network_module_init();
     #endif
-
-    #if CIRCUITPY_USB_HID
-    if (usb_enabled()) {
-        usb_hid_setup_devices();
-    }
-    #endif
 }
 
 STATIC void stop_mp(void) {
diff --git a/shared-module/usb_cdc/__init__.c b/shared-module/usb_cdc/__init__.c
@@ -224,6 +224,9 @@ bool common_hal_usb_cdc_configure_usb(bool repl_enabled, bool data_enabled) {
         return false;
     }
 
+    // Right now these objects contain no heap objects, but if that changes,
+    // they will need to be protected against gc.
+
     usb_cdc_repl_is_enabled = repl_enabled;
     usb_cdc_set_repl(repl_enabled ? MP_OBJ_FROM_PTR(&usb_cdc_repl_obj) : mp_const_none);
 
diff --git a/shared-module/usb_hid/Device.c b/shared-module/usb_hid/Device.c
@@ -71,6 +71,9 @@ static const uint8_t keyboard_report_descriptor[] = {
 };
 
 const usb_hid_device_obj_t usb_hid_device_keyboard_obj = {
+    .base = {
+        .type = &usb_hid_device_type,
+    },
     .report_descriptor = keyboard_report_descriptor,
     .report_descriptor_length = sizeof(keyboard_report_descriptor),
     .usage_page = 0x01,
@@ -119,6 +122,9 @@ static const uint8_t mouse_report_descriptor[] = {
 };
 
 const usb_hid_device_obj_t usb_hid_device_mouse_obj = {
+    .base = {
+        .type = &usb_hid_device_type,
+    },
     .report_descriptor = mouse_report_descriptor,
     .report_descriptor_length = sizeof(mouse_report_descriptor),
     .usage_page = 0x01,
@@ -145,6 +151,9 @@ static const uint8_t consumer_control_report_descriptor[] = {
 };
 
 const usb_hid_device_obj_t usb_hid_device_consumer_control_obj = {
+    .base = {
+        .type = &usb_hid_device_type,
+    },
     .report_descriptor = consumer_control_report_descriptor,
     .report_descriptor_length = sizeof(consumer_control_report_descriptor),
     .usage_page = 0x0C,
diff --git a/shared-module/usb_hid/__init__.c b/shared-module/usb_hid/__init__.c
@@ -83,6 +83,9 @@ static supervisor_allocation *hid_report_descriptor_allocation;
 static usb_hid_device_obj_t hid_devices[MAX_HID_DEVICES];
 static mp_int_t hid_devices_num;
 
+// This tuple is store in usb_hid.devices.
+static mp_obj_tuple_t *hid_devices_tuple;
+
 static mp_obj_tuple_t default_hid_devices_tuple = {
     .base = {
         .type = &mp_type_tuple,
@@ -132,6 +135,19 @@ size_t usb_hid_add_descriptor(uint8_t *descriptor_buf, uint8_t *current_interfac
     return sizeof(usb_hid_descriptor_template);
 }
 
+// Make up a fresh tuple containing the device objects saved in the static
+// devices table. Save the tuple in usb_hid.devices.
+static void usb_hid_set_devices_from_hid_devices(void) {
+    mp_obj_t tuple_items[hid_devices_num];
+    for (mp_int_t i = 0; i < hid_devices_num; i++) {
+        tuple_items[i] = &hid_devices[i];
+    }
+
+    // Remember tuple for gc purposes.
+    hid_devices_tuple = mp_obj_new_tuple(hid_devices_num, tuple_items);
+    usb_hid_set_devices(hid_devices_tuple);
+}
+
 bool common_hal_usb_hid_configure_usb(const mp_obj_t devices) {
     // We can't change the devices once we're connected.
     if (tud_connected()) {
@@ -151,20 +167,16 @@ bool common_hal_usb_hid_configure_usb(const mp_obj_t devices) {
         memcpy(&hid_devices[i], device, sizeof(usb_hid_device_obj_t));
     }
 
+    usb_hid_set_devices_from_hid_devices();
+
     return true;
 }
 
+// Called when HID devices are ready to be used, when code.py or the REPL starts running.
 void usb_hid_setup_devices(void) {
-    // Make up a fresh tuple containing the device objects are saved in the static
-    // devices table. Save the tuple in usb_hid.devices.
+    usb_hid_set_devices_from_hid_devices();
 
-    mp_obj_t tuple_items[hid_devices_num];
-    for (mp_int_t i = 0; i < hid_devices_num; i++) {
-        tuple_items[i] = &hid_devices[i];
-    }
-    usb_hid_set_devices(mp_obj_new_tuple(hid_devices_num, tuple_items));
-
-    // Create report buffers on the heap.
+   // Create report buffers on the heap.
     for (mp_int_t i = 0; i < hid_devices_num; i++) {
         usb_hid_device_create_report_buffers(&hid_devices[i]);
     }
@@ -174,10 +186,7 @@ void usb_hid_setup_devices(void) {
 size_t usb_hid_report_descriptor_length(void) {
     size_t total_hid_report_descriptor_length = 0;
     for (mp_int_t i = 0; i < hid_devices_num; i++) {
-        // hid_devices has already been validated to contain only usb_hid_device_obj_t objects.
-        usb_hid_device_obj_t *device =
-            MP_OBJ_TO_PTR(mp_obj_subscr(hid_devices, MP_OBJ_NEW_SMALL_INT(i), MP_OBJ_SENTINEL));
-        total_hid_report_descriptor_length += device->report_descriptor_length;
+        total_hid_report_descriptor_length += hid_devices[i].report_descriptor_length;
     }
 
     // Don't need space for a report id if there's only one device.
@@ -205,9 +214,15 @@ void usb_hid_build_report_descriptor(uint8_t* report_descriptor_space, size_t re
             // Copy the whole descriptor and fill in the report id.
             memcpy(report_descriptor_start, device->report_descriptor, device->report_descriptor_length);
             report_descriptor_start[device->report_id_index] = i + 1;
+
+            // Remember the report id that was assigned.
+            device->report_id = i + 1;
+
+            // Advance to the next free chunk for the next report descriptor.x
             report_descriptor_start += device->report_descriptor_length;
         }
         // Clear the heap pointer to the bytes of the descriptor.
+        // We don't need it any more and it will get lost when the heap goes away.
         device->report_descriptor = NULL;
     }
 }
@@ -220,11 +235,14 @@ void usb_hid_save_report_descriptor(uint8_t *report_descriptor_space, size_t rep
     // Copy the descriptor from the temporary area to a supervisor storage allocation that
     // will leave between VM instantiations.
     hid_report_descriptor_allocation =
-        allocate_memory(report_descriptor_length, /*high_address*/ false, /*movable*/ false);
+        allocate_memory(align32_size(report_descriptor_length),
+                        /*high_address*/ false, /*movable*/ false);
     memcpy((uint8_t *) hid_report_descriptor_allocation->ptr, report_descriptor_space, report_descriptor_length);
 }
 
 void usb_hid_gc_collect(void) {
+    gc_collect_ptr(hid_devices_tuple);
+
     // Mark any heap pointers in the static device list as in use.
     for (mp_int_t i = 0; i < hid_devices_num; i++) {
         gc_collect_ptr(&hid_devices[i]);
diff --git a/shared-module/usb_midi/__init__.c b/shared-module/usb_midi/__init__.c
@@ -234,6 +234,9 @@ static const mp_rom_obj_tuple_t midi_ports_tuple = {
 };
 
 void usb_midi_setup_ports(void) {
+    // Right now midi_ports_tuple contains no heap objects, but if it does in the future,
+    // it will need to be protected against gc.
+
     mp_obj_tuple_t *ports = usb_midi_is_enabled ? MP_OBJ_FROM_PTR(&midi_ports_tuple) : mp_const_empty_tuple;
     mp_map_lookup(&usb_midi_module_globals.map, MP_ROM_QSTR(MP_QSTR_ports), MP_MAP_LOOKUP)->value =
         MP_OBJ_FROM_PTR(ports);
diff --git a/supervisor/shared/usb/tusb_config.h b/supervisor/shared/usb/tusb_config.h
@@ -59,14 +59,17 @@ extern "C" {
 // DEVICE CONFIGURATION
 // --------------------------------------------------------------------+
 
+#if USB_HIGHSPEED
 #define CFG_TUSB_RHPORT0_MODE       (OPT_MODE_DEVICE | OPT_MODE_HIGH_SPEED)
+#else
+#define CFG_TUSB_RHPORT0_MODE       (OPT_MODE_DEVICE)
+#endif
 
 // Vendor name included in Inquiry response, max 8 bytes
-#define CFG_TUD_MSC_VENDOR          USB_MANUFACTURER
+#define CFG_TUD_MSC_VENDOR          USB_MANUFACTURER_8
 
 // Product name included in Inquiry response, max 16 bytes
-#define CFG_TUD_MSC_PRODUCT         USB_PRODUCT
-
+#define CFG_TUD_MSC_PRODUCT         USB_PRODUCT_16
 #define CFG_TUD_ENDPOINT0_SIZE       64
 
 // ------------- CLASS -------------//
diff --git a/supervisor/shared/usb/usb_desc.c b/supervisor/shared/usb/usb_desc.c
@@ -63,9 +63,9 @@ static interface_string_t collected_interface_strings[MAX_INTERFACE_STRINGS];
 static size_t collected_interface_strings_length;
 static uint8_t current_interface_string;
 
-supervisor_allocation *device_descriptor_allocation;
-supervisor_allocation *configuration_descriptor_allocation;
-supervisor_allocation *string_descriptors_allocation;
+static supervisor_allocation *device_descriptor_allocation;
+static supervisor_allocation *configuration_descriptor_allocation;
+static supervisor_allocation *string_descriptors_allocation;
 
 static const char manufacturer_name[] = USB_MANUFACTURER;
 static const char product_name[] = USB_PRODUCT;
@@ -114,7 +114,8 @@ static const uint8_t configuration_descriptor_template[] = {
 
 static void usb_build_device_descriptor(uint16_t vid, uint16_t pid) {
     device_descriptor_allocation =
-        allocate_memory(sizeof(device_descriptor_template), /*high_address*/ false, /*movable*/ false);
+        allocate_memory(align32_size(sizeof(device_descriptor_template)),
+                        /*high_address*/ false, /*movable*/ false);
     uint8_t *device_descriptor = (uint8_t *) device_descriptor_allocation->ptr;
     memcpy(device_descriptor, device_descriptor_template, sizeof(device_descriptor_template));
 
@@ -170,7 +171,8 @@ static void usb_build_configuration_descriptor(void) {
 
     // Now we now how big the configuration descriptor will be, so we can allocate space for it.
     configuration_descriptor_allocation =
-        allocate_memory(total_descriptor_length, /*high_address*/ false, /*movable*/ false);
+        allocate_memory(align32_size(total_descriptor_length),
+                        /*high_address*/ false, /*movable*/ false);
     uint8_t *configuration_descriptor = (uint8_t *) configuration_descriptor_allocation->ptr;
 
     // Copy the template, which is the first part of the descriptor, and fix up its length.
@@ -248,7 +250,7 @@ static void usb_build_interface_string_table(void) {
     // Allocate space for the le16 String descriptors.
     // Space needed is 2 bytes for String Descriptor header, then 2 bytes for each character
     string_descriptors_allocation =
-        allocate_memory(current_interface_string * 2 + collected_interface_strings_length * 2,
+        allocate_memory(align32_size(current_interface_string * 2 + collected_interface_strings_length * 2),
                         /*high_address*/ false, /*movable*/ false);
     uint16_t *string_descriptors = (uint16_t *) string_descriptors_allocation->ptr;
 
@@ -309,7 +311,7 @@ void usb_build_descriptors(void) {
 // Invoked when GET DEVICE DESCRIPTOR is received.
 // Application return pointer to descriptor
 uint8_t const *tud_descriptor_device_cb(void) {
-    return (uint8_t *) device_descriptor_allocation;
+    return (uint8_t *) device_descriptor_allocation->ptr;
 }
 
 // Invoked when GET CONFIGURATION DESCRIPTOR is received.
diff --git a/supervisor/supervisor.mk b/supervisor/supervisor.mk
@@ -144,7 +144,13 @@ ifneq ($(USB_VID),)
 CFLAGS += -DUSB_VID=$(USB_VID)
 CFLAGS += -DUSB_PID=$(USB_PID)
 CFLAGS += -DUSB_MANUFACTURER='$(USB_MANUFACTURER)'
+USB_MANUFACTURER_8 := "$(shell echo $(USB_MANUFACTURER) | cut -c 1-8)"
+# Length-limited versions of strings for MSC names.
+CFLAGS += -DUSB_MANUFACTURER_8='$(USB_MANUFACTURER_8)'
+USB_PRODUCT_16 := "$(shell echo $(USB_PRODUCT) | cut -c 1-16)"
+CFLAGS += -DUSB_PRODUCT_16='$(USB_PRODUCT_16)'
 CFLAGS += -DUSB_PRODUCT='$(USB_PRODUCT)'
+
 endif
 
 # In the following URL, don't include the https:// prefix.
@@ -157,6 +163,7 @@ CFLAGS += -DCFG_TUD_CDC=2
 endif
 
 USB_HIGHSPEED ?= 0
+CFLAGS += -DUSB_HIGHSPEED=$(USB_HIGHSPEED)
 
 $(BUILD)/supervisor/shared/translate.o: $(HEADER_BUILD)/qstrdefs.generated.h
 

Original file line number	Diff line number	Diff line change
`@@ -224,6 +224,9 @@ bool common_hal_usb_cdc_configure_usb(bool repl_enabled, bool data_enabled) {`
`224`	`224`	`return false;`
`225`	`225`	`}`
`226`	`226`
	`227`	`+ // Right now these objects contain no heap objects, but if that changes,`
	`228`	`+ // they will need to be protected against gc.`
	`229`	`+`
`227`	`230`	`usb_cdc_repl_is_enabled = repl_enabled;`
`228`	`231`	`usb_cdc_set_repl(repl_enabled ? MP_OBJ_FROM_PTR(&usb_cdc_repl_obj) : mp_const_none);`
`229`	`232`