Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V-USB Interface reorder #9090

Merged
merged 3 commits into from
May 14, 2020
Merged

Conversation

yiancar
Copy link
Contributor

@yiancar yiancar commented May 13, 2020

This might seem irrelevant now as we only got 2 endpoints, but with a 3rd endpoint (#9020) it is needed.

I have PRed in master as this is NOT breaking change.

Description

  • Reordered usb_interface enumeration.
  • VIA expects RAW_HID endpoint to be at interface 1.
  • This does not break anything with only 2 endpoints. Keyboard is always at interface 0 and the extra feature is at interface 1.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

This might seem irrelevant now as we only got 2 endpoints, but with a 3rd endpoint (qmk#9020) it is needed.

I have PRed in master as this is NOT breaking change.

- Reordered usb_interface enumeration.
- VIA expects RAW_HID endpoint to be at interface 1.
- This does not break anything with only 2 endpoints. Keyboard is always at interface 0 and the extra feature is at interface 1.
@zvecr zvecr added the core label May 13, 2020
Copy link
Member

@zvecr zvecr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand what this PR aims to do, i dont agree with the technical direction. I would rather see us move in the direction that allows interface id to be keyed to vusb interface functions.

@yiancar
Copy link
Contributor Author

yiancar commented May 13, 2020

I think the technical change your proposing is more generalized than V-USB.
Chibios and LUFA use:

enum usb_interfaces {
which similarly is executed in the same way as the change proposed here.

TL;DR: this PR aligns the interface behavior as the other 2 USB "libraries".

My general goal is to get everything aligned for the future branch merge in order for 3 endpoints to be accessible to V-USB keyboards. I will be updating #9020 with successful tests across a plethora of boards ( 32a, 328p, usbasp bootloader, bootmapper bootloader ).

Copy link
Member

@zvecr zvecr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should at least add the comment from usb_descriptor.h.

This seems like an odd technical limitation, but not one i really care to look into.

tmk_core/protocol/vusb/vusb.c Show resolved Hide resolved
Copy link
Member

@fauxpark fauxpark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per #6801, the raw HID descriptors should also be placed after the keyboard descriptors in vusb.[ch].

@yiancar
Copy link
Contributor Author

yiancar commented May 13, 2020

Done

Copy link
Member

@fauxpark fauxpark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch, that diff... confirmed manually 👍
Though, this is bound to introduce merge conflicts between this and future now. :(

@yiancar
Copy link
Contributor Author

yiancar commented May 14, 2020

It did before anyway sadly

@fauxpark fauxpark merged commit 632285c into qmk:master May 14, 2020
@yiancar yiancar deleted the V-USB_Interface_reorder branch May 14, 2020 09:05
bitherder pushed a commit to bitherder/qmk_firmware that referenced this pull request May 15, 2020
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request May 24, 2020
sowbug pushed a commit to sowbug/qmk_firmware that referenced this pull request May 24, 2020
fdidron pushed a commit to zsa/qmk_firmware that referenced this pull request Jun 12, 2020
turky pushed a commit to turky/qmk_firmware that referenced this pull request Jun 13, 2020
jakobaa pushed a commit to jakobaa/qmk_firmware that referenced this pull request Jul 7, 2020
sjmacneil pushed a commit to sjmacneil/qmk_firmware that referenced this pull request Feb 19, 2021
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants