Skip to content

rp2/tusb_port.c: Allow boards to configure USB VID and PID.#7594

Closed
iabdalkader wants to merge 2 commits intomicropython:masterfrom
iabdalkader:rp2_pid_vid
Closed

rp2/tusb_port.c: Allow boards to configure USB VID and PID.#7594
iabdalkader wants to merge 2 commits intomicropython:masterfrom
iabdalkader:rp2_pid_vid

Conversation

@iabdalkader
Copy link
Copy Markdown
Contributor

No description provided.

@mcauser
Copy link
Copy Markdown
Contributor

mcauser commented Jul 30, 2021

All of the Adafruit RP2 boards are configured with VIDs and PIDs. eg.
https://github.com/adafruit/circuitpython/blob/main/ports/raspberrypi/boards/adafruit_feather_rp2040/mpconfigboard.mk

Add them to this PR too?

@iabdalkader
Copy link
Copy Markdown
Contributor Author

iabdalkader commented Jul 30, 2021

@mcauser Add them how do you mean ?

I only see you have 1 board ADAFRUIT_FEATHER_RP2040 merged and multiple open PRs to add more. I can add USB PID/VID for the ADAFRUIT_FEATHER_RP2040 if you like, then after this is merged you can rebase your PRs.

@dpgeorge Can you please review this, it seems it may save extra PRs later if it's merged first.

@mcauser
Copy link
Copy Markdown
Contributor

mcauser commented Jul 30, 2021

@iabdalkader Inside each mpconfigboard.h file add the board's specific macros:

#define USBD_VID = 0x239A
#define USBD_PID = 0x80F2

I have a bunch of open PRs for other RP2 boards.
If this one gets merged first, I can append the macros to each of my PRs.
Or sort it all out later.

@iabdalkader iabdalkader force-pushed the rp2_pid_vid branch 2 times, most recently from dc5ecb8 to eded97f Compare July 30, 2021 01:48
@iabdalkader
Copy link
Copy Markdown
Contributor Author

@mcauser Added VID/PID for ADAFRUIT_FEATHER_RP2040.

@dpgeorge
Copy link
Copy Markdown
Member

Thanks for this, it's a needed improvement.

But do you think it makes sense to call the config values MICROPY_HW_USBD_VID and MICROPY_HW_USBD_PID, to match other "hardware" based config values? Other ports like stm32 already use USBD_VID and USBD_PID, so they should also (eventually) be changed as well to prefix MICROPY_HW_ if we choose to use these names here.

So the choice is:

  • stick with USBD_VID and USBD_PID
  • use MICROPY_HW_USBD_VID and MICROPY_HW_USBD_PID in this PR, and change other ports to match in a separate PR

@iabdalkader
Copy link
Copy Markdown
Contributor Author

use MICROPY_HW_USBD_VID and MICROPY_HW_USBD_PID in this PR, and change other ports to match in a separate PR

@dpgeorge I choose 2 I think we should stop copying this inconsistency from port to port.

@iabdalkader
Copy link
Copy Markdown
Contributor Author

iabdalkader commented Jul 31, 2021

@dpgeorge Although I think it should be called MICROPY_HW_USB instead of MICROPY_HW_USBD to match all the other USB related macros in rp2, nrf and stm32 ports, and because it's not necessarily a USB device it could be a host.

@dpgeorge
Copy link
Copy Markdown
Member

Although I think it should be called MICROPY_HW_USB instead of MICROPY_HW_USBD to match all the other USB related macros in rp2, nrf and stm32 ports, and because it's not necessarily a USB device it could be a host.

Does a USB host have a VID/PID? A hub will have a VID/PID but that's because it's a device and a host, and it's the device side that has the VID/PID.

Regardless of that, I agree it makes sense to use MICROPY_HW_USB as the prefix because it does match all the other constants. I guess it's just a question of MICROPY_HW_USB_DEV_VID vs MICROPY_HW_USB_VID.

@iabdalkader
Copy link
Copy Markdown
Contributor Author

Does a USB host have a VID/PID? A hub will have a VID/PID but that's because it's a device and a host, and it's the device side that has the VID/PID.

No I don't think so, but macros like MICROPY_HW_USB_HS can be used for a host or a device. But if only a device would use them, then there's no need to make the distinction between host/device, otherwise all the other macros should also be changed to MICROPY_HW_USB_DEV_ or MICROPY_HW_USBD_. Alternatively we could use MICROPY_HW_USB_HOST_ when the distinction is needed. Either way is fine by me as long as they're consistent in board files and across ports.

@dpgeorge
Copy link
Copy Markdown
Member

dpgeorge commented Aug 1, 2021

... otherwise all the other macros should also be changed to MICROPY_HW_USB_DEV_ or MICROPY_HW_USBD_. Alternatively we could use MICROPY_HW_USB_HOST_ when the distinction is needed.

Ok, yes, let's leave it as MICROPY_HW_USB_VID etc.

@dpgeorge
Copy link
Copy Markdown
Member

dpgeorge commented Aug 1, 2021

Merged in 7ae9e6e and 23e2e00

@dpgeorge
Copy link
Copy Markdown
Member

dpgeorge commented Aug 1, 2021

See follow up #7603.

@iabdalkader iabdalkader deleted the rp2_pid_vid branch July 17, 2022 11:08
tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Feb 18, 2023
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.

3 participants