From 9512883d7e56a9d67ae72f2f6f29da6c89fc898e Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Sat, 9 Nov 2024 13:20:47 -0800 Subject: [PATCH 1/3] Review of code prior to its removal --- src/usb_descriptors.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/src/usb_descriptors.c b/src/usb_descriptors.c index 78c7d8c0..b2cd9e36 100644 --- a/src/usb_descriptors.c +++ b/src/usb_descriptors.c @@ -37,9 +37,41 @@ #define _PID_MAP(itf, n) ((CFG_TUD_##itf) << (n)) //#define USB_PID 0x7332 /*(0x4000 | _PID_MAP(CDC, 0) | _PID_MAP(MSC, 1) | _PID_MAP(HID, 2) | \ _PID_MAP(MIDI, 3) | _PID_MAP(VENDOR, 4) )*/ +/* + * CFG_TUD_xxx is defined as zero if the interface is not enabled. + * CFG_TUD_xxx is otherwise defined as the count of that type of interface. (????) + * + * It therefore appears that the "Auto ProductID layout" is designed ONLY for + * devices that have, at most, a single instance of each of those interface types. + * Because the buspirate device has two CDC interfaces, the above method results + * in an unexpected "auto-PID", and thus should NOT be used: + * + * _PID_MAP(CDC, 0) ==> _PID_MAP(CFG_TUD_CDC, 0) == _PID_MAP(2, 0) == 2 << 0 == 0x02 (!!! FAILS DUE TO MULTIPLE CDC INTERFACES !!!) + * _PID_MAP(MSC, 1) ==> _PID_MAP(CFG_TUD_MSC, 1) == _PID_MAP(1, 1) == 1 << 1 == 0x02 (!!!) + * _PID_MAP(HID, 2) ==> _PID_MAP(CFG_TUD_HID, 2) == _PID_MAP(0, 2) == 0 << 2 == 0x00 + * _PID_MAP(MIDI, 3) ==> _PID_MAP(CFG_TUD_MIDI, 3) == _PID_MAP(0, 3) == 0 << 3 == 0x00 + * + * Specifically, because CFG_TUD_CDC is defined as 2, _PID_MAP(CDC, 0) does not set + * the least significant bit (as expected per the second macro parameter), but instead + * sets the bit that is also used by _PID_MAP(MSC, 1). The resulting USB_PID would + * therefore be: 0x4000 | 0x02 | 0x02 | 0x00 | 0x00 == 0x4002. Decoding the bits + * would indicate that the device has a single MSC interface only (no CDC). + * + * I have no idea where this "Auto ProductID layout" came from, but the macros + * are not defined to fail gracefully when multiple instances of an interface + * are present. Moreover, at least for VID 0x1209, the above mapping would overlap + * with already-assigned VID/PID combinations. Therefore, this appears to not only + * be dead code, but also dangerously incorrect code. + * + * BusPirate has obtained two VID/PID combinations from https://pid.codes/. + * 0x1209 / 0x7331 - https://pid.codes/1209/7331/ - "Bus Pirate Next Gen CDC" + * 0x1209 / 0x7332 - https://pid.codes/1209/7332/ - "Bus Pirate Next Gen Logic Analyzer" + * + * The code is now hard-coded to use the first of these. + * + */ #define USB_BCD 0x0200 -// #define USB_VID 0x1209 //--------------------------------------------------------------------+ // Device Descriptors From 3150ac5a766b5c9cc7bcc19a74037246f5ba3ec9 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Sun, 10 Nov 2024 10:40:45 -0800 Subject: [PATCH 2/3] Unique USB Descriptor string for each variant --- src/usb_descriptors.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/usb_descriptors.c b/src/usb_descriptors.c index b2cd9e36..c9d4c3eb 100644 --- a/src/usb_descriptors.c +++ b/src/usb_descriptors.c @@ -292,7 +292,15 @@ uint8_t const* tud_descriptor_configuration_cb(uint8_t index) { char const* string_desc_arr[] = { (const char[]){ 0x09, 0x04 }, // 0: is supported language is English (0x0409) "Bus Pirate", // 1: Manufacturer +#if (BP_VER == 5) "Bus Pirate 5", // 2: Product +#elif (BP_VER == XL5) + "Bus Pirate 5XL", // 2: Product +#elif (BP_VER == 6) + "Bus Pirate 6", // 2: Product +#else + #error "Unknown Bus Pirate version" +#endif "5buspirate", // 3: Serial -- now using chip ID (serial port can be remembered per device) "Bus Pirate CDC", // 4: CDC Interface "Bus Pirate MSC", // 5: MSC Interface From 4c1fc72b50e8a377cd05cc938e9abeed47b9deab Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Sun, 10 Nov 2024 10:46:09 -0800 Subject: [PATCH 3/3] remove incorrect macro It was dead code anyways, but also had a number of bugs. Not good. --- src/usb_descriptors.c | 37 +------------------------------------ 1 file changed, 1 insertion(+), 36 deletions(-) diff --git a/src/usb_descriptors.c b/src/usb_descriptors.c index c9d4c3eb..d2a7282c 100644 --- a/src/usb_descriptors.c +++ b/src/usb_descriptors.c @@ -28,47 +28,12 @@ #include "tusb.h" #include "pirate/mcu.h" -/* A combination of interfaces must have a unique product id, since PC will save device driver after the first plug. - * Same VID/PID with different interface e.g MSC (first), then CDC (later) will possibly cause system error on PC. - * - * Auto ProductID layout's Bitmap: - * [MSB] HID | MSC | CDC [LSB] - */ -#define _PID_MAP(itf, n) ((CFG_TUD_##itf) << (n)) -//#define USB_PID 0x7332 /*(0x4000 | _PID_MAP(CDC, 0) | _PID_MAP(MSC, 1) | _PID_MAP(HID, 2) | \ - _PID_MAP(MIDI, 3) | _PID_MAP(VENDOR, 4) )*/ /* - * CFG_TUD_xxx is defined as zero if the interface is not enabled. - * CFG_TUD_xxx is otherwise defined as the count of that type of interface. (????) - * - * It therefore appears that the "Auto ProductID layout" is designed ONLY for - * devices that have, at most, a single instance of each of those interface types. - * Because the buspirate device has two CDC interfaces, the above method results - * in an unexpected "auto-PID", and thus should NOT be used: - * - * _PID_MAP(CDC, 0) ==> _PID_MAP(CFG_TUD_CDC, 0) == _PID_MAP(2, 0) == 2 << 0 == 0x02 (!!! FAILS DUE TO MULTIPLE CDC INTERFACES !!!) - * _PID_MAP(MSC, 1) ==> _PID_MAP(CFG_TUD_MSC, 1) == _PID_MAP(1, 1) == 1 << 1 == 0x02 (!!!) - * _PID_MAP(HID, 2) ==> _PID_MAP(CFG_TUD_HID, 2) == _PID_MAP(0, 2) == 0 << 2 == 0x00 - * _PID_MAP(MIDI, 3) ==> _PID_MAP(CFG_TUD_MIDI, 3) == _PID_MAP(0, 3) == 0 << 3 == 0x00 - * - * Specifically, because CFG_TUD_CDC is defined as 2, _PID_MAP(CDC, 0) does not set - * the least significant bit (as expected per the second macro parameter), but instead - * sets the bit that is also used by _PID_MAP(MSC, 1). The resulting USB_PID would - * therefore be: 0x4000 | 0x02 | 0x02 | 0x00 | 0x00 == 0x4002. Decoding the bits - * would indicate that the device has a single MSC interface only (no CDC). - * - * I have no idea where this "Auto ProductID layout" came from, but the macros - * are not defined to fail gracefully when multiple instances of an interface - * are present. Moreover, at least for VID 0x1209, the above mapping would overlap - * with already-assigned VID/PID combinations. Therefore, this appears to not only - * be dead code, but also dangerously incorrect code. - * * BusPirate has obtained two VID/PID combinations from https://pid.codes/. * 0x1209 / 0x7331 - https://pid.codes/1209/7331/ - "Bus Pirate Next Gen CDC" * 0x1209 / 0x7332 - https://pid.codes/1209/7332/ - "Bus Pirate Next Gen Logic Analyzer" * - * The code is now hard-coded to use the first of these. - * + * The firmware is hard-coded to use the first of these. */ #define USB_BCD 0x0200