Add library for reset interface #2629

2025-08-14T08:54:08-05:00

will-v-pi commented

2025-08-14 08:54:08 -05:00

(Migrated from github.com)

This separates the stdio_usb reset interface out into a separate library, which can be used by applications which use TinyUSB directly.

No change in behaviour when using the typical stdio_usb (eg in the hello_usb example).

Keeps the PICO_STDIO_USB_ prefix on the config defines to maintain compatibility, and also maintains compatibility with picotool.

Instructions to use this are added to the header file.

See implementation in debugprobe and the dev_multi_cdc example

This separates the stdio_usb reset interface out into a separate library, which can be used by applications which use TinyUSB directly. No change in behaviour when using the typical stdio_usb (eg in the hello_usb example). Keeps the `PICO_STDIO_USB_` prefix on the config defines to maintain compatibility, and also maintains compatibility with picotool. Instructions to use this are added to the header file. See implementation in [debugprobe](https://github.com/raspberrypi/debugprobe/pull/178) and the [dev_multi_cdc example](https://github.com/raspberrypi/pico-examples/pull/694)

👍 1

kilograham (Migrated from github.com) reviewed

2025-08-14 12:00:46 -05:00

src/common/pico_usb_reset_interface_headers/include/pico/usb_reset_interface.h

i would be tempted to move this into a separate header under pico_usb_reset_interface_headers, that can be included if desired

kilograham (Migrated from github.com) reviewed

2025-08-14 12:01:23 -05:00

src/common/pico_usb_reset_interface_headers/include/pico/usb_reset_interface.h

not sure resetd is a sufficient/good prefix for the public API

not sure `resetd` is a sufficient/good prefix for the public API

kilograham (Migrated from github.com) reviewed

2025-08-14 12:02:27 -05:00

src/common/pico_usb_reset_interface_headers/include/pico/usb_reset_interface.h

note also, on this and the below, this is peculiar to TinyUSB - as we think about CherryUSB etc, we (i haven't looked in detail) should consider what if any of this is exposed (and whether we should expose them again in a separate header - e.g. one for TinyUSB one for CherryUSB)

will-v-pi (Migrated from github.com) reviewed

2025-08-15 05:45:19 -05:00

src/common/pico_usb_reset_interface_headers/include/pico/usb_reset_interface.h

I will rename them to pico_usb_reset_interface_...

Note that these functions are not supposed to be exposed as a public API - the only thing that should be used publicly is the _resetd_driver struct, I will try to refactor it so that's the case

I will rename them to `pico_usb_reset_interface_...` Note that these functions are not supposed to be exposed as a public API - the only thing that should be used publicly is the `_resetd_driver` struct, I will try to refactor it so that's the case

will-v-pi (Migrated from github.com) reviewed

2025-08-15 06:32:51 -05:00

src/common/pico_usb_reset_interface_headers/include/pico/usb_reset_interface.h

Have made a separate usb_reset_interface_device.h file which should be included instead of usb_reset_interface.h

Have made a separate `usb_reset_interface_device.h` file which should be included instead of `usb_reset_interface.h`

will-v-pi (Migrated from github.com) reviewed

2025-08-15 06:38:20 -05:00

src/common/pico_usb_reset_interface_headers/include/pico/usb_reset_interface.h

I made a separate TinyUSB header, and made it so this should be compatible with the CherryUSB PR (as that only uses the usb_reset_interface.h header)

I couldn't refactor the functions out of the header, because I think the pico_usb_reset_interface_driver has to be defined in the header as static const in order for the debugprobe inclusion to compile, and therefore the functions must be in the header too

I made a separate TinyUSB header, and made it so this should be compatible with the CherryUSB PR (as that only uses the `usb_reset_interface.h` header) I couldn't refactor the functions out of the header, because I think the `pico_usb_reset_interface_driver` has to be defined in the header as `static const` in order for the debugprobe inclusion to compile, and therefore the functions must be in the header too

kilograham (Migrated from github.com) reviewed

2025-08-15 10:52:47 -05:00

src/common/pico_usb_reset_interface_headers/include/pico/usb_reset_interface.h

I will rename them to pico_usb_reset_interface_...

Note that these functions are not supposed to be exposed as a public API - the only thing that should be used publicly is the _resetd_driver struct, I will try to refactor it so that's the case

note pico_/hardware_ is not included in function names; e.g. dma_foo

> I will rename them to `pico_usb_reset_interface_...` > > Note that these functions are not supposed to be exposed as a public API - the only thing that should be used publicly is the `_resetd_driver` struct, I will try to refactor it so that's the case note `pico_`/`hardware_` is not included in function names; e.g. `dma_foo`

lurch (Migrated from github.com) reviewed

2025-08-17 07:24:55 -05:00

src/common/pico_usb_reset_interface_headers/include/pico/usb_reset_interface_config.h

// PICO_CONFIG: PICO_STDIO_USB_RESET_INTERFACE_MS_OS_20_DESCRIPTOR_ITF, If vendor reset interface is included this specifies the USB interface number for the reset interface, type=int, default=2 if application is not using TinyUSB directly, undefined otherwise, group=pico_usb_reset_interface

Perhaps this is slightly more readable?

```suggestion // PICO_CONFIG: PICO_STDIO_USB_RESET_INTERFACE_MS_OS_20_DESCRIPTOR_ITF, If vendor reset interface is included this specifies the USB interface number for the reset interface, type=int, default=2 if application is not using TinyUSB directly, undefined otherwise, group=pico_usb_reset_interface ``` Perhaps this is slightly more readable?

lurch (Migrated from github.com) reviewed

2025-08-17 07:30:40 -05:00

src/common/pico_usb_reset_interface_headers/include/pico/usb_reset_interface_config.h

// PICO_CONFIG: PICO_STDIO_USB_RESET_INCLUDE_DEFAULT_APP_DRIVER_CB, Set to 0 if your application defines its own usbd_app_driver_get_cb function, type=bool, default=1 when using pico_usb_reset_interface, 0 otherwise, group=pico_usb_reset_interface
#ifndef PICO_STDIO_USB_RESET_INCLUDE_DEFAULT_APP_DRIVER_CB
#define PICO_STDIO_USB_RESET_INCLUDE_DEFAULT_APP_DRIVER_CB LIB_PICO_STDIO_USB
#endif

Maybe? 🤷‍♂️

```suggestion // PICO_CONFIG: PICO_STDIO_USB_RESET_INCLUDE_DEFAULT_APP_DRIVER_CB, Set to 0 if your application defines its own usbd_app_driver_get_cb function, type=bool, default=1 when using pico_usb_reset_interface, 0 otherwise, group=pico_usb_reset_interface #ifndef PICO_STDIO_USB_RESET_INCLUDE_DEFAULT_APP_DRIVER_CB #define PICO_STDIO_USB_RESET_INCLUDE_DEFAULT_APP_DRIVER_CB LIB_PICO_STDIO_USB #endif ``` Maybe? :man_shrugging:

lurch (Migrated from github.com) reviewed

2025-08-17 07:35:08 -05:00

src/common/pico_usb_reset_interface_headers/include/pico/usb_reset_interface_device.h

Does this new pico_usb_reset_interface group need adding to docs/index.h ?

Does this new `pico_usb_reset_interface` group need adding to `docs/index.h` ?

popcornmix commented

2026-03-03 09:17:38 -06:00

(Migrated from github.com)

Any reason this has stalled? I'd quite like to use this, so my project can handle reset to reconfigure in a new mode.

mtfurlan commented

2026-03-03 09:28:04 -06:00

(Migrated from github.com)

I've been using this PR in a project for a while now, which isn't great, but it has been working.

lurch commented

2026-03-05 11:18:00 -06:00

(Migrated from github.com)

Looks like this PR has conflicts which need resolving? @will-v-pi

will-v-pi commented

2026-03-05 11:30:55 -06:00

(Migrated from github.com)

Looks like this PR has conflicts which need resolving? @will-v-pi

I've rebased it onto develop to resolve those - this PR is awaiting review by @kilograham

> Looks like this PR has conflicts which need resolving? @will-v-pi I've rebased it onto develop to resolve those - this PR is awaiting review by @kilograham

kilograham commented

2026-05-16 12:35:48 -05:00

(Migrated from github.com)

oops - sorry i seem to have ignored this!

we should probably remove the STDIO_ from the define names, and add backwards-compatibility in the least ugly place possible
we have hit a slight snafu obv with the fact that pico/usb_reset_interface (and particularly the header) existed before.. i'm kinda of loath to have a pico/foo where foo.h is not the API, so i wonder if we should rename this to something else

oops - sorry i seem to have ignored this! 1. we should probably remove the STDIO_ from the define names, and add backwards-compatibility in the least ugly place possible 2. we have hit a slight snafu obv with the fact that pico/usb_reset_interface (and particularly the header) existed before.. i'm kinda of loath to have a pico/foo where foo.h is not the API, so i wonder if we should rename this to something else

will-v-pi commented

2026-05-18 11:34:17 -05:00

(Migrated from github.com)

oops - sorry i seem to have ignored this!

we should probably remove the STDIO_ from the define names, and add backwards-compatibility in the least ugly place possible

we have hit a slight snafu obv with the fact that pico/usb_reset_interface (and particularly the header) existed before.. i'm kinda of loath to have a pico/foo where foo.h is not the API, so i wonder if we should rename this to something else

I can rename the new library to pico_usb_reset_interface_device, as the header name you actually include to use it is already usb_reset_interface_device.h. That will make it the longest library name, but it's only slightly longer than pico_bootsel_via_double_reset so it's probably fine. I can also move the new headers out of common and into that library.

> oops - sorry i seem to have ignored this! > > 1. we should probably remove the STDIO_ from the define names, and add backwards-compatibility in the least ugly place possible > 2. we have hit a slight snafu obv with the fact that pico/usb_reset_interface (and particularly the header) existed before.. i'm kinda of loath to have a pico/foo where foo.h is not the API, so i wonder if we should rename this to something else I can rename the new library to `pico_usb_reset_interface_device`, as the header name you actually include to use it is already `usb_reset_interface_device.h`. That will make it the longest library name, but it's only slightly longer than `pico_bootsel_via_double_reset` so it's probably fine. I can also move the new headers out of `common` and into that library.

will-v-pi commented

2026-05-18 11:45:54 -05:00

(Migrated from github.com)

Actually, maybe just pico_usb_reset is fine for the library name, given it also includes the resetting via baud rate

Actually, maybe just `pico_usb_reset` is fine for the library name, given it also includes the resetting via baud rate

lurch (Migrated from github.com) reviewed

2026-05-19 07:03:40 -05:00

src/common/pico_usb_reset_interface_headers/include/pico/usb_reset_interface_config.h

"default=1 when using pico_usb_reset_interface, 0 otherwise"
😕

Search&replace typo?

"default=1 when using pico_usb_reset_interface, 0 otherwise" :confused: Search&replace typo?

will-v-pi (Migrated from github.com) reviewed

2026-05-19 07:06:45 -05:00

src/common/pico_usb_reset_interface_headers/include/pico/usb_reset_interface_config.h

// PICO_CONFIG: PICO_USB_RESET_INCLUDE_DEFAULT_APP_DRIVER_CB, Set to 0 if your application defines its own usbd_app_driver_get_cb function, type=bool, default=1 when using pico_stdio_usb, 0 otherwise, group=pico_usb_reset_interface

```suggestion // PICO_CONFIG: PICO_USB_RESET_INCLUDE_DEFAULT_APP_DRIVER_CB, Set to 0 if your application defines its own usbd_app_driver_get_cb function, type=bool, default=1 when using pico_stdio_usb, 0 otherwise, group=pico_usb_reset_interface ```

lurch (Migrated from github.com) reviewed

2026-05-26 03:01:57 -05:00

src/rp2_common/pico_usb_reset/usb_reset.c

					
				@ -0,0 +111,4 @@

				            if (request->wValue & (1u << 8)) {

				                active_low = request->wValue & (1u << 7);

				                gpio = request->wValue >> 9u;

				            }

It might be worth swapping around two of these lines, so that the code better matches the comment?

            // wValue layout:
            //   bits 0-1  : forwarded as the bootrom disable_interface_mask
            //   bit 7     : 1 if the activity-LED GPIO is active-low
            //   bit 8     : 1 if an activity-LED GPIO is being specified
            //   bits 9-15 : activity-LED GPIO number (0-127)
            if (request->wValue & 0x100) {
                active_low = request->wValue & 0x80;
                gpio = request->wValue >> 9u;
            }

It _might_ be worth swapping around two of these lines, so that the code better matches the comment? ```suggestion // wValue layout: // bits 0-1 : forwarded as the bootrom disable_interface_mask // bit 7 : 1 if the activity-LED GPIO is active-low // bit 8 : 1 if an activity-LED GPIO is being specified // bits 9-15 : activity-LED GPIO number (0-127) if (request->wValue & 0x100) { active_low = request->wValue & 0x80; gpio = request->wValue >> 9u; } ```

kilograham commented

2026-05-26 16:49:28 -05:00

(Migrated from github.com)

ok, so there was maybe a bit of a miscommunication on the renaming, but i have looked at it again anyway.

I am cool with leaving it called pico_usb_reset_interface since thats really what it is - the original headers can remain.

even though they are long, we should probably keep PICO_USB_RESET_INTERFACE_ in the names to be consistent (i could handle contraction to IFC i guess)

It's kinda ugly, but the real headers really belong in rp2_common, so i'd actually put them with the code in rp2_common, but add them to the pico_usb_reset_interface_headers, and #include them from the original one guarded by a #if LIB_PICO_USB_RESET_INTERFACE

ok, so there was maybe a bit of a miscommunication on the renaming, but i have looked at it again anyway. I am cool with leaving it called `pico_usb_reset_interface` since thats really what it is - the original headers can remain. even though they are long, we should probably keep PICO_USB_RESET_INTERFACE_ in the names to be consistent (i could handle contraction to IFC i guess) It's kinda ugly, but the real headers really belong in rp2_common, so i'd actually put them with the code in rp2_common, but add them to the pico_usb_reset_interface_headers, and #include them from the original one guarded by a `#if LIB_PICO_USB_RESET_INTERFACE`

will-v-pi commented

2026-05-27 03:10:08 -05:00

(Migrated from github.com)

ok, so there was maybe a bit of a miscommunication on the renaming, but i have looked at it again anyway.

I am cool with leaving it called pico_usb_reset_interface since thats really what it is - the original headers can remain.

even though they are long, we should probably keep PICO_USB_RESET_INTERFACE_ in the names to be consistent (i could handle contraction to IFC i guess)

It's kinda ugly, but the real headers really belong in rp2_common, so i'd actually put them with the code in rp2_common, but add them to the pico_usb_reset_interface_headers, and #include them from the original one guarded by a #if LIB_PICO_USB_RESET_INTERFACE

Sorry, I was occupied with other stuff - I think the rename to pico_usb_reset makes all of this cleaner so I will do that, as it allows for a separate pico_usb_reset_headers library so everything can be in rp2_common without any hacks in the existing header

> ok, so there was maybe a bit of a miscommunication on the renaming, but i have looked at it again anyway. > > I am cool with leaving it called `pico_usb_reset_interface` since thats really what it is - the original headers can remain. > > even though they are long, we should probably keep PICO_USB_RESET_INTERFACE_ in the names to be consistent (i could handle contraction to IFC i guess) > > It's kinda ugly, but the real headers really belong in rp2_common, so i'd actually put them with the code in rp2_common, but add them to the pico_usb_reset_interface_headers, and #include them from the original one guarded by a `#if LIB_PICO_USB_RESET_INTERFACE` Sorry, I was occupied with other stuff - I think the rename to `pico_usb_reset` makes all of this cleaner so I will do that, as it allows for a separate pico_usb_reset_headers library so everything can be in rp2_common without any hacks in the existing header

lurch (Migrated from github.com) reviewed

2026-05-27 04:39:44 -05:00

src/rp2_common/pico_usb_reset/include/pico/usb_reset_config.h

					
				@ -0,0 +1,126 @@

				/*

https://github.com/raspberrypi/pico-sdk/pull/2971/files mentions that the activity led active_low setting is ignored on RP2040. Is the same true here? (And if so, should it be mentioned?)

lurch (Migrated from github.com) reviewed

2026-05-27 04:47:10 -05:00

src/rp2_common/pico_usb_reset/include/pico/usb_reset_config.h

					
				@ -0,0 +1,126 @@

				/*

For readability purposes, I wonder if it might make sense to move all these PICO_USB_RESET_BOOTSEL_ settings below PICO_USB_RESET_SUPPORT_RESET_TO_BOOTSEL ? (Since if the latter is set to 0, I guess none of the former settings are applicable?)

For readability purposes, I wonder if it might make sense to move all these `PICO_USB_RESET_BOOTSEL_` settings below `PICO_USB_RESET_SUPPORT_RESET_TO_BOOTSEL` ? (Since if the latter is set to 0, I guess none of the former settings are applicable?)

lurch (Migrated from github.com) reviewed

2026-05-27 04:53:32 -05:00

src/rp2_common/pico_usb_reset/include/pico/usb_reset_tusb.h

					
				@ -0,0 +57,4 @@

				    .reset            = usb_reset_interface_reset,

				    .open             = usb_reset_interface_open,

				    .control_xfer_cb  = usb_reset_interface_control_xfer_cb,

				    .xfer_cb          = usb_reset_interface_xfer_cb,

To match the renaming done in a recent commit, would it also make sense to rename these functions to usb_reset_init etc.? Or does the usb_reset_interface_ naming still make sense here? 🤷

To match the renaming done in a recent commit, would it also make sense to rename these functions to `usb_reset_init` etc.? Or does the `usb_reset_interface_` naming still make sense here? :shrug:

will-v-pi (Migrated from github.com) reviewed

2026-05-27 04:56:36 -05:00

src/rp2_common/pico_usb_reset/include/pico/usb_reset_tusb.h

					
				@ -0,0 +57,4 @@

				    .reset            = usb_reset_interface_reset,

				    .open             = usb_reset_interface_open,

				    .control_xfer_cb  = usb_reset_interface_control_xfer_cb,

				    .xfer_cb          = usb_reset_interface_xfer_cb,

I think it still makes sense here, as these functions are specific to the USB interface that handles the reset

will-v-pi (Migrated from github.com) reviewed

2026-05-27 05:28:38 -05:00

src/rp2_common/pico_usb_reset/usb_reset.c

					
				@ -0,0 +111,4 @@

				            if (request->wValue & (1u << 8)) {

				                active_low = request->wValue & (1u << 7);

				                gpio = request->wValue >> 9u;

				            }

I've swapped them, and replaced 0x100 etc with (1u << 8) to make that clearer as well

I've swapped them, and replaced `0x100` etc with `(1u << 8)` to make that clearer as well

lurch (Migrated from github.com) reviewed

2026-05-27 06:51:46 -05:00

src/rp2_common/pico_usb_reset/include/pico/usb_reset_tusb.h

					
				@ -0,0 +28,4 @@

				  U16_TO_U8S_LE(0x0080), U16_TO_U8S_LE(MS_OS_20_FEATURE_REG_PROPERTY), \

				  U16_TO_U8S_LE(0x0001), U16_TO_U8S_LE(0x0028), /* wPropertyDataType, wPropertyNameLength and PropertyName "DeviceInterfaceGUID" in UTF-16 */ \

				  'D', 0x00, 'e', 0x00, 'v', 0x00, 'i', 0x00, 'c', 0x00, 'e', 0x00, 'I', 0x00, 'n', 0x00, 't', 0x00, 'e', 0x00, \

				  'r', 0x00, 'f', 0x00, 'a', 0x00, 'c', 0x00, 'e', 0x00, 'G', 0x00, 'U', 0x00, 'I', 0x00, 'D', 0x00, 0x00, 0x00, \

Not directly-related to this PR (so feel free to ignore this suggestion), but maybe this could be reformatted to:

  'D', 0,     'e', 0,     'v', 0,     'i', 0,     'c', 0,     'e', 0, \
  'I', 0,     'n', 0,     't', 0,     'e', 0,     'r', 0,     'f', 0, \
  'a', 0,     'c', 0,     'e', 0,     'G', 0,     'U', 0,     'I', 0, \
  'D', 0,     0, 0, \

as this would then match the formatting of the {bc7398c1-... UTF-16 string below?

Not directly-related to this PR (so feel free to ignore this suggestion), but _maybe_ this could be reformatted to: ```suggestion 'D', 0, 'e', 0, 'v', 0, 'i', 0, 'c', 0, 'e', 0, \ 'I', 0, 'n', 0, 't', 0, 'e', 0, 'r', 0, 'f', 0, \ 'a', 0, 'c', 0, 'e', 0, 'G', 0, 'U', 0, 'I', 0, \ 'D', 0, 0, 0, \ ``` as this would then match the formatting of the `{bc7398c1-...` UTF-16 string below?