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.

Sign up for GitHub

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

Enable library for Arduino SAMD based on USBCON #163

Closed
wants to merge 1 commit into from
Closed

Enable library for Arduino SAMD based on USBCON #163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Adafruit_TinyUSB.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
#define ADAFRUIT_TINYUSB_H_

// Error message for Core that must select TinyUSB via menu
#if !defined(USE_TINYUSB) && ( defined(ARDUINO_ARCH_SAMD) || \
(defined(ARDUINO_ARCH_RP2040) && !defined(ARDUINO_ARCH_MBED)) )
#if !defined(USE_TINYUSB) && ((defined(ARDUINO_ARCH_SAMD) && defined(ARDUINO_SAMD_ADAFRUIT)) || \
(defined(ARDUINO_ARCH_RP2040) && !defined(ARDUINO_ARCH_MBED)))
#error TinyUSB is not selected, please select it in "Tools->Menu->USB Stack"
#endif

Expand Down
2 changes: 1 addition & 1 deletion src/arduino/ports/esp32/Adafruit_TinyUSB_esp32.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,4 +183,4 @@ extern "C" void yield(void) {
}
#endif // commented out

#endif
#endif // defined(ARDUINO_ARCH_ESP32) && TUSB_OPT_DEVICE_ENABLED
4 changes: 2 additions & 2 deletions src/arduino/ports/nrf/Adafruit_TinyUSB_nrf.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

#include "tusb_option.h"

#if defined ARDUINO_NRF52_ADAFRUIT && TUSB_OPT_DEVICE_ENABLED
#if defined(ARDUINO_NRF52_ADAFRUIT) && TUSB_OPT_DEVICE_ENABLED

#include "nrfx.h"
#include "nrfx_power.h"
Expand Down Expand Up @@ -134,4 +134,4 @@ static void usb_hardware_init(void) {
}
}

#endif // USE_TINYUSB
#endif // defined(ARDUINO_NRF52_ADAFRUIT) && TUSB_OPT_DEVICE_ENABLED
2 changes: 1 addition & 1 deletion src/arduino/ports/nrf/tusb_config_nrf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ extern "C" {
//--------------------------------------------------------------------
#define CFG_TUSB_MCU OPT_MCU_NRF5X

#ifdef USE_TINYUSB
#ifdef USE_TINYUSB || !defined(USBCON)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert, since this PR is about samd

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, I'll fix it.

#define CFG_TUSB_RHPORT0_MODE OPT_MODE_DEVICE
#else
#define CFG_TUSB_RHPORT0_MODE OPT_MODE_NONE
Expand Down
4 changes: 2 additions & 2 deletions src/arduino/ports/rp2040/Adafruit_TinyUSB_rp2040.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

#include "tusb_option.h"

#if defined ARDUINO_ARCH_RP2040 && TUSB_OPT_DEVICE_ENABLED
#if defined(ARDUINO_ARCH_RP2040) && TUSB_OPT_DEVICE_ENABLED

#include "Arduino.h"

Expand Down Expand Up @@ -151,4 +151,4 @@ void TinyUSB_Device_Task(void) {
}
}

#endif
#endif // defined(ARDUINO_ARCH_RP2040) && TUSB_OPT_DEVICE_ENABLED
4 changes: 2 additions & 2 deletions src/arduino/ports/samd/Adafruit_TinyUSB_samd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

#include "tusb_option.h"

#if defined ARDUINO_ARCH_SAMD && TUSB_OPT_DEVICE_ENABLED
#if defined(ARDUINO_ARCH_SAMD) && TUSB_OPT_DEVICE_ENABLED

#include "Arduino.h"
#include <Reset.h> // Needed for auto-reset with 1200bps port touch
Expand Down Expand Up @@ -156,4 +156,4 @@ uint8_t TinyUSB_Port_GetSerialNumber(uint8_t serial_id[16]) {
return 16;
}

#endif
#endif // defined(ARDUINO_ARCH_SAMD) && TUSB_OPT_DEVICE_ENABLED
2 changes: 1 addition & 1 deletion src/arduino/ports/samd/tusb_config_samd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ extern "C" {
#define CFG_TUSB_MCU OPT_MCU_SAMD21
#endif

#ifdef USE_TINYUSB
#if defined(USE_TINYUSB) || !defined(USBCON)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@hathach hathach Mar 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#define CFG_TUSB_RHPORT0_MODE OPT_MODE_DEVICE
#else
#define CFG_TUSB_RHPORT0_MODE OPT_MODE_NONE
Expand Down