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: fix compilation issues with USB programmable buttons #14454

Merged
merged 1 commit into from Sep 15, 2021

Conversation

t-8ch
Copy link
Contributor

@t-8ch t-8ch commented Sep 15, 2021

Description

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

@tzarc tzarc merged commit 58d72ad into qmk:develop Sep 15, 2021
@drashna
Copy link
Member

drashna commented Sep 15, 2021

found an edge case right after merge, too. Sorry.

If mousekeys is enabled and usb programmable keys are enabled, it looks like mousekeys do not work/are not sent.

Also, the ifdef should probably be for SHARED_EN_ENABLE, but not 100% sure about that.

@t-8ch
Copy link
Contributor Author

t-8ch commented Sep 15, 2021

@drashna Works for me on lufa vusb (Plaid Pad v3) and Linux

@t-8ch t-8ch deleted the fix-usb-programmable-buttons branch September 15, 2021 21:04
@drashna
Copy link
Member

drashna commented Sep 15, 2021

A little more testing, the mouse movement keys works fine, but the mouse buttons do not work.

Tested on 2x atmega32u4 boards and a at90usb1286 board, so all pure LUFA boards.

@t-8ch t-8ch restored the fix-usb-programmable-buttons branch September 15, 2021 21:32
@t-8ch
Copy link
Contributor Author

t-8ch commented Sep 15, 2021

@drashna For me it works on a evyd13/wasdat (atmega32u4) with PB_1, KC_MS_BTN1, PB_3, KC_MS_U.
(With #14455 applied)

Which OS are you using?

@drashna
Copy link
Member

drashna commented Sep 15, 2021

can confirm that mouse buttons 1-5 don't work. but the directions do work.

Note that this happens when KEYBOARD_SHARED_EP=yes and PROGRAMMABLE_BUTTON_ENABLE=yes are set. If one or the other isn't set, then it works just fine, but if you have both enabled, that's when things stop working.

and the code change from 14455 doesn't seem to help that

@t-8ch
Copy link
Contributor Author

t-8ch commented Sep 15, 2021

I'll take a look.
Yeah 14455 is only for the Programmable Buttons part, not mousekey.

@t-8ch
Copy link
Contributor Author

t-8ch commented Sep 15, 2021

It works for me with this diff over develop, am I missing something?

diff --git a/keyboards/evyd13/wasdat/keymaps/default/keymap.c b/keyboards/evyd13/wasdat/keymaps/default/keymap.c
index f1b11e201f..ce7dd98a76 100644
--- a/keyboards/evyd13/wasdat/keymaps/default/keymap.c
+++ b/keyboards/evyd13/wasdat/keymaps/default/keymap.c
@@ -33,7 +33,7 @@ const uint16_t PROGMEM keymaps[][MATRIX_ROWS][MATRIX_COLS] = {
      * └────┴────┴────┴────────────────────────┴────┴────┴────┴────┘ └───┴───┴───┘ └───────┴───┴───┘
      */
     LAYOUT_fullsize_ansi(
-        KC_ESC,           KC_F1,   KC_F2,   KC_F3,   KC_F4,   KC_F5,   KC_F6,   KC_F7,   KC_F8,   KC_F9,   KC_F10,  KC_F11,  KC_F12,      KC_PSCR, KC_SLCK, KC_PAUS,
+        KC_ESC,           PB_1,   KC_MS_BTN1,   PB_3,   KC_MS_UP,   KC_F5,   KC_F6,   KC_F7,   KC_F8,   KC_F9,   KC_F10,  KC_F11,  KC_F12,      KC_PSCR, KC_SLCK, KC_PAUS,
 
         KC_GRV,  KC_1,    KC_2,    KC_3,    KC_4,    KC_5,    KC_6,    KC_7,    KC_8,    KC_9,    KC_0,    KC_MINS, KC_EQL,  KC_BSPC,     KC_INS,  KC_HOME, KC_PGUP,     KC_NLCK, KC_PSLS, KC_PAST, KC_PMNS,
         KC_TAB,  KC_Q,    KC_W,    KC_E,    KC_R,    KC_T,    KC_Y,    KC_U,    KC_I,    KC_O,    KC_P,    KC_LBRC, KC_RBRC, KC_BSLS,     KC_DEL,  KC_END,  KC_PGDN,     KC_P7,   KC_P8,   KC_P9,   KC_PPLS,
diff --git a/keyboards/evyd13/wasdat/rules.mk b/keyboards/evyd13/wasdat/rules.mk
index 000b9ec5e0..28879ea673 100644
--- a/keyboards/evyd13/wasdat/rules.mk
+++ b/keyboards/evyd13/wasdat/rules.mk
@@ -8,7 +8,9 @@ BOOTLOADER = qmk-dfu
 #   change yes to no to disable
 #
 BOOTMAGIC_ENABLE = lite     # Enable Bootmagic Lite
-MOUSEKEY_ENABLE = no        # Mouse keys
+MOUSEKEY_ENABLE = yes        # Mouse keys
+PROGRAMMABLE_BUTTON_ENABLE = yes
+KEYBOARD_SHARED_EP = yes
 EXTRAKEY_ENABLE = yes       # Audio control and System control
 CONSOLE_ENABLE = no         # Console for debug
 COMMAND_ENABLE = no         # Commands for debug and configuration

@drashna
Copy link
Member

drashna commented Sep 15, 2021

Using develop, I can confirm that it's not working for multiple boards.

However, this seems to be limited to macOS. On windows, it seems to be fine.
And correction: it happens regardless if shared endpoint is enabled or not

@t-8ch
Copy link
Contributor Author

t-8ch commented Sep 16, 2021

I don't have access to a macOS machine to work on this unfortunately.

Does it work with a vusb based device?

Maybe macOS gets confused because the Programmable Buttons are the same way as mouse buttons are.
(albeit with a different usage page and as part of a named array)

@t-8ch t-8ch deleted the fix-usb-programmable-buttons branch September 16, 2021 06:24
@sigprof
Copy link
Contributor

sigprof commented Sep 16, 2021

@drashna Can you set PROGRAMMABLE_BUTTON_ENABLE = no (so that mousekey would work), and then change #ifdef in

#ifdef PROGRAMMABLE_BUTTON_ENABLE
HID_RI_USAGE_PAGE(8, 0x0C), // Consumer
HID_RI_USAGE(8, 0x01), // Consumer Control
HID_RI_COLLECTION(8, 0x01), // Application
HID_RI_REPORT_ID(8, REPORT_ID_PROGRAMMABLE_BUTTON),
HID_RI_USAGE(8, 0x03), // Programmable Buttons
HID_RI_COLLECTION(8, 0x04), // Named Array
HID_RI_USAGE_PAGE(8, 0x09), // Button
HID_RI_USAGE_MINIMUM(8, 0x01), // Button 1
HID_RI_USAGE_MAXIMUM(8, 0x20), // Button 32
HID_RI_LOGICAL_MINIMUM(8, 0x01),
HID_RI_LOGICAL_MAXIMUM(8, 0x01),
HID_RI_REPORT_COUNT(8, 32),
HID_RI_REPORT_SIZE(8, 1),
HID_RI_INPUT(8, HID_IOF_DATA | HID_IOF_VARIABLE | HID_IOF_ABSOLUTE),
HID_RI_END_COLLECTION(0),
HID_RI_END_COLLECTION(0),
#endif
to enable just the HID report descriptor for programmable buttons? This way you could find out whether the problem is that macOS does not like the HID report descriptor part, or the actual QMK code is broken somehow.

@t-8ch
Copy link
Contributor Author

t-8ch commented Sep 16, 2021

There is actually another bug in the report descriptor the seemed to have crept in when copying the one from VUSB.
It's fixed with #14464

Please also test this fix.

Thanks @sigprof for quoting the descriptor here.

@drashna
Copy link
Member

drashna commented Sep 16, 2021

@drashna Can you set PROGRAMMABLE_BUTTON_ENABLE = no (so that mousekey would work), and then change #ifdef in

#ifdef PROGRAMMABLE_BUTTON_ENABLE
HID_RI_USAGE_PAGE(8, 0x0C), // Consumer
HID_RI_USAGE(8, 0x01), // Consumer Control
HID_RI_COLLECTION(8, 0x01), // Application
HID_RI_REPORT_ID(8, REPORT_ID_PROGRAMMABLE_BUTTON),
HID_RI_USAGE(8, 0x03), // Programmable Buttons
HID_RI_COLLECTION(8, 0x04), // Named Array
HID_RI_USAGE_PAGE(8, 0x09), // Button
HID_RI_USAGE_MINIMUM(8, 0x01), // Button 1
HID_RI_USAGE_MAXIMUM(8, 0x20), // Button 32
HID_RI_LOGICAL_MINIMUM(8, 0x01),
HID_RI_LOGICAL_MAXIMUM(8, 0x01),
HID_RI_REPORT_COUNT(8, 32),
HID_RI_REPORT_SIZE(8, 1),
HID_RI_INPUT(8, HID_IOF_DATA | HID_IOF_VARIABLE | HID_IOF_ABSOLUTE),
HID_RI_END_COLLECTION(0),
HID_RI_END_COLLECTION(0),
#endif

to enable just the HID report descriptor for programmable buttons? This way you could find out whether the problem is that macOS does not like the HID report descriptor part, or the actual QMK code is broken somehow.

Removing the ifdef, and setting the feature off causes the issue when shared EP is enabled. Without the shared EP, mouse buttons work fine.

@t-8ch
Copy link
Contributor Author

t-8ch commented Sep 16, 2021

@drashna Did you test with #14464 applied?

cadusk pushed a commit to cadusk/qmk_firmware that referenced this pull request Sep 19, 2021
* qmk/develop: (69 commits)
  Use opendrain pin with external pullup again (qmk#14474)
  Add i2c defaults for Convert to Proton C (qmk#14470)
  [Keyboard] Increase the way to add oled code for helix/rev3. (qmk#14426)
  [Bug] fix logical minimum in Programmable Button rdesc (qmk#14464)
  Adds optional hebrew layout (Unicode) (qmk#14156)
  New CLI subcommand to create clang-compatible compilation database (`compile_commands.json`) (qmk#14370)
  [Keyboard] Add the Idobao ID96 keyboard (qmk#14371)
  [Keyboard] Add FJLabs TF60 Variants and TF65 Variant (qmk#14392)
  [Keyboard] Add Absolute Designs AD65 Keyboard (qmk#14391)
  [Bug] Fix descriptor for USB Programmable Buttons (qmk#14455)
  Make ChibiOS PAL interactions less STM32 specific - Round 2 (qmk#14456)
  core: fix compilation issues with USB programmable buttons (qmk#14454)
  [Keyboard] add Matrix Me (qmk#14331)
  [Keyboard] Replaced Maker Keyboards & FJLabs Legacy Code (qmk#14393)
  [Keyboard] Add `NO_LED` positions to match key matrix. (qmk#14417)
  [Keymap] A slight improvement to my own ErgoDox keymap (qmk#14425)
  [Keymap] Trying again with Prime-e update! (qmk#14429)
  [Keyboard] Update lighting effects for xbows keyboard (qmk#14432)
  [Docs] add sync options heading, update led indicators (qmk#14441)
  [Bug] Fix IS31fl3741 driver to accept 1 or 2 addresses (qmk#14451)
  ...
ptrxyz pushed a commit to ptrxyz/qmk_firmware that referenced this pull request Apr 9, 2022
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.

None yet

4 participants