usb/hid: Add HID report descriptor defines #17242

bergzand · 2021-11-19T13:10:16Z

Contribution description

This PR adds a bunch of defines to help create USB HID report descriptors.

Testing procedure

Check the doxygen
Binaries of:
- tests/sys_fido2_ctap
- tests/usbus_hid
Should be unchanged

Issues/PRs references

None

bergzand · 2021-11-19T13:29:39Z

tests/sys_fido2_ctap on samr21-xpro on master: 132981dc6fbe38482b8964d82e82883c0f76e32a23f761390d36d6466c1c5f70
tests/sys_fido2_ctap on samr21-xpro this pr: 132981dc6fbe38482b8964d82e82883c0f76e32a23f761390d36d6466c1c5f70

tests/usbus_hid on samr21-xpro on master: 31d5535dcef87945c74de8df6f2948738a88d960154c3e65f0d1a7c9aac79641
tests/usbus_hid on samr21-xpro this pr: 31d5535dcef87945c74de8df6f2948738a88d960154c3e65f0d1a7c9aac79641

Compile and check session

koen@zometeen ~/dev/RIOT-dev $ git checkout pr/hid/add_descriptor_defines 
Already on 'pr/hid/add_descriptor_defines'
Your branch is up to date with 'origin/pr/hid/add_descriptor_defines'.
koen@zometeen ~/dev/RIOT-dev $ RIOT_CI_BUILD=1 make -C tests/usbus_hid/ BOARD=samr21-xpro    
make: Entering directory '/home/koen/dev/RIOT-dev/tests/usbus_hid'
Building application "tests_usbus_hid" for "samr21-xpro" with MCU "samd21".

   text    data     bss     dec     hex filename
  17868     148    4508   22524    57fc /home/koen/dev/RIOT-dev/tests/usbus_hid/bin/samr21-xpro/tests_usbus_hid.elf
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!
make: Leaving directory '/home/koen/dev/RIOT-dev/tests/usbus_hid'
koen@zometeen ~/dev/RIOT-dev $ sha256sum tests/usbus_hid/bin/samr21-xpro/tests_usbus_hid.bin 
31d5535dcef87945c74de8df6f2948738a88d960154c3e65f0d1a7c9aac79641  tests/usbus_hid/bin/samr21-xpro/tests_usbus_hid.bin
koen@zometeen ~/dev/RIOT-dev $ RIOT_CI_BUILD=1 make -C tests/sys_fido2_ctap BOARD=samr21-xpro                  
make: Entering directory '/home/koen/dev/RIOT-dev/tests/sys_fido2_ctap'
Building application "tests_sys_fido2_ctap" for "samr21-xpro" with MCU "samd21".

[INFO] cloning fido2_tests
Cloning into '/home/koen/dev/RIOT-dev/build/pkg/fido2_tests'...
remote: Enumerating objects: 855, done.
remote: Counting objects: 100% (220/220), done.
remote: Compressing objects: 100% (154/154), done.
remote: Total 855 (delta 131), reused 127 (delta 63), pack-reused 635
Receiving objects: 100% (855/855), 174.77 KiB | 8.32 MiB/s, done.
Resolving deltas: 100% (508/508), done.
HEAD is now at 3f7893d call reset to make tests repeatable
[INFO] updating fido2_tests /home/koen/dev/RIOT-dev/build/pkg/fido2_tests/.pkg-state.git-downloaded
echo 3f7893d8d1a39b009cddad7913d3808ca664d3b7 > /home/koen/dev/RIOT-dev/build/pkg/fido2_tests/.pkg-state.git-downloaded
[INFO] patch fido2_tests
[INFO] cloning micro-ecc
Cloning into '/home/koen/dev/RIOT-dev/build/pkg/micro-ecc'...
remote: Enumerating objects: 1143, done.
remote: Counting objects: 100% (48/48), done.
remote: Compressing objects: 100% (37/37), done.
remote: Total 1143 (delta 21), reused 28 (delta 9), pack-reused 1095
Receiving objects: 100% (1143/1143), 687.07 KiB | 8.08 MiB/s, done.
Resolving deltas: 100% (664/664), done.
HEAD is now at 4b1709c Merge pull request #176 from benpicco/riot
[INFO] updating micro-ecc /home/koen/dev/RIOT-dev/build/pkg/micro-ecc/.pkg-state.git-downloaded
echo 4b1709c17abbe938d6d8811f4c7c5d082a144799 > /home/koen/dev/RIOT-dev/build/pkg/micro-ecc/.pkg-state.git-downloaded
[INFO] patch micro-ecc
[INFO] cloning tiny-asn1
Cloning into '/home/koen/dev/RIOT-dev/build/pkg/tiny-asn1'...
remote: Enumerating objects: 327, done.
remote: Counting objects: 100% (13/13), done.
remote: Compressing objects: 100% (13/13), done.
remote: Total 327 (delta 3), reused 3 (delta 0), pack-reused 314
Receiving objects: 100% (327/327), 85.77 KiB | 1.59 MiB/s, done.
Resolving deltas: 100% (194/194), done.
HEAD is now at 7005fcf Merge branch '2-pipeline-status-badge-is-broken' into 'develop'
[INFO] updating tiny-asn1 /home/koen/dev/RIOT-dev/build/pkg/tiny-asn1/.pkg-state.git-downloaded
echo 7005fcff4706e96b857f257ef94b8518211c9fbc > /home/koen/dev/RIOT-dev/build/pkg/tiny-asn1/.pkg-state.git-downloaded
[INFO] patch tiny-asn1
[INFO] cloning tinycbor
Cloning into '/home/koen/dev/RIOT-dev/build/pkg/tinycbor'...
remote: Enumerating objects: 3024, done.
remote: Counting objects: 100% (225/225), done.
remote: Compressing objects: 100% (110/110), done.
remote: Total 3024 (delta 143), reused 177 (delta 115), pack-reused 2799
Receiving objects: 100% (3024/3024), 1.42 MiB | 5.30 MiB/s, done.
Resolving deltas: 100% (2038/2038), done.
HEAD is now at 755f9ef Parser: validate that maps have both key and value items
[INFO] updating tinycbor /home/koen/dev/RIOT-dev/build/pkg/tinycbor/.pkg-state.git-downloaded
echo 755f9ef932f9830a63a712fd2ac971d838b131f1 > /home/koen/dev/RIOT-dev/build/pkg/tinycbor/.pkg-state.git-downloaded
[INFO] patch tinycbor
   text    data     bss     dec     hex filename
  52444     248   30532   83224   14518 /home/koen/dev/RIOT-dev/tests/sys_fido2_ctap/bin/samr21-xpro/tests_sys_fido2_ctap.elf
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!
make: Leaving directory '/home/koen/dev/RIOT-dev/tests/sys_fido2_ctap'
koen@zometeen ~/dev/RIOT-dev $ sha256sum tests/sys_fido2_ctap/bin/samr21-xpro/tests_sys_fido2_ctap.bin 
132981dc6fbe38482b8964d82e82883c0f76e32a23f761390d36d6466c1c5f70  tests/sys_fido2_ctap/bin/samr21-xpro/tests_sys_fido2_ctap.bin
koen@zometeen ~/dev/RIOT-dev $ git checkout upstream/master
HEAD is now at 09e0692a85 Merge pull request #17201 from bergzand/pr/nrf5x/periph_qdec
koen@zometeen ~/dev/RIOT-dev $ RIOT_CI_BUILD=1 make -C tests/usbus_hid/ BOARD=samr21-xpro
make: Entering directory '/home/koen/dev/RIOT-dev/tests/usbus_hid'
Building application "tests_usbus_hid" for "samr21-xpro" with MCU "samd21".

   text    data     bss     dec     hex filename
  17868     148    4508   22524    57fc /home/koen/dev/RIOT-dev/tests/usbus_hid/bin/samr21-xpro/tests_usbus_hid.elf
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!
make: Leaving directory '/home/koen/dev/RIOT-dev/tests/usbus_hid'
koen@zometeen ~/dev/RIOT-dev $ sha256sum tests/usbus_hid/bin/samr21-xpro/tests_usbus_hid.bin 
31d5535dcef87945c74de8df6f2948738a88d960154c3e65f0d1a7c9aac79641  tests/usbus_hid/bin/samr21-xpro/tests_usbus_hid.bin
koen@zometeen ~/dev/RIOT-dev $ RIOT_CI_BUILD=1 make -C tests/sys_fido2_ctap BOARD=samr21-xpro
make: Entering directory '/home/koen/dev/RIOT-dev/tests/sys_fido2_ctap'
Building application "tests_sys_fido2_ctap" for "samr21-xpro" with MCU "samd21".

   text    data     bss     dec     hex filename
  52444     248   30532   83224   14518 /home/koen/dev/RIOT-dev/tests/sys_fido2_ctap/bin/samr21-xpro/tests_sys_fido2_ctap.elf
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!
make: Leaving directory '/home/koen/dev/RIOT-dev/tests/sys_fido2_ctap'
koen@zometeen ~/dev/RIOT-dev $ sha256sum tests/sys_fido2_ctap/bin/samr21-xpro/tests_sys_fido2_ctap.bin 
132981dc6fbe38482b8964d82e82883c0f76e32a23f761390d36d6466c1c5f70  tests/sys_fido2_ctap/bin/samr21-xpro/tests_sys_fido2_ctap.bin

bergzand · 2021-11-19T14:29:44Z

I've excluded the usage tables themselves from doxygen. I don't think it adds anything to have them in doxygen and they will only cause warnings. The other solution is to comment them all with something obvious and I think that decreases the already bad readability of the header file.

Ollrogge · 2021-11-19T16:43:41Z

Something is broke. tests/sys_fido2_ctap does not work on WebAuthn.io anymore

Ollrogge · 2021-11-19T16:54:36Z

Something is broke. tests/sys_fido2_ctap does not work on WebAuthn.io anymore

Okay master also doesn't work. The hid report descriptor looks fine and this is the only thing that affects the FIDO2 code.
Therefore changes are fine with me :)

bergzand · 2021-11-19T16:55:29Z

Something is broke. tests/sys_fido2_ctap does not work on WebAuthn.io anymore

Maybe I messed something up in one of my later pushes. Do you have time to check if things still work with current master? Otherwise I will investigate later

Okay master also doesn't work. The hid report descriptor looks fine and this is the only thing that affects the FIDO2 code.
Therefore changes are fine with me :)

Thanks for investigating! Could be that #17230 caused some unwelcome side effects.

Ollrogge · 2021-11-19T17:03:00Z

Something is broke. tests/sys_fido2_ctap does not work on WebAuthn.io anymore

Maybe I messed something up in one of my later pushes. Do you have time to check if things still work with current master? Otherwise I will investigate later

Okay master also doesn't work. The hid report descriptor looks fine and this is the only thing that affects the FIDO2 code.
Therefore changes are fine with me :)

Thanks for investigating! Could be that #17230 caused some unwelcome side effects.

Thanks for the pointer :) Will investigate

dylad

Few mistakes spotted.
And I didn't look at hid_usage.h yet. It might take some time...

dylad · 2021-11-19T19:22:22Z

sys/include/usb/hid.h

+/**
+ * @brief Physical collection type
+ */
+#define USB_HID_COLLECTION_PHYSICAL     0x00
+
+/**
+ * @brief Application collection type
+ */
+#define USB_HID_COLLECTION_APPLICATION  0x01


Any reasons not to declare
USB_HID_COLLECTION_LOGICAL
USB_HID_COLLECTION_REPORT
... and so on ?

Nope, added them.

sys/include/usb/hid.h

aabadie · 2022-01-03T14:12:55Z

ping @bergzand

dylad · 2022-01-10T10:35:09Z

Please squash.

Content of the HID report descriptor itself is unchanged

bergzand · 2022-01-10T10:36:27Z

Squashed

bergzand added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Nov 19, 2021

bergzand requested review from aabadie, fjmolinas, leandrolanzieri, MichelRottleuthner and smlng as code owners November 19, 2021 13:10

bergzand requested review from Ollrogge and dylad November 19, 2021 13:10

github-actions bot added Area: sys Area: System Area: tests Area: tests and testing framework Area: USB Area: Universal Serial Bus labels Nov 19, 2021

bergzand force-pushed the pr/hid/add_descriptor_defines branch 2 times, most recently from da9cb1b to db113e1 Compare November 19, 2021 13:25

bergzand force-pushed the pr/hid/add_descriptor_defines branch 2 times, most recently from 636cf6e to 7d7b70b Compare November 19, 2021 14:23

bergzand added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 19, 2021

benpicco approved these changes Nov 19, 2021

View reviewed changes

dylad requested changes Nov 19, 2021

View reviewed changes

dylad approved these changes Jan 10, 2022

View reviewed changes

bergzand added 2 commits January 10, 2022 11:35

usb/hid: Add HID report descriptor defines

758a0bc

sys/fido2: Adapt CTAP HID descriptors to use helper defines

a148f2d

Content of the HID report descriptor itself is unchanged

tests/usbus_hid: Adapt HID descriptors to use helper defines

8ea334d

Content of the HID report descriptor itself is unchanged

bergzand force-pushed the pr/hid/add_descriptor_defines branch from bfe742d to 8ea334d Compare January 10, 2022 10:36

benpicco enabled auto-merge January 10, 2022 10:55

benpicco merged commit f33b3ad into RIOT-OS:master Jan 10, 2022

fjmolinas added this to the Release 2022.01 milestone Jan 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

usb/hid: Add HID report descriptor defines #17242

usb/hid: Add HID report descriptor defines #17242

bergzand commented Nov 19, 2021

bergzand commented Nov 19, 2021

bergzand commented Nov 19, 2021

Ollrogge commented Nov 19, 2021

Ollrogge commented Nov 19, 2021

bergzand commented Nov 19, 2021

Ollrogge commented Nov 19, 2021

dylad left a comment

dylad Nov 19, 2021

bergzand Jan 10, 2022

aabadie commented Jan 3, 2022

dylad commented Jan 10, 2022

bergzand commented Jan 10, 2022

usb/hid: Add HID report descriptor defines #17242

usb/hid: Add HID report descriptor defines #17242

Conversation

bergzand commented Nov 19, 2021

Contribution description

Testing procedure

Issues/PRs references

bergzand commented Nov 19, 2021

bergzand commented Nov 19, 2021

Ollrogge commented Nov 19, 2021

Ollrogge commented Nov 19, 2021

bergzand commented Nov 19, 2021

Ollrogge commented Nov 19, 2021

dylad left a comment

Choose a reason for hiding this comment

dylad Nov 19, 2021

Choose a reason for hiding this comment

bergzand Jan 10, 2022

Choose a reason for hiding this comment

aabadie commented Jan 3, 2022

dylad commented Jan 10, 2022

bergzand commented Jan 10, 2022