Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow disabling CDC with -DCDC_DISABLED #383

Merged
merged 1 commit into from
Oct 1, 2021

Conversation

DanielGibson
Copy link
Contributor

This change makes it relatively easy to disable the CDC at compiletime.
Disabling it of course means that the serial console won't work anymore, and that you can only flash directly after resetting the board, but I think that's not much of a problem.

CDC_DISABLED is also used in ArduinoCore-samd for the same purpose,
see https://github.com/arduino/ArduinoCore-samd/blob/master/cores/arduino/USB/USBDesc.h#L24-L27

Disabling CDC helps making Arduino-based USB devices work with old or otherwise problematic hardware or software.
For example, USB Boot Keyboards don't work with many KVM switches if the CDC sub-devices are there; I'm sure there's other systems with the same problem (BIOS/UEFI implementations? Old operating systems - someone mentioned assistive devices still running Windows XP). Apart from Keyboards, I read that disabling CDC for Arduino-based MIDI devices.

based on https://github.com/gdsports/usb-metamorph/tree/master/USBSerPassThruLine

See also NicoHood/HID#225 and arduino/Arduino#6387 and https://forum.arduino.cc/index.php?topic=545288.msg3717028#msg3717028

Sometimes Arduino-based USB devices don't work because some hardware
(like KVM switches) gets confused by the CDC sub-devices.
This change makes it relatively easy to disable CDC at compiletime.
Disabling it of course means that the serial console won't work anymore,
so you need to use the reset button when flashing.

CDC_DISABLED is also used in ArduinoCore-samd for the same purpose.

based on
https://github.com/gdsports/usb-metamorph/tree/master/USBSerPassThruLine

See also NicoHood/HID#225 and
arduino/Arduino#6387 and
https://forum.arduino.cc/index.php?topic=545288.msg3717028#msg3717028
@teliot
Copy link

teliot commented Mar 7, 2021

This rolls back facchinm/ArduinoCore-avr@a1ba49e

@DanielGibson
Copy link
Contributor Author

DanielGibson commented Mar 7, 2021

Why was this compiletime-option removed in the first place?

It's very useful, after all.

@teliot
Copy link

teliot commented Mar 7, 2021

It looks what was removed was a define to enable/disable the CDC endpoint. My assumption is the current USB capabilities were made to work awaiting further improvement...

I can see that around the same period a few changes were made such as PluggableUSB and HID which forces a CDC endpoint. This is restricting and would be nice to be able to expand USB capabilities without blowing up the size of the compiled binaries. However its been like this for 5 years now.

@DanielGibson
Copy link
Contributor Author

I can see that around the same period a few changes were made such as PluggableUSB and HID which forces a CDC endpoint.

I'm not sure I understand - with my patch I'm using HID without CDC being enabled?

@CLAassistant
Copy link

CLAassistant commented Apr 9, 2021

CLA assistant check
All committers have signed the CLA.

@mdevaev
Copy link

mdevaev commented Jul 14, 2021

@facchinm Could you take a look at this? The problem really exists and affects several projects that are working on boot hid devices.

@NicoHood
Copy link
Contributor

I 2nd this PR. It would help a lot of people reporting issue with the HID Keyboard at bios time!

@SukkoPera
Copy link

+1

@DanielGibson
Copy link
Contributor Author

Any chance this can get merged?
If not, what should I change?

pinging @facchinm @silvanocerza

@teliot
Copy link

teliot commented Oct 1, 2021

EDIT: The below issue does not effect this patch and is to be ignored.

Found an issue with the change.
The caterina bootloader is not removing the USB interrupts. Without code clearing the USB interrupts once sei() is called my sparkfun pro micro unit locks up. Should clear these interrupts if CDC_DISABLED is defined.
#179
USBCON = 0;
USBINT &= ~(1 << VBUSTI);
UHWCON &= ~(1 << UVREGE);

@DanielGibson
Copy link
Contributor Author

Interesting - I didn't have that problem with my cheap Pro Micro clone - do you get it from just disabling CDC or only if you completeley disable USB?

@DanielGibson
Copy link
Contributor Author

Also, what changes exactly must be done?
https://github.com/arduino/ArduinoCore-avr/blob/master/bootloaders/caterina/Caterina.c doesn't really look like anything mentioned in #179 (or your comment): USBCON, USBINT and UHWCON aren't used at all

@teliot
Copy link

teliot commented Oct 1, 2021

I was having issues when i removed the USB code and was using hardware serial. The USB interrupts were still firing away and locked up at the first sei(); until i changed those registers. I will do a proper test now using your CDC_DISABLED patch but i suspect the same result as caterina bootloader is leaving usb in a semi initialized state.

@teliot
Copy link

teliot commented Oct 1, 2021

I tested the patch and found no issues as it does still leave USB connected to the host which is different then what I have been doing.

@SukkoPera
Copy link

I've also been using this patch for a while and it really works like a charm, please merge.

@facchinm facchinm merged commit 44dc454 into arduino:master Oct 1, 2021
@facchinm
Copy link
Member

facchinm commented Oct 1, 2021

Merged, sorry for the incredibly long delay

@DanielGibson
Copy link
Contributor Author

Thanks a lot! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants