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

Add stdio_usb reset interface detection for custom VendorID / ProductID setups #83

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

tomas-pecserke
Copy link

@tomas-pecserke tomas-pecserke commented May 24, 2023

It would be really nice to use picotool with projects using custom VID/PID as well.

With the new Pico SDK 1.5.0 having improved compatibility of stdio_usb with TinyUSB it's easy to implement stdio_usb reset feature with custom USB setup.

With this patch I am able to reboot the Pico running a TinyUSB stack into BOOTSEL mode and upload new firmware automatically.

That been said I still have several issues I'd like some feedback on:

  1. With non-standard VID / PID supported, it would be helpful to add device filters based on (at least) these attributes:
  2. User might want to have the CLI option to enable/disable this feature. ✔️ Implemented
  3. UDEV rule based on VENDOR interface and stdio_usb reset interface subclass and protocol might be nice, but the attributes required seem to be very distribution dependent. This prevents definition of reliable rule to apply in all situations. As this is advanced use case anyway, additional step required to allow specific user's device is acceptable. ❌ Will not implement

Example use case:
I would like to create automated upload cmake target for a Pico project:

# Custom command to upload firmware to Raspberry Pi Pico
add_custom_target(upload_firmware
    COMMAND ${PICOTOOL_EXECUTABLE} load -x ${TARGET_NAME}.uf2 --detect-reset-interface -f
    DEPENDS ${TARGET_NAME}
    COMMENT "Uploading firmware to Raspberry Pi Pico"
)

I also implemented a tiny library that simplyfies the implementation of custom TinyUSB reset interface -
pico_tusb_reset_interface.

@lurch
Copy link
Contributor

lurch commented May 24, 2023

Just to "keep the dots connected", #54 is also requesting filtering by Serial ID.

@tomas-pecserke tomas-pecserke changed the base branch from master to develop June 3, 2023 18:24
@tomas-pecserke tomas-pecserke force-pushed the feature/stdio_usb_reset_interface_detetion branch from 6ed0a3f to b203309 Compare June 3, 2023 23:53
README.md Outdated Show resolved Hide resolved
@tomas-pecserke tomas-pecserke force-pushed the feature/stdio_usb_reset_interface_detetion branch from 83ad4f0 to a25aa9a Compare June 5, 2023 08:41
main.cpp Outdated
@@ -2276,19 +2283,25 @@ int main(int argc, char **argv) {
#if defined(_WIN32)
printer(dr_vidpid_stdio_usb,
" appears to be a RP2040 device with a USB serial connection, not in BOOTSEL mode. You can force reboot into BOOTSEL mode via 'picotool reboot -f -u' first.");
printer(dr_reset_interface,
" appears to be a RP2040 device with a USB reset interface, not in BOOTSEL mode. You can force reboot into BOOTSEL mode via 'picotool reboot -f -u' first.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the example picotool command-line here also include the -i flag?

Copy link
Author

Choose a reason for hiding this comment

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

Right. The users will only see this message, if they already are using '-i' option. Do we want to promote the possibility of using the option similar to how we promote use of -f option?

main.cpp Show resolved Hide resolved
@tomas-pecserke tomas-pecserke force-pushed the feature/stdio_usb_reset_interface_detetion branch 2 times, most recently from a5d9964 to c5128a9 Compare June 5, 2023 15:24
@tomas-pecserke
Copy link
Author

@lurch if you are interested, pls have a look at a companion library implementing the reset interface for custom TinyUSB stack:
https://github.com/tomas-pecserke/pico_tusb_reset_interface

@lurch
Copy link
Contributor

lurch commented Jun 5, 2023

I'm not the picotool maintainer (that's @kilograham ), I just try to leave helpful comments for the little problems that I happen to spot 🙂

@lurch
Copy link
Contributor

lurch commented Jun 5, 2023

@lurch if you are interested, pls have a look at a companion library implementing the reset interface for custom TinyUSB stack: https://github.com/tomas-pecserke/pico_tusb_reset_interface

Hmmm, not sure if it's wise to say "picotool supports custom devices with compatible reset interface with use of --detect-reset-interface or -i option.", if this picotool PR hasn't even been merged yet? Probably best to just add a link to this PR for the time being? 🤔

@tomas-pecserke
Copy link
Author

tomas-pecserke commented Jun 5, 2023

Sorry about that. I did not mean to impose on you. I appreciate any effort you choose to put in. But I'd hate for you to feel pressured or obligated in any way.

Thank you for your input. I find it very helpful. And you are quite correct, I have definitely jumped the gun on this one.

@tomas-pecserke tomas-pecserke force-pushed the feature/stdio_usb_reset_interface_detetion branch 3 times, most recently from d931c70 to a15e57e Compare June 7, 2023 11:00
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.

3 participants