Skip to content

USB VID/PID: allow for boards to define default custom VID/PID#13865

Merged
aabadie merged 2 commits intoRIOT-OS:masterfrom
benpicco:usb_custom_id
Jun 2, 2020
Merged

USB VID/PID: allow for boards to define default custom VID/PID#13865
aabadie merged 2 commits intoRIOT-OS:masterfrom
benpicco:usb_custom_id

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented Apr 14, 2020 •
edited
Loading

Contribution description

For boards that have terminals in their shipped configurations it makes sense to reuse those VID/PIDs when providing a terminal in RIOT as well.

That affects the builtin-peripherals IDs and not the custom specified ones.

Taken from #13652 (comment)

Testing procedure

Set RIOT_INTERNALPERIPHERAL_VID and RIOT_INTERNALPERIPHERAL_PID for a board.
RIOT should use those IDs now and not spit out warnings when those IDs are used for stdio over USB.

Issues/PRs references

alternative to #13652

@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: USB Area: Universal Serial Bus labels Apr 14, 2020
@benpicco benpicco requested review from bergzand and chrysn April 14, 2020 22:04
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 21, 2020
@benpicco benpicco force-pushed the usb_custom_id branch 2 times, most recently from 27e82b0 to 41657eb Compare April 21, 2020 08:39
@benpicco benpicco mentioned this pull request May 1, 2020
Merged
@benpicco benpicco added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label May 1, 2020
@benpicco benpicco requested a review from fjmolinas May 3, 2020 12:38
@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Jun 2, 2020

Can't this already be done by setting CONFIG_USB_VID? Why the need for the extra define?

@fjmolinas fjmolinas self-assigned this Jun 2, 2020
@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Jun 2, 2020

Can't this already be done by setting CONFIG_USB_VID? Why the need for the extra define?

Sorry hadn't checked the discussion.

fjmolinas
fjmolinas reviewed Jun 2, 2020
View reviewed changes
@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Jun 2, 2020

Set RIOT_INTERNALPERIPHERAL_VID and RIOT_INTERNALPERIPHERAL_PID for a board.
RIOT should use those IDs now and not spit out warnings when those IDs are used for stdio over USB.

Are you talking about makefile warnings? Or other warnings? Makefile warnings are still present with this PR.

Otherwise I was able to override the definition on a samr21-xpro:

[10052.798514] cdc_acm 1-1.2.3:1.0: ttyACM1: USB ACM device
[10107.234803] usb 1-1.2.3: USB disconnect, device number 24
[10113.342776] usb 1-1.2.3: new full-speed USB device number 25 using xhci_hcd
[10113.445219] usb 1-1.2.3: New USB device found, idVendor=1309, idProduct=4400, bcdDevice= 0.00
[10113.445229] usb 1-1.2.3: New USB device strings: Mfr=3, Product=2, SerialNumber=0
[10113.445234] usb 1-1.2.3: Product: USB device
[10113.445239] usb 1-1.2.3: Manufacturer: RIOT-os.org
[10113.455652] cdc_acm 1-1.2.3:1.0: ttyACM1: USB ACM device

@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jun 2, 2020 •
edited
Loading

Are you talking about makefile warnings? Or other warnings? Makefile warnings are still present with this PR.

I only later found out that the warnings were actually caused by #14174, so this is just for cosmetics.

But if you use a VID/PID outside the RIOT internal use space, there should be no warnings.

@benpicco benpicco removed the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Jun 2, 2020
fjmolinas
fjmolinas reviewed Jun 2, 2020
View reviewed changes
sys/include/usb.h
/* These can be overridden by boards that should come up with their board
* supplier VID/PID pair. Boards should only override this if the RIOT built-in
* peripherals are compatible with whatever is usually shipped with that pair
* */
Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas Jun 2, 2020

Choose a reason for hiding this comment

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

Nit-picking here but does the variable name make sense? If its overridne then its actually not RIOTs PID but the board or manufacturer right? Would it make more sense to just have INTERNAL_PERIPHERAL_PID (If my understanding of the variable meaning is correct)

@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Jun 2, 2020

I only later found out that the warnings were actually caused by #14174, so this is just for cosmetics.

Murdock warnings are fixed with #14174, so this one is waiting for it.

@fjmolinas fjmolinas added State: waiting for other PR State: The PR requires another PR to be merged first CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 2, 2020
@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Jun 2, 2020

Warning should be fixed now.

@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Jun 2, 2020

@benpicco do you agree with this PR as a prefered alternative to #13652?

@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jun 2, 2020

It touches less code, so that alone makes it preferable in my book.

fjmolinas
fjmolinas approved these changes Jun 2, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

ACK, please squash!

benpicco and others added 2 commits June 2, 2020 14:17
@benpicco @chrysn
USB VID/PID: allow for boards to define default custom VID/PID
bc12d2e 
For boards that have terminals in their shipped configurations it makes sense
to reuse those VID/PIDs when providing a terminal in RIOT as well.

That affects the builtin-peripherals IDs and not the custom specified ones.

Co-authored-by: chrysn <[email protected]>
@benpicco
boards/serpente: properly configure USB ID
cb21aea
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Jun 2, 2020

I also moved the configuration for serpente to the proper place now, looks like it's working:

[20299.302432] usb 2-1.5.3.1: new full-speed USB device number 21 using ehci-pci
[20299.413148] usb 2-1.5.3.1: New USB device found, idVendor=239a, idProduct=0057, bcdDevice= 0.00
[20299.413150] usb 2-1.5.3.1: New USB device strings: Mfr=3, Product=2, SerialNumber=0
[20299.413151] usb 2-1.5.3.1: Product: USB device
[20299.413152] usb 2-1.5.3.1: Manufacturer: RIOT-os.org
[20299.413741] cdc_acm 2-1.5.3.1:1.0: ttyACM3: USB ACM device

@aabadie aabadie merged commit 954ac22 into RIOT-OS:master Jun 2, 2020
@benpicco benpicco deleted the usb_custom_id branch June 2, 2020 18:25
@miri64 miri64 added this to the Release 2020.07 milestone Jun 24, 2020
@chrysn chrysn mentioned this pull request Mar 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fjmolinas fjmolinas fjmolinas approved these changes

@chrysn chrysn Awaiting requested review from chrysn

@bergzand bergzand Awaiting requested review from bergzand

Assignees

@fjmolinas fjmolinas

Labels

Area: USB Area: Universal Serial Bus CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2020.07

Development

Successfully merging this pull request may close these issues.

4 participants

@benpicco @fjmolinas @miri64 @aabadie