Skip to content

Conversation

benpicco
Copy link
Contributor

Contribution description

Some boards already have a unique PID:VID combination assigned to them.
When running RIOT on these boards, we should keep this ID combination.

Thus allow to set CONFIG_USB_VID and CONFIG_USB_PID by the board Makefile.include.

Tests and examples have to be adapted so they don't set CFLAGS themselves anymore.

Testing procedure

All boards that don't have a unique VID:PID should still use the Riot ID.
But boards can now specify their own ID.

Issues/PRs references

Will be used by #12778

Some boards already have a unique PID:VID combination assigned to them.
When running RIOT on these boards, we should keep this ID combination.

Thus allow to set `CONFIG_USB_VID` and `CONFIG_USB_PID` by the board
`Makefile.include`.

Tests and examples have to be adapted so they don't set CFLAGS themselves anymore.
@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: boards Area: Board ports Area: sys Area: System labels Mar 18, 2020
@miri64 miri64 requested a review from chrysn March 19, 2020 09:50
@chrysn
Copy link
Member

chrysn commented Mar 19, 2020

Could you elaborate on / provide pointers to how a board can have its own VID/PID pair? After all, it's the software running on a board (possibly in combination with the hardware) that determines the PID, not the board on its own.

@benpicco
Copy link
Contributor Author

benpicco commented Mar 19, 2020

AFAIU VID:PID pairs are assigned to hardware, not software.
e.g. the Serpente board has 239A:0057 from the realm of Adafruit Industries LLC. (It comes with a firmware that has this ID, but it's a development board where you are expected to run your own software on)

@chrysn
Copy link
Member

chrysn commented Apr 3, 2020

My understanding so far was that it's quite important to have different IDs for different firmwares, for if you connect a thing with one VID/PID pair to Windows and it implements a mouse, and next time you flash it with the same pair as a block device, things don't work. (Could have been old bugs, but that's what the custom ID assignments ask one to consider).

... but if the concrete board vendors insist on this, so be it.

The other part is where the configuration happens -- there's already #13382 that changes this, would you consider checking up on that and possibly rebasing this on that?

@chrysn
Copy link
Member

chrysn commented Apr 3, 2020

Summarizing an IRC discussion, I understand that the use case is for boards that have terminals in their shipped configurations to reuse those IDs when providing a terminal in RIOT as well.

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

I'd suggest that instead of setting the CONFIG_USB_VID per board, the values used for built-in peripherals be overridable:

diff --git a/sys/include/usb.h b/sys/include/usb.h
index 92dc6eaf7d..efaf94c57e 100644
--- a/sys/include/usb.h
+++ b/sys/include/usb.h
@@ -24,6 +24,15 @@
 extern "C" {
 #endif
 
+/* 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
+ * */
+#ifndef RIOT_INTERNALPERIPHERAL_VID
+#define RIOT_INTERNALPERIPHERAL_VID (0x1209)
+#define RIOT_INTERNALPERIPHERAL_PID (0x7D00)
+#endif
+
 /**
  * @defgroup usb_conf USB peripheral compile time configurations
  * @ingroup config
@@ -33,8 +42,8 @@ extern "C" {
 #if !(defined(CONFIG_USB_VID) && defined(CONFIG_USB_PID))
 #ifdef USB_H_USER_IS_RIOT_INTERNAL
 /* Reserved for RIOT standard peripherals as per http://pid.codes/1209/7D00/ */
-#define CONFIG_USB_VID (0x1209)
-#define CONFIG_USB_PID (0x7D00)
+#define CONFIG_USB_VID RIOT_INTERNALPERIPHERAL_VID
+#define CONFIG_USB_PID RIOT_INTERNALPERIPHERAL_PID
 #else
 #error Please configure your vendor and product IDs. For development, you may \
     set USB_VID=${USB_VID_TESTING} USB_PID=${USB_PID_TESTING}.

(Not that I'd particularly recommend doing this, still, which is why that's text and not a PR -- but when it's done, that's how I think it'd be done best.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: build system Area: Build system Area: sys Area: System 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
Development

Successfully merging this pull request may close these issues.

2 participants