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

usb: Refactor some TinyUSB code from rp2 port to shared/tinyusb. #9816

Closed
wants to merge 3 commits into from

Conversation

projectgus
Copy link
Contributor

Testing

Have tested these permutations:

  • Default rp2 PICO board config with USB CDC REPL.
  • CDC & MSC (MICROPY_HW_USB_MSC 1).
  • MSC only (MICROPY_HW_USB_MSC 1, MICROPY_HW_USB_CDC 0).
  • USB device, but no CDC & no MSC (MICROPY_HW_USB_MSC 0, MICROPY_HW_USB_CDC 0). This creates a device that reports a single configuration with no interfaces, currently not very useful!
  • No USB (MICROPY_HW_ENABLE_USBDEV 0 and MICROPY_HW_ENABLE_UART_REPL 1).

All seems to work as expected.

@projectgus
Copy link
Contributor Author

@dpgeorge As promised! @jimmo PTAL as well if you have time.

ports/rp2/mpconfigport.h Outdated Show resolved Hide resolved
#if MICROPY_HW_ENABLE_USBDEV

#ifndef MICROPY_HW_USB_STR_MANUF
#define MICROPY_HW_USB_STR_MANUF "MicroPython"
Copy link
Member

Choose a reason for hiding this comment

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

This is MICROPY_HW_USB_MANUFACTURER_STRING in the stm32 port... we should make them the same. I do prefer having STR before the string name, rather than _STRING at the very end. And then I would prefer the full word "MANUFACTURER" because "MANUF" could be confusing to a newcomer.

Well, whatever we go with, it should be consistent across ports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dpgeorge Got it. Looks like I saw this name from mimxrt port, where it was added in commit b73073d.

I'll change to MICROPY_HW_USB_MANUFACTURER_STRING for now. If the mimxrt port changes to use this header in the future, we can always add a compatibility #ifdef to pick up the old macro name.

shared/tinyusb/usbd.h Outdated Show resolved Hide resolved
@@ -2,6 +2,7 @@
* The MIT License (MIT)
*
* Copyright (c) 2020-2021 Damien P. George
* Copyright (c) 2022 Blake W. Felt
Copy link
Member

Choose a reason for hiding this comment

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

@Molorius For this file I'm not sure it's correct to add your copyright. This file was simply moved from one location to another, so there's not much added IP.

Do you agree to the removal of this copyright line?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. If I remember correctly I initially had more functions in here which is why I added my name.

@projectgus
Copy link
Contributor Author

@dpgeorge Rebased and made some updates including renaming some of the source files, let me know what you think.

App the mp_ prefix to usbd_ symbols and files which are defined here and
not in TinyUSB.

rp2 only for now. This includes some groundwork for dynamic USB
devices (defined in Python).

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
Requires changing the USB-CDC stdin/stdout guards from
MICROPY_HW_ENABLE_USBDEV to the new (in this port)
MICROPY_HW_USB_CDC.
@projectgus
Copy link
Contributor Author

Additional change to allow building rp2 port with USB device enabled but USB-CDC interface driver disabled.

@dpgeorge I can easily put this commit into a separate PR if it's too much to review in one hit, as well.

@robert-hh
Copy link
Contributor

Should I change MICROPY_HW_USB_STR_MANUF to MICROPY_HW_USB_MANUFACTURER_STRING in the mimxrt port. At the moment, these are just local to the mimxrt directory. So it's not urgent and I can put that into the next service pack.

@dpgeorge
Copy link
Member

dpgeorge commented Nov 9, 2022

Should I change MICROPY_HW_USB_STR_MANUF to MICROPY_HW_USB_MANUFACTURER_STRING in the mimxrt port

Yes, that would make it consistent with this PR and stm32.

@robert-hh
Copy link
Contributor

Ok. Done.

@dpgeorge
Copy link
Member

Merged in eed4eb2 through c8913fd with the following changes:

  • removed a copyright line as discussed above
  • renamed header include guards to have SHARED_TINYUSB in them, instead of LIB_UTILS
  • reverted enabling of MICROPY_HW_ENABLE_UART_REPL (I assume it was done just for testing and accidentally left enabled)

Thanks @projectgus !

@projectgus
Copy link
Contributor Author

reverted enabling of MICROPY_HW_ENABLE_UART_REPL (I assume it was done just for testing and accidentally left enabled)

Oops, yes indeed! Thanks for cleaning those parts up, @dpgeorge.

@projectgus projectgus deleted the refactor/common_usb branch November 11, 2022 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants