usb: Warn on test-ID usage in a unified location

[chrysn]

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]

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]

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.

[leandrolanzieri]

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

[chrysn]

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.)

[leandrolanzieri]

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?

[chrysn]

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?

[leandrolanzieri]

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.

[benpicco]

Needs a rebase.

[chrysn]

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]

Looks like Murdock is not so happy about those warnings.

[chrysn]

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]

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]

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]

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

[miri64]

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

Ping @chrysn

[fjmolinas]

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

[chrysn]

Thanks for the ping, I'm at it.

[chrysn]

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]

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]

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.