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

[Core] usb_device_state: consolidate usb state handling across implementations #24258

Merged
merged 3 commits into from
Oct 18, 2024

Conversation

KarlK90
Copy link
Member

@KarlK90 KarlK90 commented Aug 9, 2024

Description

Previously all usb drivers and platform implementations (expect for our oddball atsam) tracked the same two global variables:

  • keyboard_protocol: to indicate if we are in report or boot protocol
  • keyboard_idle: for the idle_rate of the keyboard endpoint

And a local variable that was exposed trough some indirection:

  • keyboard_led_state: for the currently set indicator leds (caps lock etc.)

These have all been moved into the usb_device_state struct wich is accessible by getters and setters.

This reduces code duplication and centralizes the state management across platforms and drivers.

This is then put into use for usb hid specification section 7.2.6:

When initialized, all devices default to report protocol. However the
host should not make any assumptions about the device’s state and should
set the desired protocol whenever initializing a device.

Thus on reset we should always do exactly that.

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: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • 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).

@github-actions github-actions bot added the core label Aug 9, 2024
@KarlK90 KarlK90 changed the title Maintenance/usb device state [Core] USB: consolidate hid state across drivers Aug 9, 2024
@KarlK90 KarlK90 force-pushed the maintenance/usb-device-state branch 4 times, most recently from d208380 to a159096 Compare August 9, 2024 19:58
@KarlK90 KarlK90 changed the title [Core] USB: consolidate hid state across drivers [Core] usb_device_state: consolidate usb state handling across implementations Aug 9, 2024
@KarlK90 KarlK90 changed the title [Core] usb_device_state: consolidate usb state handling across implementations [Core] usb_device_state: consolidate usb state handling across implementations Aug 9, 2024
@KarlK90 KarlK90 marked this pull request as ready for review August 9, 2024 20:03
@KarlK90 KarlK90 requested a review from a team August 9, 2024 20:04
@KarlK90 KarlK90 force-pushed the maintenance/usb-device-state branch from a159096 to b02f9ad Compare August 9, 2024 20:06
@drashna
Copy link
Member

drashna commented Aug 11, 2024

action_util.c and report.c error due to not having USB_PROTOCOL_* defined.

@KarlK90 KarlK90 force-pushed the maintenance/usb-device-state branch from b02f9ad to ef6403e Compare August 12, 2024 07:53
@KarlK90
Copy link
Member Author

KarlK90 commented Aug 12, 2024

action_util.c and report.c error due to not having USB_PROTOCOL_* defined.

Thanks! Fixed now.

@KarlK90 KarlK90 force-pushed the maintenance/usb-device-state branch 2 times, most recently from c414c6a to 5b62e78 Compare August 12, 2024 17:34
@KarlK90 KarlK90 force-pushed the maintenance/usb-device-state branch from 5b62e78 to 4a2aa2b Compare August 12, 2024 17:36
@KarlK90 KarlK90 force-pushed the maintenance/usb-device-state branch from 4a2aa2b to 2546936 Compare August 12, 2024 20:53
@KarlK90 KarlK90 requested review from zvecr, drashna and a team August 13, 2024 08:05
@KarlK90 KarlK90 force-pushed the maintenance/usb-device-state branch from 2546936 to 96a59a7 Compare September 8, 2024 13:05
@KarlK90 KarlK90 force-pushed the maintenance/usb-device-state branch from 96a59a7 to 19fcfa7 Compare October 6, 2024 09:42
@drashna
Copy link
Member

drashna commented Oct 11, 2024

Looks like there are some lint errors for OS detection, and the tests are failing for it, too.

Previously all usb drivers and platform implementations (expect for our
oddball atsam) tracked the same two global variables:

- keyboard_protocol: to indicate if we are in report or boot protocol
- keyboard_idle: for the idle_rate of the keyboard endpoint

And a local variable that was exposed trough some indirection:

- keyboard_led_state: for the currently set indicator leds (caps lock etc.)

These have all been moved into the usb_device_state struct wich is
accessible by getters and setters.

This reduces code duplication and centralizes the state management
across platforms and drivers.

Signed-off-by: Stefan Kerkmann <[email protected]>
The usb hid specification section 7.2.6 states:

When initialized, all devices default to report protocol. However the
host should not make any assumptions about the device’s state and should
set the desired protocol whenever initializing a device.

Thus on reset we should always do exactly that.

Signed-off-by: Stefan Kerkmann <[email protected]>
Signed-off-by: Stefan Kerkmann <[email protected]>
@KarlK90 KarlK90 force-pushed the maintenance/usb-device-state branch from 19fcfa7 to 89dfacf Compare October 12, 2024 17:54
@KarlK90
Copy link
Member Author

KarlK90 commented Oct 12, 2024

Thanks, I've addressed the failing tests.

@drashna drashna requested a review from a team October 12, 2024 19:32
@KarlK90 KarlK90 merged commit 3f9d464 into qmk:develop Oct 18, 2024
4 of 5 checks passed
ilham-agustiawan pushed a commit to ilham-agustiawan/qmk_firmware that referenced this pull request Nov 30, 2024
…ementations (qmk#24258)

* usb_device_state: add idle_rate, led and protocol

Previously all usb drivers and platform implementations (expect for our
oddball atsam) tracked the same two global variables:

- keyboard_protocol: to indicate if we are in report or boot protocol
- keyboard_idle: for the idle_rate of the keyboard endpoint

And a local variable that was exposed trough some indirection:

- keyboard_led_state: for the currently set indicator leds (caps lock etc.)

These have all been moved into the usb_device_state struct wich is
accessible by getters and setters.

This reduces code duplication and centralizes the state management
across platforms and drivers.

Signed-off-by: Stefan Kerkmann <[email protected]>

* usb_device_state: reset protocol on reset

The usb hid specification section 7.2.6 states:

When initialized, all devices default to report protocol. However the
host should not make any assumptions about the device’s state and should
set the desired protocol whenever initializing a device.

Thus on reset we should always do exactly that.

Signed-off-by: Stefan Kerkmann <[email protected]>

* keyboards: fix oversize warnings

Signed-off-by: Stefan Kerkmann <[email protected]>

---------

Signed-off-by: Stefan Kerkmann <[email protected]>
smallketchup82 pushed a commit to smallketchup82/qmk_firmware that referenced this pull request Dec 1, 2024
…ementations (qmk#24258)

* usb_device_state: add idle_rate, led and protocol

Previously all usb drivers and platform implementations (expect for our
oddball atsam) tracked the same two global variables:

- keyboard_protocol: to indicate if we are in report or boot protocol
- keyboard_idle: for the idle_rate of the keyboard endpoint

And a local variable that was exposed trough some indirection:

- keyboard_led_state: for the currently set indicator leds (caps lock etc.)

These have all been moved into the usb_device_state struct wich is
accessible by getters and setters.

This reduces code duplication and centralizes the state management
across platforms and drivers.

Signed-off-by: Stefan Kerkmann <[email protected]>

* usb_device_state: reset protocol on reset

The usb hid specification section 7.2.6 states:

When initialized, all devices default to report protocol. However the
host should not make any assumptions about the device’s state and should
set the desired protocol whenever initializing a device.

Thus on reset we should always do exactly that.

Signed-off-by: Stefan Kerkmann <[email protected]>

* keyboards: fix oversize warnings

Signed-off-by: Stefan Kerkmann <[email protected]>

---------

Signed-off-by: Stefan Kerkmann <[email protected]>
jlaptavi pushed a commit to jlaptavi/qmk_firmware that referenced this pull request Dec 3, 2024
…ementations (qmk#24258)

* usb_device_state: add idle_rate, led and protocol

Previously all usb drivers and platform implementations (expect for our
oddball atsam) tracked the same two global variables:

- keyboard_protocol: to indicate if we are in report or boot protocol
- keyboard_idle: for the idle_rate of the keyboard endpoint

And a local variable that was exposed trough some indirection:

- keyboard_led_state: for the currently set indicator leds (caps lock etc.)

These have all been moved into the usb_device_state struct wich is
accessible by getters and setters.

This reduces code duplication and centralizes the state management
across platforms and drivers.

Signed-off-by: Stefan Kerkmann <[email protected]>

* usb_device_state: reset protocol on reset

The usb hid specification section 7.2.6 states:

When initialized, all devices default to report protocol. However the
host should not make any assumptions about the device’s state and should
set the desired protocol whenever initializing a device.

Thus on reset we should always do exactly that.

Signed-off-by: Stefan Kerkmann <[email protected]>

* keyboards: fix oversize warnings

Signed-off-by: Stefan Kerkmann <[email protected]>

---------

Signed-off-by: Stefan Kerkmann <[email protected]>
DmNosachev pushed a commit to DmNosachev/qmk_firmware that referenced this pull request Dec 7, 2024
…ementations (qmk#24258)

* usb_device_state: add idle_rate, led and protocol

Previously all usb drivers and platform implementations (expect for our
oddball atsam) tracked the same two global variables:

- keyboard_protocol: to indicate if we are in report or boot protocol
- keyboard_idle: for the idle_rate of the keyboard endpoint

And a local variable that was exposed trough some indirection:

- keyboard_led_state: for the currently set indicator leds (caps lock etc.)

These have all been moved into the usb_device_state struct wich is
accessible by getters and setters.

This reduces code duplication and centralizes the state management
across platforms and drivers.

Signed-off-by: Stefan Kerkmann <[email protected]>

* usb_device_state: reset protocol on reset

The usb hid specification section 7.2.6 states:

When initialized, all devices default to report protocol. However the
host should not make any assumptions about the device’s state and should
set the desired protocol whenever initializing a device.

Thus on reset we should always do exactly that.

Signed-off-by: Stefan Kerkmann <[email protected]>

* keyboards: fix oversize warnings

Signed-off-by: Stefan Kerkmann <[email protected]>

---------

Signed-off-by: Stefan Kerkmann <[email protected]>
SyrupSplashin pushed a commit to SyrupSplashin/qmk_firmware that referenced this pull request Dec 10, 2024
…ementations (qmk#24258)

* usb_device_state: add idle_rate, led and protocol

Previously all usb drivers and platform implementations (expect for our
oddball atsam) tracked the same two global variables:

- keyboard_protocol: to indicate if we are in report or boot protocol
- keyboard_idle: for the idle_rate of the keyboard endpoint

And a local variable that was exposed trough some indirection:

- keyboard_led_state: for the currently set indicator leds (caps lock etc.)

These have all been moved into the usb_device_state struct wich is
accessible by getters and setters.

This reduces code duplication and centralizes the state management
across platforms and drivers.

Signed-off-by: Stefan Kerkmann <[email protected]>

* usb_device_state: reset protocol on reset

The usb hid specification section 7.2.6 states:

When initialized, all devices default to report protocol. However the
host should not make any assumptions about the device’s state and should
set the desired protocol whenever initializing a device.

Thus on reset we should always do exactly that.

Signed-off-by: Stefan Kerkmann <[email protected]>

* keyboards: fix oversize warnings

Signed-off-by: Stefan Kerkmann <[email protected]>

---------

Signed-off-by: Stefan Kerkmann <[email protected]>
LeonMusCoden pushed a commit to LeonMusCoden/qmk_firmware that referenced this pull request Jan 5, 2025
…ementations (qmk#24258)

* usb_device_state: add idle_rate, led and protocol

Previously all usb drivers and platform implementations (expect for our
oddball atsam) tracked the same two global variables:

- keyboard_protocol: to indicate if we are in report or boot protocol
- keyboard_idle: for the idle_rate of the keyboard endpoint

And a local variable that was exposed trough some indirection:

- keyboard_led_state: for the currently set indicator leds (caps lock etc.)

These have all been moved into the usb_device_state struct wich is
accessible by getters and setters.

This reduces code duplication and centralizes the state management
across platforms and drivers.

Signed-off-by: Stefan Kerkmann <[email protected]>

* usb_device_state: reset protocol on reset

The usb hid specification section 7.2.6 states:

When initialized, all devices default to report protocol. However the
host should not make any assumptions about the device’s state and should
set the desired protocol whenever initializing a device.

Thus on reset we should always do exactly that.

Signed-off-by: Stefan Kerkmann <[email protected]>

* keyboards: fix oversize warnings

Signed-off-by: Stefan Kerkmann <[email protected]>

---------

Signed-off-by: Stefan Kerkmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants