Enable library for Arduino SAMD based on USBCON #163
Conversation
There was a problem hiding this comment.
thank you for your PR, I have a few requests as follows:
- revert change to nrf since it is out of scope of this PR
- please explain again why changes with samd would help
- your testing matrix: examples + boards + core (adafruit/arduino)
Note: since this PR need arduino/ArduinoCore-samd#668, we could approve this but it won't get merged until the other is also approved/merged
| @@ -34,7 +34,7 @@ extern "C" { | |||
| //-------------------------------------------------------------------- | |||
| #define CFG_TUSB_MCU OPT_MCU_NRF5X | |||
|
|
|||
| #ifdef USE_TINYUSB | |||
| #ifdef USE_TINYUSB || !defined(USBCON) | |||
There was a problem hiding this comment.
please revert, since this PR is about samd
| @@ -38,7 +38,7 @@ extern "C" { | |||
| #define CFG_TUSB_MCU OPT_MCU_SAMD21 | |||
| #endif | |||
|
|
|||
| #ifdef USE_TINYUSB | |||
| #if defined(USE_TINYUSB) || !defined(USBCON) | |||
There was a problem hiding this comment.
can you explain again, why putting || !defined(USBCON) would help. I can imagine you drop the USBCON in your board.txt, why not also add USE_TINYUSB as well. I would prefer USE_TINYUSB is explicitly specify when using with the stack.
There was a problem hiding this comment.
In my proposed changes to the Arduino SAMD core USBCON would not longer be defined as you have said. I don't add USE_TINYUSB because it would be a library specific change to the Arduino core and I think the team would be less likely to accept it.
Also, if USE_TINYUSB was added, the logic for defining Serial would need updated as it would conflict with the Arduino SAMD core's definition. This is another reason why I chose this route.
I agree that not explicitly specifying USE_TINYUSB is not ideal but you've said yourself that Arduino team probably would not want to do it.
There was a problem hiding this comment.
from Arduino core pov, USBCON is defined when using CDC as Serial, and there is other board which use FTDI/CP210x as Serial, which lack of USBCON actually means. It may not be popular with samd core, but it is with avr one. And USBCON is consistently used in that manner.
Therefore I don't think you could persuade Arduino team to use lack of it as mean for TinyUSB, especially Arduino goal is having consistent API across other cores and libraries. Asking them to add menu for other usb stack has a better standing ground IMHO. But let wait to see if I am wrong.
There was a problem hiding this comment.
That makes sense. I haven't looked much at the AVR core so I didn't have that experience for reference. I figured USBCON had a more important use but like you said it wasn't obvious within the SAMD core.
If they aren't comfortable with it, a different defiine can be created to disable the built-in USB core.
Ideally a menu option could be added for TinyUSB but we'll see.
|
convert to draft to prevent accidentally merged. Should be marked as ready when other Arduino Core PR is merged, we could then continue with the review. |
|
closed due to lack of activities |
TinyUSB can be used without explicitly defining USE_TINYUSB. This is useful for cores without built-in support for the define.
Related PR: arduino/ArduinoCore-samd#668