USB: Use default VID/PID for RIOT-included peripherals #13148
Conversation
The USB subsystem is public API, so there can be, and #12268 adds an example that does access those as an application. bergzand's work on USB audio has not hit the repository AFAICT, but is another likely scenario.
I'm not trying to make this malice-proof; it's all C after all and people can define what they like. The main goal here is to make the user experience smooth for unaware USB users (no blinky-blinky-get-your-own-code just because my board happens to use USB UART) without letting the RIOT VID/PID pair degenerate into "could be anything really as long as it's RIOT based" codes that more resemble the test pairs than something recognizable. For that, USB code blocksenabled by USEMODULE (etc, but especially the choice of a board) need to distinguish themselves from USB code blocks introduced by the user. A define looks like the right choice for me to make that distinction; naming is flexible and could just as well be
Thanks, will work them in with the next batch. |
Ah, so "RIOT" in this case means system internals, not RIOT applications? |
|
Yes. Code that is part of the operating system. So maybe |
|
|
Sounds good, will be in next patch series along with fixes for my stylistic missteps. |
|
Updated, reflecting the style nits and the guard rename. If I may squash, I'll try to make the rest of Travis happy (currently it's hanging at my commit messages, won't get past that without the destructive steps). |
|
Yepp, feel free to squash :-) |
|
(and change the commit message length) |
|
There is also a typo in this commit's message body |
A new define, USB_H_USER_IS_RIOT_INTERNAL, is defined that may only be set from within RIOT's own compilation units that deem themselves standard RIOT peripherals. If all usb.h users in a program match that requirement, a default VID/PID pair is set. Due to the new composite check, the individual checks for VID/PID being set become moot and are removed.
This list is probably incomplete as it was created experimentally.
As the whitelist define can be set per compilation unit in all legitimate cases, the checks do not need to be run on every single usb.h inclusion. This is done for two reaons: * It is sufficient -- if any user C file includes usb.h, there's already a good chance that the user is doing something USB related manualy. (And conversely, the existing examples with boards that happen to pull in CDC-ACM or CDC-ECM do not include usb.h from an example C file). * Defining the USB_H_USER_IS_RIOT around legitimate uses of the header by other headers would allow accidental sidestepping: If a user includes a legitimate usb.h using header (say, board.h) and just forgets to include usb.h on their own, their application that'd mess with USB would still work as usb.h is transitively included, and the check for custom includes does not trigger.
9dac020 to
cca756d
Compare
There was a problem hiding this comment.
ACK. Tested with tests/usbus_cdc_acm_stdio on feather-nrf52840 with the following patch:
diff --git a/tests/usbus_cdc_acm_stdio/Makefile b/tests/usbus_cdc_acm_stdio/Makefile
index 4d07162137..f4f1a13784 100644
--- a/tests/usbus_cdc_acm_stdio/Makefile
+++ b/tests/usbus_cdc_acm_stdio/Makefile
@@ -7,21 +7,4 @@ USEMODULE += shell
USEMODULE += shell_commands
USEMODULE += ps
-# USB device vendor and product ID
-DEFAULT_VID = 1209
-DEFAULT_PID = 7D00
-USB_VID ?= $(DEFAULT_VID)
-USB_PID ?= $(DEFAULT_PID)
-
-CFLAGS += -DCONFIG_USB_VID=0x$(USB_VID) -DCONFIG_USB_PID=0x$(USB_PID)
-
include $(RIOTBASE)/Makefile.include
-
-.PHONY: usb_id_check
-usb_id_check:
- @if [ $(USB_VID) = $(DEFAULT_VID) ] || [ $(USB_PID) = $(DEFAULT_PID) ] ; then \
- $(COLOR_ECHO) "$(COLOR_RED)Private testing pid.codes USB VID/PID used!, do not use it outside of test environments!$(COLOR_RESET)" 1>&2 ; \
- $(COLOR_ECHO) "$(COLOR_RED)MUST NOT be used on any device redistributed, sold or manufactured, VID/PID is not unique!$(COLOR_RESET)" 1>&2 ; \
- fi
-
-all: | usb_id_check$ lsusb | grep 1209
Bus 003 Device 041: ID 1209:7d00 Generic
BTW when removing the same lines in tests/usbus/Makefile, the include via usbdev_mock.c causes the error message to be triggered when the application is compiled, but I guess that is how it is intended.
In file included from /home/mlenders/Repositories/RIOT-OS/RIOT/drivers/include/periph/usbdev.h:78,
from /home/mlenders/Repositories/RIOT-OS/RIOT/tests/usbus/usbdev_mock.c:21:
/home/mlenders/Repositories/RIOT-OS/RIOT/sys/include/usb.h:39:2: error: #error Please configure your vendor and product IDs. For development, you may set CONFIG_USB_VID=0x1209 CONFIG_USB_PID=0x7D01.
#error Please configure your vendor and product IDs. For development, you may \
^~~~~
make[1]: *** [/home/mlenders/Repositories/RIOT-OS/RIOT/Makefile.base:109: /home/mlenders/Repositories/RIOT-OS/RIOT/tests/usbus/bin/particle-xenon/application_tests_usbus/usbdev_mock.o] Error 1
make[1]: *** Waiting for unfinished jobs....
In file included from /home/mlenders/Repositories/RIOT-OS/RIOT/drivers/include/periph/usbdev.h:78,
from /home/mlenders/Repositories/RIOT-OS/RIOT/tests/usbus/main.c:25:
/home/mlenders/Repositories/RIOT-OS/RIOT/sys/include/usb.h:39:2: error: #error Please configure your vendor and product IDs. For development, you may set CONFIG_USB_VID=0x1209 CONFIG_USB_PID=0x7D01.
#error Please configure your vendor and product IDs. For development, you may \
^~~~~
|
Thanks. The removal of the config code is something that I'd like to do in a follow-up, but in one go with unifying The error case around mock is exactly as intended, yes; that does "custom USB stuff" and will need to retain explicit (testing) IDs. |
Contribution description
This adds a check for whether any custom user code (ie. anything but built-in peripherals) uses USB, and if nothing does, allows for 1209/7D00 to be the default VID/PID pair.
It follows the action plan of #12273 (comment), with the simplification that it operates per compilation unit, and thus the ifdef/define combination can be inside the include guard where those things usually are.
Alternatives
If the build system can tell RIOT's compilation units apart from application compilation units or packages, it could set USB_H_USER_IS_RIOT on all modules. That would significantly reduce the number of files this touches. (But then again, any helpers for creating custom HID devices would need to unset the flag again, so meh).
The name USB_H_USER_IS_RIOT is up for bikeshedding.
Testing procedure
usbus_cdc_ecm,auto_init_usbusandauto_init_gnrc_netifmodules without setting CONFIG_USB_VID. (stdio_cdc_acmwould work the same).Issues/PRs references
This is a first step in properly addressing #12273.
A follow-up (preferably in an own PR, but if you prefer I can pull it in here as well) should move the usb_id_check that's currently replicated across examples into common make infrastructure. In the end, the only USB related lines in a USB example makefile should be
USB_VID = 1209/USB_PID = 7D00(or${RIOT_TESTING_[VP]ID}) and the appropriate USEMODULE. Boards that get USB by default (eg. because their default stdio is CDC-ACM, or because gnrc_netdev_default pulls in CEC-ECM) already don't need anything that says "USB" in their Makefiles with this PR.