Skip to content

Commit 9f1001e

Browse files
fix: address PR review feedback for MIDI 2.0 drivers
Host driver (midi2_host.c): - midih2_open() now returns actual parsed length instead of max_len, preventing composite device interface conflicts - Parsers (alt0/alt1) refactored to return const uint8_t* end pointer following midi_host.c switch/case pattern - Alt 1 CS Endpoint now parses MIDI 2.0 layout (bNumGrpTrmBlk at offset 3 with MIDI_CS_ENDPOINT_GENERAL_2_0 subtype check) instead of reusing MIDI 1.0 struct (bNumEmbMIDIJack) - midih2_set_config() now issues SET_INTERFACE control request via tuh_interface_set() before completing configuration. Falls back to alt 0 if SET_INTERFACE fails - Extracted midih2_set_config_complete() and midih2_set_interface_cb() for async SET_INTERFACE handling Device driver (midi2_device.c): - midi2d_open() skip loop now checks bInterfaceNumber, stopping at interfaces that belong to other functions in composite devices - SET_INTERFACE handler now rejects alt > 1 (returns false/stall) - Named constants for GTB descriptor types and MIDI protocol values Descriptor macros (usbd.h): - TUD_MIDI2_DESC_ALT1_HEAD: iInterface set to 0 (consistent with Alt 0), wTotalLength now uses TUD_MIDI2_DESC_ALT1_CS_LEN to cover all Alt 1 class-specific descriptors - TUD_MIDI2_DESC_ALT1_EP: now accepts GTB ID list via variadic args, emitting complete CS endpoint descriptor Host example: - CMakeLists.txt restricted to rp2040 family (display.c requires Pico SDK headers) - display.c: null terminator after strncpy in log scroll Documentation: - class_drivers.rst updated to reflect SET_INTERFACE behavior and auto-select with fallback Addresses: Codex P1 (#1, #2, #3), Copilot (#4-#9)
1 parent 5d5c50a commit 9f1001e

File tree

6 files changed

+166
-97
lines changed

6 files changed

+166
-97
lines changed

docs/reference/class_drivers.rst

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ The MIDI 2.0 Host driver enables TinyUSB to enumerate and communicate with USB M
8787
**Key Features:**
8888

8989
- **Reactive Architecture**: Auto-detects Alt Setting 1 (MIDI 2.0) capability during enumeration
90-
- **Auto-Selection**: Automatically selects the highest available protocol (MIDI 2.0 preferred)
90+
- **Auto-Selection**: Automatically selects the highest available protocol and issues SET_INTERFACE to activate Alt Setting 1 when MIDI 2.0 is detected
9191
- **Transparent Stream Messages**: All data (UMP packets + Stream Messages) flow through callbacks
9292
- **Memory Safe**: No dynamic allocation, fixed-size instances per device
9393

@@ -271,9 +271,9 @@ Architecture
271271
The MIDI 2.0 Host driver uses a **reactive, callback-driven architecture** that mirrors the proven patterns in TinyUSB's existing device drivers (CDC, HID, etc.):
272272

273273
- **Auto-Detection**: Host automatically detects Alt Setting 1 capability
274-
- **Auto-Selection**: Selects highest protocol available (MIDI 2.0 preferred)
275-
- **Application Control**: App makes protocol behavior decisions via callbacks
276-
- **Transparent I/O**: Stream Messages and UMP packets flow transparently
274+
- **Auto-Selection**: Selects highest protocol available and issues SET_INTERFACE
275+
- **Transparent I/O**: Stream Messages and UMP packets flow through callbacks
276+
- **Callback-Driven**: App receives events via callbacks (descriptor, mount, rx, tx, unmount)
277277

278278
Differences from MIDI 1.0 Host
279279
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

examples/host/midi2_host/CMakeLists.txt

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@ project(midi2_host C CXX ASM)
66

77
family_initialize_project(${PROJECT_NAME} ${CMAKE_CURRENT_LIST_DIR})
88

9-
if(FAMILY STREQUAL "espressif")
9+
# This example requires PIO-USB and Pico SDK (I2C, SSD1306 display)
10+
if(NOT FAMILY STREQUAL "rp2040")
1011
return()
1112
endif()
1213

examples/host/midi2_host/src/display.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,7 @@ void display_log(const char* text, uint16_t color) {
190190
if (log_count >= LOG_LINES) {
191191
for (int i = 0; i < LOG_LINES - 1; i++) {
192192
strncpy(log_lines[i], log_lines[i + 1], CHARS_PER_LINE);
193+
log_lines[i][CHARS_PER_LINE] = '\0';
193194
}
194195
log_count = LOG_LINES - 1;
195196
}

src/class/midi/midi2_device.c

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -505,12 +505,19 @@ uint16_t midi2d_open(uint8_t rhport, const tusb_desc_interface_t* desc_itf, uint
505505
}
506506

507507
// Skip remaining descriptors (alt setting 1, CS endpoints, GTB)
508+
// Stop at any interface descriptor that is not our MIDI Streaming alt setting
508509
while (tu_desc_in_bounds(p_desc, desc_end)) {
509510
uint8_t dtype = tu_desc_type(p_desc);
510-
if (dtype != TUSB_DESC_CS_INTERFACE && dtype != TUSB_DESC_CS_ENDPOINT &&
511-
dtype != TUSB_DESC_INTERFACE && dtype != TUSB_DESC_ENDPOINT) {
511+
512+
if (dtype == TUSB_DESC_INTERFACE) {
513+
const tusb_desc_interface_t* next_itf = (const tusb_desc_interface_t*) p_desc;
514+
// Continue only if this is an alternate setting of our own interface
515+
if (next_itf->bInterfaceNumber != desc_midi->bInterfaceNumber) break;
516+
} else if (dtype != TUSB_DESC_CS_INTERFACE && dtype != TUSB_DESC_CS_ENDPOINT &&
517+
dtype != TUSB_DESC_ENDPOINT) {
512518
break;
513519
}
520+
514521
p_desc = tu_desc_next(p_desc);
515522
}
516523

@@ -528,6 +535,9 @@ bool midi2d_control_xfer_cb(uint8_t rhport, uint8_t stage, const tusb_control_re
528535
uint8_t itf_num = tu_u16_low(request->wIndex);
529536
uint8_t alt = tu_u16_low(request->wValue);
530537

538+
// Only Alt Setting 0 (MIDI 1.0) and 1 (UMP) are valid
539+
if (alt > 1) return false;
540+
531541
uint8_t idx = find_midi2_itf_by_num(itf_num);
532542
if (idx >= CFG_TUD_MIDI2) return false;
533543

src/class/midi/midi2_host.c

Lines changed: 135 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -125,9 +125,10 @@ static inline uint8_t get_idx_by_ep_addr(uint8_t daddr, uint8_t ep_addr) {
125125
// Descriptor parsing
126126
//--------------------------------------------------------------------+
127127

128-
static void midih2_parse_descriptors_alt0(midih2_interface_t *p_midi,
128+
// Parse Alt Setting 0 (MIDI 1.0) descriptors. Returns pointer past last consumed descriptor.
129+
static const uint8_t* midih2_parse_descriptors_alt0(midih2_interface_t *p_midi,
129130
const tusb_desc_interface_t *desc_itf, const uint8_t *desc_end) {
130-
TU_VERIFY(AUDIO_SUBCLASS_MIDI_STREAMING == desc_itf->bInterfaceSubClass,);
131+
TU_VERIFY(AUDIO_SUBCLASS_MIDI_STREAMING == desc_itf->bInterfaceSubClass, NULL);
131132

132133
p_midi->bInterfaceNumber = desc_itf->bInterfaceNumber;
133134

@@ -136,93 +137,113 @@ static void midih2_parse_descriptors_alt0(midih2_interface_t *p_midi,
136137

137138
uint8_t rx_cable_count = 0;
138139
uint8_t tx_cable_count = 0;
140+
bool found_new_interface = false;
139141

140-
while (tu_desc_in_bounds(p_desc, desc_end)) {
141-
if (tu_desc_type(p_desc) == TUSB_DESC_INTERFACE) {
142-
break;
143-
}
144-
145-
if (tu_desc_type(p_desc) == TUSB_DESC_ENDPOINT) {
146-
const tusb_desc_endpoint_t *p_ep = (const tusb_desc_endpoint_t *) p_desc;
147-
148-
// Open endpoint and stream
149-
TU_ASSERT(tuh_edpt_open(p_midi->daddr, p_ep),);
150-
if (tu_edpt_dir(p_ep->bEndpointAddress) == TUSB_DIR_IN) {
151-
tu_edpt_stream_open(&p_midi->ep_stream.rx, p_midi->daddr, p_ep, tu_edpt_packet_size(p_ep));
152-
tu_edpt_stream_clear(&p_midi->ep_stream.rx);
153-
} else {
154-
tu_edpt_stream_open(&p_midi->ep_stream.tx, p_midi->daddr, p_ep, tu_edpt_packet_size(p_ep));
155-
tu_edpt_stream_clear(&p_midi->ep_stream.tx);
156-
}
157-
158-
p_desc = tu_desc_next(p_desc);
142+
while (tu_desc_in_bounds(p_desc, desc_end) && !found_new_interface) {
143+
switch (tu_desc_type(p_desc)) {
144+
case TUSB_DESC_INTERFACE:
145+
found_new_interface = true;
146+
break;
159147

160-
if (tu_desc_in_bounds(p_desc, desc_end) && tu_desc_type(p_desc) == TUSB_DESC_CS_ENDPOINT) {
161-
const midi_desc_cs_endpoint_t *p_csep = (const midi_desc_cs_endpoint_t *) p_desc;
148+
case TUSB_DESC_ENDPOINT: {
149+
const tusb_desc_endpoint_t *p_ep = (const tusb_desc_endpoint_t *) p_desc;
162150

163-
if (tu_edpt_dir(p_ep->bEndpointAddress) == TUSB_DIR_OUT) {
164-
tx_cable_count = p_csep->bNumEmbMIDIJack;
151+
TU_ASSERT(tuh_edpt_open(p_midi->daddr, p_ep), NULL);
152+
if (tu_edpt_dir(p_ep->bEndpointAddress) == TUSB_DIR_IN) {
153+
tu_edpt_stream_open(&p_midi->ep_stream.rx, p_midi->daddr, p_ep, tu_edpt_packet_size(p_ep));
154+
tu_edpt_stream_clear(&p_midi->ep_stream.rx);
165155
} else {
166-
rx_cable_count = p_csep->bNumEmbMIDIJack;
156+
tu_edpt_stream_open(&p_midi->ep_stream.tx, p_midi->daddr, p_ep, tu_edpt_packet_size(p_ep));
157+
tu_edpt_stream_clear(&p_midi->ep_stream.tx);
158+
}
159+
160+
p_desc = tu_desc_next(p_desc);
161+
if (tu_desc_in_bounds(p_desc, desc_end) && tu_desc_type(p_desc) == TUSB_DESC_CS_ENDPOINT) {
162+
const midi_desc_cs_endpoint_t *p_csep = (const midi_desc_cs_endpoint_t *) p_desc;
163+
if (tu_edpt_dir(p_ep->bEndpointAddress) == TUSB_DIR_OUT) {
164+
tx_cable_count = p_csep->bNumEmbMIDIJack;
165+
} else {
166+
rx_cable_count = p_csep->bNumEmbMIDIJack;
167+
}
167168
}
169+
break;
168170
}
171+
172+
default:
173+
break;
169174
}
170175

171-
p_desc = tu_desc_next(p_desc);
176+
if (!found_new_interface) {
177+
p_desc = tu_desc_next(p_desc);
178+
}
172179
}
173180

174181
p_midi->rx_cable_count_alt0 = rx_cable_count;
175182
p_midi->tx_cable_count_alt0 = tx_cable_count;
183+
return p_desc;
176184
}
177185

178-
static void midih2_parse_descriptors_alt1(midih2_interface_t *p_midi,
186+
// Parse Alt Setting 1 (MIDI 2.0 UMP) descriptors. Returns pointer past last consumed descriptor.
187+
static const uint8_t* midih2_parse_descriptors_alt1(midih2_interface_t *p_midi,
179188
const tusb_desc_interface_t *desc_itf, const uint8_t *desc_end) {
180-
TU_VERIFY(AUDIO_SUBCLASS_MIDI_STREAMING == desc_itf->bInterfaceSubClass,);
181-
TU_VERIFY(desc_itf->bAlternateSetting == 1,);
189+
TU_VERIFY(AUDIO_SUBCLASS_MIDI_STREAMING == desc_itf->bInterfaceSubClass, NULL);
190+
TU_VERIFY(desc_itf->bAlternateSetting == 1, NULL);
182191

183192
const uint8_t *p_desc = (const uint8_t *) desc_itf;
184193
p_desc = tu_desc_next(p_desc);
185194

186195
uint8_t rx_cable_count = 0;
187196
uint8_t tx_cable_count = 0;
197+
bool found_new_interface = false;
188198

189-
while (tu_desc_in_bounds(p_desc, desc_end)) {
190-
if (tu_desc_type(p_desc) == TUSB_DESC_INTERFACE) {
191-
break;
192-
}
199+
while (tu_desc_in_bounds(p_desc, desc_end) && !found_new_interface) {
200+
switch (tu_desc_type(p_desc)) {
201+
case TUSB_DESC_INTERFACE:
202+
found_new_interface = true;
203+
break;
193204

194-
if (tu_desc_type(p_desc) == TUSB_DESC_CS_INTERFACE) {
195-
if (tu_desc_subtype(p_desc) == MIDI_CS_INTERFACE_HEADER) {
196-
const uint8_t *bcd_ptr = p_desc + 3;
197-
p_midi->bcdMSC_lo = bcd_ptr[0];
198-
p_midi->bcdMSC_hi = bcd_ptr[1];
205+
case TUSB_DESC_CS_INTERFACE:
206+
if (tu_desc_subtype(p_desc) == MIDI_CS_INTERFACE_HEADER) {
207+
// bcdMSC at offset 3-4 in CS Interface Header
208+
const uint8_t *bcd_ptr = p_desc + 3;
209+
p_midi->bcdMSC_lo = bcd_ptr[0];
210+
p_midi->bcdMSC_hi = bcd_ptr[1];
211+
if (p_midi->bcdMSC_hi == 0x02) { // bcdMSC 0x0200 = USB-MIDI 2.0
212+
p_midi->protocol_version = 1;
213+
}
214+
}
215+
break;
199216

200-
if (p_midi->bcdMSC_hi == 0x02) {
201-
p_midi->protocol_version = 1;
217+
case TUSB_DESC_ENDPOINT: {
218+
const tusb_desc_endpoint_t *p_ep = (const tusb_desc_endpoint_t *) p_desc;
219+
p_desc = tu_desc_next(p_desc);
220+
221+
if (tu_desc_in_bounds(p_desc, desc_end) && tu_desc_type(p_desc) == TUSB_DESC_CS_ENDPOINT) {
222+
// MIDI 2.0 CS Endpoint General 2.0: bNumGrpTrmBlk at offset 3
223+
if (p_desc[0] >= 4 && p_desc[2] == MIDI_CS_ENDPOINT_GENERAL_2_0) {
224+
uint8_t num_grp_trm_blk = p_desc[3];
225+
if (tu_edpt_dir(p_ep->bEndpointAddress) == TUSB_DIR_OUT) {
226+
tx_cable_count = num_grp_trm_blk;
227+
} else {
228+
rx_cable_count = num_grp_trm_blk;
229+
}
230+
}
202231
}
232+
break;
203233
}
234+
235+
default:
236+
break;
204237
}
205238

206-
if (tu_desc_type(p_desc) == TUSB_DESC_ENDPOINT) {
207-
const tusb_desc_endpoint_t *p_ep = (const tusb_desc_endpoint_t *) p_desc;
239+
if (!found_new_interface) {
208240
p_desc = tu_desc_next(p_desc);
209-
210-
if (tu_desc_in_bounds(p_desc, desc_end) && tu_desc_type(p_desc) == TUSB_DESC_CS_ENDPOINT) {
211-
const midi_desc_cs_endpoint_t *p_csep = (const midi_desc_cs_endpoint_t *) p_desc;
212-
213-
if (tu_edpt_dir(p_ep->bEndpointAddress) == TUSB_DIR_OUT) {
214-
tx_cable_count = p_csep->bNumEmbMIDIJack;
215-
} else {
216-
rx_cable_count = p_csep->bNumEmbMIDIJack;
217-
}
218-
}
219241
}
220-
221-
p_desc = tu_desc_next(p_desc);
222242
}
223243

224244
p_midi->rx_cable_count_alt1 = rx_cable_count;
225245
p_midi->tx_cable_count_alt1 = tx_cable_count;
246+
return p_desc;
226247
}
227248

228249
//--------------------------------------------------------------------+
@@ -327,33 +348,20 @@ uint16_t midih2_open(uint8_t rhport, uint8_t dev_addr, const tusb_desc_interface
327348
desc_itf->bInterfaceNumber, desc_itf->bAlternateSetting, dev_addr);
328349

329350
// Dispatch to appropriate parser based on Alt Setting
351+
const uint8_t *p_end = NULL;
330352
if (desc_itf->bAlternateSetting == 0) {
331-
midih2_parse_descriptors_alt0(p_midi, desc_itf, desc_end);
353+
p_end = midih2_parse_descriptors_alt0(p_midi, desc_itf, desc_end);
332354
} else if (desc_itf->bAlternateSetting == 1) {
333-
midih2_parse_descriptors_alt1(p_midi, desc_itf, desc_end);
355+
p_end = midih2_parse_descriptors_alt1(p_midi, desc_itf, desc_end);
334356
}
335357

336-
return max_len;
358+
// Return number of bytes consumed (following midi_host.c pattern)
359+
uint16_t const parsed_len = (p_end != NULL) ? (uint16_t)(p_end - desc_start) : 0;
360+
return parsed_len;
337361
}
338362

339-
bool midih2_set_config(uint8_t dev_addr, uint8_t itf_num) {
340-
uint8_t idx = 0;
341-
for (idx = 0; idx < CFG_TUH_MIDI2; idx++) {
342-
if (_midi2_host[idx].daddr == dev_addr && _midi2_host[idx].bInterfaceNumber == itf_num) {
343-
break;
344-
}
345-
}
346-
347-
if (idx >= CFG_TUH_MIDI2) {
348-
// Not our interface (e.g. Audio Control) - pass through to next
349-
usbh_driver_set_config_complete(dev_addr, itf_num);
350-
return true;
351-
}
352-
353-
midih2_interface_t *p_midi = &_midi2_host[idx];
354-
355-
// Auto-select alt setting
356-
midih2_auto_select_alt_setting(p_midi);
363+
static void midih2_set_config_complete(midih2_interface_t *p_midi, uint8_t idx) {
364+
uint8_t dev_addr = p_midi->daddr;
357365

358366
// Invoke descriptor_cb
359367
tuh_midi2_descriptor_cb_t desc_cb = {
@@ -374,7 +382,7 @@ bool midih2_set_config(uint8_t dev_addr, uint8_t itf_num) {
374382

375383
// Invoke mount_cb
376384
tuh_midi2_mount_cb_t mount_cb = {
377-
.daddr = p_midi->daddr,
385+
.daddr = dev_addr,
378386
.bInterfaceNumber = p_midi->bInterfaceNumber,
379387
.protocol_version = p_midi->protocol_version,
380388
.alt_setting_active = p_midi->alt_setting_current,
@@ -387,7 +395,56 @@ bool midih2_set_config(uint8_t dev_addr, uint8_t itf_num) {
387395
tu_edpt_stream_read_xfer(&p_midi->ep_stream.rx);
388396

389397
// Signal USBH that configuration is complete
390-
usbh_driver_set_config_complete(dev_addr, itf_num);
398+
usbh_driver_set_config_complete(dev_addr, p_midi->bInterfaceNumber);
399+
}
400+
401+
static void midih2_set_interface_cb(tuh_xfer_t *xfer) {
402+
uint8_t const dev_addr = xfer->daddr;
403+
uint8_t const itf_num = (uint8_t) tu_le16toh(xfer->setup->wIndex);
404+
405+
// Find our interface
406+
for (uint8_t idx = 0; idx < CFG_TUH_MIDI2; idx++) {
407+
if (_midi2_host[idx].daddr == dev_addr && _midi2_host[idx].bInterfaceNumber == itf_num) {
408+
if (xfer->result == XFER_RESULT_SUCCESS) {
409+
midih2_set_config_complete(&_midi2_host[idx], idx);
410+
} else {
411+
// SET_INTERFACE failed, fall back to alt 0
412+
TU_LOG_DRV("MIDI2 SET_INTERFACE failed, falling back to alt 0\r\n");
413+
_midi2_host[idx].alt_setting_current = 0;
414+
midih2_set_config_complete(&_midi2_host[idx], idx);
415+
}
416+
return;
417+
}
418+
}
419+
}
420+
421+
bool midih2_set_config(uint8_t dev_addr, uint8_t itf_num) {
422+
uint8_t idx = 0;
423+
for (idx = 0; idx < CFG_TUH_MIDI2; idx++) {
424+
if (_midi2_host[idx].daddr == dev_addr && _midi2_host[idx].bInterfaceNumber == itf_num) {
425+
break;
426+
}
427+
}
428+
429+
if (idx >= CFG_TUH_MIDI2) {
430+
// Not our interface (e.g. Audio Control) - pass through to next
431+
usbh_driver_set_config_complete(dev_addr, itf_num);
432+
return true;
433+
}
434+
435+
midih2_interface_t *p_midi = &_midi2_host[idx];
436+
437+
// Auto-select alt setting
438+
midih2_auto_select_alt_setting(p_midi);
439+
440+
// If MIDI 2.0 detected, issue SET_INTERFACE to activate Alt Setting 1
441+
if (p_midi->alt_setting_current == 1) {
442+
TU_LOG_DRV("MIDI2 requesting SET_INTERFACE alt 1 for itf %u\r\n", itf_num);
443+
TU_ASSERT(tuh_interface_set(dev_addr, itf_num, 1, midih2_set_interface_cb, 0));
444+
} else {
445+
// MIDI 1.0 only, complete immediately
446+
midih2_set_config_complete(p_midi, idx);
447+
}
391448

392449
return true;
393450
}

0 commit comments

Comments
 (0)