BlokasLabs / USBMIDI

USB MIDI library for Arduino.
Other
189 stars 14 forks source link

Make USB enumeration work on Digistump cores #14

Closed nb-programmer closed 3 years ago

nb-programmer commented 3 years ago

On the fork of the Digistump core, Digispark did not enumerate the MIDI device after the bootloader timed out. This commit is adapted from the DigiKeyboard library code and seems to make it work now

gtrainavicius commented 3 years ago

Hey, thank you for submitting the pull request. It looks good, but is disabling and enabling interrupts really necessary there?

As this code is invoked during global initialization, even before the 'main' entry point, it could be unexpected for interrupts to have already been enabled when reaching main.

Did you try this without noInterrupts() and interrupts() calls? If it'd work, then perhaps only the delay part should be changed after '// USB Reset by device only required on Watchdog Reset.' comment to do the delay differently? :)

nb-programmer commented 3 years ago

I removed the interrupt enable/disable calls, and it still seems to work! So the issue was only the delay loop, which I think was interfering with v-usb's poll routine? We might need to investigate this. Anyway, this is the function:

static void midiUsbInit(void) {
    // Activate pull-ups except on USB lines.
    USB_CFG_IOPORT = (uint8_t)~((1 << USB_CFG_DMINUS_BIT) | (1 << USB_CFG_DPLUS_BIT));
    // All pins input except USB (-> USB reset).

#ifdef USB_CFG_PULLUP_IOPORT // Use usbDeviceConnect()/usbDeviceDisconnect() if available.
    USBDDR = 0;              // We do RESET by deactivating pullup.
    usbDeviceDisconnect();
#else
    USBDDR = (1 << USB_CFG_DMINUS_BIT) | (1 << USB_CFG_DPLUS_BIT);
#endif //USB_CFG_PULLUP_IOPORT

#ifdef ARDUINO_AVR_DIGISPARK
    _delay_ms(10);
#else
    // USB Reset by device only required on Watchdog Reset.
    uint8_t j = 0;
    while (--j) {   /* USB Reset by device only required on Watchdog Reset */
        uint8_t i = 0;
        while (--i);  /* delay >10ms for USB reset */
    }
#endif

#ifdef USB_CFG_PULLUP_IOPORT
    usbDeviceConnect();
#else
    USBDDR = 0; // Remove USB reset condition.
#endif //USB_CFG_PULLUP_IOPORT
    usbInit();
}

Also, I think the delay can be reduced, but 250 seems to be the magic number used in most of Digispark's libraries Edit: Yep, just 10ms is enough to work :)

nb-programmer commented 3 years ago

I have an UPDATE: It seems this issue is also present on the official Digispark core (Digispark with stock bootloader). I have one Digispark without the new bootloader, and it also fails to enumerate as well. So I think this problem is not just specific to the new core. (but my changes are not compatible with the old core as some libraries are renamed in the newer core)

gtrainavicius commented 3 years ago

I suspect the delay routine here, taken from V-USB examples, may be too quick to run, and is independent of the MCU frequency. Could you try enclosing it in some for (int k=0; k<4; ++k) loop to make it longer and see if that helps?

Exact time of the delay routine could be measured by triggering output of some available IO pin and measuring the signal using an oscilloscope or a logic analyzer.

nb-programmer commented 3 years ago

I've tried re-iterating the while loops with different ranges, from 0 to 20000 times, but still no luck.

//Still nothing
for (int k=0; k < 20000; k++) {
    uint8_t j = 0;
    while (--j) {   /* USB Reset by device only required on Watchdog Reset */
        uint8_t i = 0;
        while (--i);  /* delay >10ms for USB reset */
    }
}

Maybe maybe... could it be compiler optimization? The inner while loop may be getting optimized out, instead of being a nop?

There is something about _delay_ms, it seems to have changed based on the source code. It uses a function __builtin_avr_delay_cycles(), which was before a while loop calling _delay_loop_2()

gtrainavicius commented 3 years ago

Try adding volatile to the variable declarations.

nb-programmer commented 3 years ago

Unbelievable! That did the trick! I can't believe it was just this simple! Did Arduino's AVR optimization flags change at some point? I'm using v1.8.13

gtrainavicius commented 3 years ago

Good to hear we've found a decent solution. :) The optimization flags may depend on the board/core that is being used, so they may have changed over the time.

Could you please 'squash' the commits on your branch to contain only the latest version of the code and fix up the commit message? Then I'll merge the code. :)

nb-programmer commented 3 years ago

Done, e7f39358e791ebd967c61d907e0f82912bd01d06 should be the final change now

gtrainavicius commented 3 years ago

Done, thank you. :)