usb: Warn on test-ID usage in a unified location by chrysn · Pull Request #13382 · RIOT-OS/RIOT

chrysn · 2020-02-14T12:06:54Z

Contribution description

As discussed in #12273 and prepared in #13148, there is still a lot of Makefile code that'd get copy-pasted around when applications are started from examples, or (worse) simply removed because it's annoying.

This moves the checks for whether to show a big red warning about using test VID/PIDs into the main build infrastructure.

In particular (quoting the commit), this

renames DEFAULT_xID to USB_xID_TESTING as it is not really a default
(if anyting, the 7D00 is, and it's not that)
moves the check into Makefile
generalizes the check to all test PID/VID pairs
- in doing so, fixes the "or" (which would have ruled out warning-free
  use of an allocated pid.codes number), and compares to the actual
  testing PID rather than the RIOT-peripheral PID
removes all occurrences of duplicated checks in examples or tests,
leaving definitions only where they are needed
moves the Kconfig defaults of the usbus_minimal example into the main
Kconfig, as these are good defaults for all cases when USB is enabled
manually

Open issues

Setting variables currently take two independent paths, they either come from the environment and are set (manually still, I'd pull that over when the rest is clear) to CFLAGS, or they come from Kconfig with defines in CFLAGS and also set a CONFIG_USB_VID variable in addition to the define.

The Kconfig-provided CFLAGS are hard to check, so I'd check the CONFIG_USB_VID, but then that'd be another checking path relative to the Makefile-based configuration.

It'd be tempting to change the USB_VID settings in the makefile-based configuration to CONFIG_USB_VID (as it's exported by Kconfig), but then there'd be a general CFLAGS addition that adds in those, and Kconfig's settings wind up there twice.

Is there a general pattern that has been established for such cases?
The code in USB: Use default VID/PID for RIOT-included peripherals #13148 fails to detect cases where the 1209:7D00 is set manually, which allows applications to claim that they only use RIOT internal peripherals even though they don't.

I'd like to add a check for this, but am not sure yet how sever it should be (red warning vs breaking), and whether to test in Makefile or in the C header.

Testing procedure

USB examples that only enable RIOT-provided devices (tests/usbus_cdc_acm, tests/usbus_cdc_ecm) build without any USB specific code in their Makefile, and show as 1209:7D00; they build without showing a warning.
USB examples that actually do something with USB (tests/usbus) set their IDs to in two single lines and then show the warning at build time.
USB examples that opt in to testing IDs (examples/usbus_minimal) can set them and get the warnings without actually copying the warning around.
USB examples with explicit IDs err out when 1209/7D00 is given explicitly, as needing to set that would be against that code's description.

Issues/PRs references

Closes: #12273

[edit: Added last testing point]

chrysn · 2020-02-14T12:51:11Z

After some playing around with Kconfig, I've updated the PR to use the conversion from the example unconditionally.

This might be a good point to think about how we want to deal with conflict between Kconfig and Makefile-based configuration, whether we want to, or whether we'll just let it wind up in conflicting CFLAGS or overridden variables like other such conflicts would currently. (Or not, and think about it later, but come back to this particular piece of code).

chrysn · 2020-02-14T13:03:12Z

I've added the check originally listed under "open issues" to the makefiles -- gives output that is readable more easily than an #error.

The #error from not having a VID/PID set was updated to reflect the new environment variables; when Kconfig is more widespread / accepted, it could also mention how to do that in Kconfig.

sys/usb/Kconfig

Makefile.include

leandrolanzieri · 2020-02-17T08:26:32Z

sys/include/usb.h

 #else
 #error Please configure your vendor and product IDs. For development, you may \
-    set CONFIG_USB_VID=0x1209 CONFIG_USB_PID=0x7D01.
+    set USB_VID=${USB_VID_TESTING} USB_PID=${USB_PID_TESTING}.


Isn't this still CONFIG_USB_VID=${USB_VID_TESTING} CONFIG_USB_PID=${USB_PID_TESTING}?

When IDs are set in makefiles, currently (by rough convention in the examples, it's not like there is mechanism for that), they are set as USB_xID make variables, and later set as -DCONFIG_USB_xID=${USB_xID}.

Could certainly rename them to CONFIG_USB_xID in the makefile, but then both make and Kconfig would write the same variable. That's easier in some parts, but then what would the condition be on which the makefile decides on whether to set CFLAGS += -DCONFIG_USB_xID=${CONFIG_USB_xID}? (As I understand, Kconfig sets that automatically -- but when Kconfig is not in use, it'll still need manual exporting.)

But it seems a bit odd to me that an error message that is present on one of our header files refers to a Makefile variable. As I see it, the message is indicating that you may set CONFIG_USB_xID macros to the testing values. Also, I guess we could actually hard-code the testing values here as they are not likely to change?

The error message tries to give concrete guidance as to what to do; previously that would have been setting something in the Makefile; currently it should be "configure however you configure your things", which is harder to express concisely.

I suppose this is easier when there is a unified name CONFIG_USB_xID for both configuration ways, as "set CONFIG_USB_PID=7D01" can be understood both in a Makefile and a Kconfig context -- but for that I'd need a criterion based on which to decide whether a CFLAGS += -DCONFIG_USB_PID=... is indicated. What can that criterion be?

If we just indicate that the macros CONFIG_USB_xID need to be configured, then the user can decide the way to do that, I don't know if it is up to the error message to define this.

But if we do want to give a 'concrete guidance', then this is fine, as CFLAGS is still the default configuration way for now.

See-Also: RIOT-OS#13382 (comment)

benpicco · 2020-04-03T10:01:24Z

Needs a rebase.

See-Also: RIOT-OS#13382 (comment)

chrysn · 2020-04-03T10:12:35Z

Needs a rebase.

Done (I guess I had that coming); still running some checks to see there were no conflicts, but so far it looks good.

benpicco · 2020-04-03T10:47:07Z

Looks like Murdock is not so happy about those warnings.

chrysn · 2020-04-03T11:12:35Z

Looks like Murdock is not so happy about those warnings.

It's not the warning, it's the errors (at least where I checked) -- and they are justified, in that I did not capture all include locations for all boards. I'll start a local build on all boards to see whether I capture the relevant code paths before I bother Murdock again.

chrysn · 2020-04-07T11:01:12Z

Hrmpf -- unlike the platforms I previously tested this with, the complaining ones include usb.h in their generic headers. That's not completely unforeseen, but the first results had given me hope that it could be avoided.

I'll add the suitable defines around those inclusions, but it does have a subtle downside: If an application uses functions from usb.h but doesn't include it manually (because it only includes them via the board files), it will silently circumvent the protection. I'd say that's good enough; the only alternative I see is setting the USB_H_USER_IS_INTERNAL flag from the build system on all internal compilation units.

chrysn · 2020-05-03T14:18:13Z

With benpicco's discovery that the USB include through the general periph headers is not essential (cherry-picked from #12778), this should now be in a more useful state.

benpicco · 2020-05-03T14:20:07Z

Want to rebase & solve the merge conflict so we can see what Murdock thinks of this now? 🙂️

chrysn · 2020-05-03T15:13:52Z

Oups, a merge can't be processed by travis ... rebasing.

See-Also: RIOT-OS#13382 (comment)

fjmolinas · 2020-06-02T09:10:28Z

This one needs a rebase again @chrysn, I can test right after the rebase. The change is a nice cleanup. @leandrolanzieri was your comment answered?

miri64 · 2020-06-25T14:39:43Z

This one needs a rebase again @chrysn, […]

Ping @chrysn

fjmolinas · 2020-06-30T08:20:41Z

@chrysn I can rebase and push if you are short on time :)

chrysn · 2020-06-30T08:35:38Z

Thanks for the ping, I'm at it.

This * renames DEFAULT_xID to USB_xID_TESTING as it is not really a default (if anyting, the 7D00 is, and it's not that) * moves the check into Makefile * generalizes the check to all test PID/VID pairs * in doing so, fixes the "or" (which would have ruled out warning-free use of an allocated pid.codes number), and compares to the actual testing PID rather than the RIOT-peripheral PID * removes all occurrences of duplicated checks in examples or tests, leaving definitions only where they are needed * moves the Kconfig defaults of the usbus_minimal example into the main Kconfig, as these are good defaults for all cases when USB is enabled manually Closes: RIOT-OS#12273

This allows the check for test IDs to run independently of the configuration source, and provides a canonical point for the configurable (and tested) Makefile variable to enter CFLAGS.

That pair is reserved for cases when it can be set implicitly by the build system. The check could just as well be done in sys/include/usb.h, but this gives prettier output.

The clause was left over from moving the lines into the main configuration from an example config without understanding its relevance.

See-Also: RIOT-OS#13382 (comment)

chrysn · 2020-06-30T09:01:50Z

I've pushed a rebased branch for Murdock to have a look at, but I'm right now baffled at one of the tests failing (the usbus_minimal example builds even when no VID/PID is set, which it shouldn't), looking into that.

chrysn · 2020-06-30T09:27:34Z

All fine from my tests -- I had just mixed up the tests/usbus and examples/usbus_minimal case. The latter really does opt in because it does nothing custom and is thus eligible for the 7D00 code -- but chooses to appear custom for the benefit of people starting from that example.

fjmolinas · 2020-06-30T11:22:43Z

Testing procedure:

USB examples that only enable RIOT-provided devices (tests/usbus_cdc_acm, tests/usbus_cdc_ecm) build without any USB specific code in their Makefile, and show as 1209:7D00; they build without showing a warning.

[15710.251361] usb 4-1.5.5: new full-speed USB device number 23 using xhci_hcd
[15710.357191] usb 4-1.5.5: New USB device found, idVendor=1209, idProduct=7d00, bcdDevice= 0.00
[15710.357194] usb 4-1.5.5: New USB device strings: Mfr=3, Product=2, SerialNumber=0

USB examples that actually do something with USB (tests/usbus) set their IDs to in two single lines and then show the warning at build time.

 BOARD=samr21-xpro make -C tests/usbus flash -j3
Building application "tests_usbus" for "samr21-xpro" with MCU "samd21".

Private testing pid.codes USB VID/PID used!, do not use it outside of test environments!
MUST NOT be used on any device redistributed, sold or manufactured, VID/PID is not unique!

USB examples that opt in to testing IDs (examples/usbus_minimal) can set them and get the warnings without actually copying the warning around.

BOARD=samr21-xpro make -C examples/usbus_minimal/ flash -j3
make: Entering directory '/home/francisco/workspace/RIOT/examples/usbus_minimal'
Building application "usbus_minimal" for "samr21-xpro" with MCU "samd21".

Private testing pid.codes USB VID/PID used!, do not use it outside of test environments!
MUST NOT be used on any device redistributed, sold or manufactured, VID/PID is not unique!

USB examples with explicit IDs err out when 1209/7D00 is given explicitly, as needing to set that would be against that code's description.

 USB_PID=7d00 BOARD=samr21-xpro make -C examples/usbus_minimal/ flash -j3
make: Entering directory '/home/francisco/workspace/RIOT/examples/usbus_minimal'
Building application "usbus_minimal" for "samr21-xpro" with MCU "samd21".

RIOT standard peripherals code (1209/7D00) cannot be set explicitly.
Unset USB_VID / USB_PID for the code to be picked automatically, or set
them to ${USB_VID_TESTING} / ${USB_PID_TESTING} during development.

fjmolinas

ACK, nice cleanup, tested all look OK.

See-Also: RIOT-OS#13382 (comment)

chrysn added Area: Kconfig Area: Kconfig integration Area: USB Area: Universal Serial Bus Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Feb 14, 2020

benpicco requested review from bergzand and leandrolanzieri February 14, 2020 14:50

leandrolanzieri reviewed Feb 17, 2020

View reviewed changes

chrysn requested review from MichelRottleuthner, MrKevinWeiss, aabadie, cgundogan, fjmolinas, jia200x, miri64 and smlng as code owners February 17, 2020 08:37

chrysn added a commit to chrysn-pull-requests/RIOT that referenced this pull request Feb 18, 2020

usb Kconfig: Provide explicit defaults

7825077

See-Also: RIOT-OS#13382 (comment)

chrysn mentioned this pull request Apr 3, 2020

usbus: allow to set CONFIG_USB_PID/CONFIG_USB_VID by board #13652

Closed

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 3, 2020

chrysn force-pushed the usb-check-unification branch from 7825077 to 123288c Compare April 3, 2020 10:12

chrysn added a commit to chrysn-pull-requests/RIOT that referenced this pull request Apr 3, 2020

usb Kconfig: Provide explicit defaults

123288c

See-Also: RIOT-OS#13382 (comment)

chrysn requested review from DipSwitch and vincent-d as code owners May 3, 2020 14:16

chrysn added a commit to chrysn-pull-requests/RIOT that referenced this pull request May 3, 2020

usb Kconfig: Provide explicit defaults

5435943

See-Also: RIOT-OS#13382 (comment)

chrysn force-pushed the usb-check-unification branch from fbc75a6 to 78c56cf Compare May 3, 2020 15:16

benpicco mentioned this pull request Jun 2, 2020

cpu/stm32: don't include usbdev_stm32.h in periph_cpu_common.h #14174

Merged

fjmolinas self-assigned this Jun 2, 2020

chrysn added 7 commits June 30, 2020 10:51

usb: Move Kconfig/Makefile merging into main Makefile.include

346093c

This allows the check for test IDs to run independently of the configuration source, and provides a canonical point for the configurable (and tested) Makefile variable to enter CFLAGS.

usb: Check against explicit use of 1209/7D00 VID/PID pair

a1725b0

That pair is reserved for cases when it can be set implicitly by the build system. The check could just as well be done in sys/include/usb.h, but this gives prettier output.

usb: Adapt error message to newly present build variables

c0cc03a

usb: Use separate Makefile for configuration and checks

a181c0d

usb Kconfig: Remove conditional

fbd7b66

The clause was left over from moving the lines into the main configuration from an example config without understanding its relevance.

usb Kconfig: Provide explicit defaults

85d7042

See-Also: RIOT-OS#13382 (comment)

chrysn force-pushed the usb-check-unification branch from 78c56cf to 85d7042 Compare June 30, 2020 09:00

fjmolinas approved these changes Jun 30, 2020

View reviewed changes

fjmolinas merged commit 1004fb3 into RIOT-OS:master Jun 30, 2020

miri64 added this to the Release 2020.07 milestone Jun 30, 2020

chrysn deleted the usb-check-unification branch July 2, 2020 05:10

bergzand pushed a commit to bergzand/RIOT that referenced this pull request Jul 15, 2020

usb Kconfig: Provide explicit defaults

e76fe19

See-Also: RIOT-OS#13382 (comment)

jia200x pushed a commit to jia200x/RIOT that referenced this pull request Jul 20, 2020

usb Kconfig: Provide explicit defaults

a47c915

See-Also: RIOT-OS#13382 (comment)

leandrolanzieri mentioned this pull request Nov 4, 2020

tests/usbus_cdc_ecm: remove unneeded Kconfig file #15379

Merged

jia200x pushed a commit to jia200x/RIOT that referenced this pull request Mar 9, 2021

usb Kconfig: Provide explicit defaults

753aeab

See-Also: RIOT-OS#13382 (comment)

chrysn mentioned this pull request Nov 18, 2021

usbus/msc: add initial support for Mass Storage Class #15941

Closed

Conversation

chrysn commented Feb 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Open issues

Testing procedure

Issues/PRs references

Uh oh!

chrysn commented Feb 14, 2020

Uh oh!

chrysn commented Feb 14, 2020

Uh oh!

Uh oh!

Uh oh!

leandrolanzieri Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

chrysn Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri Feb 18, 2020

Choose a reason for hiding this comment

Uh oh!

chrysn Feb 18, 2020

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

benpicco commented Apr 3, 2020

Uh oh!

chrysn commented Apr 3, 2020

Uh oh!

benpicco commented Apr 3, 2020

Uh oh!

chrysn commented Apr 3, 2020 via email

Uh oh!

chrysn commented Apr 7, 2020

Uh oh!

chrysn commented May 3, 2020

Uh oh!

benpicco commented May 3, 2020

Uh oh!

chrysn commented May 3, 2020

Uh oh!

fjmolinas commented Jun 2, 2020

Uh oh!

miri64 commented Jun 25, 2020

Uh oh!

fjmolinas commented Jun 30, 2020

Uh oh!

chrysn commented Jun 30, 2020 via email

Uh oh!

chrysn commented Jun 30, 2020

Uh oh!

chrysn commented Jun 30, 2020

Uh oh!

fjmolinas commented Jun 30, 2020

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chrysn commented Feb 14, 2020 •

edited

Loading