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

[Feature] Swap buttons on PS2 Mouse/Trackball #9205

Merged
merged 13 commits into from
Aug 5, 2021
Merged

Conversation

jkutianski
Copy link

Description

This add the config parameter PS2_MOUSE_INVERT_BUTTONS that invert the left & right button on the PS2 mouse.

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

@zvecr zvecr changed the title [Feature Request] Swap buttons on PS2 Mouse/Trackball [Feature] Swap buttons on PS2 Mouse/Trackball May 27, 2020
@zvecr zvecr requested a review from a team May 27, 2020 00:37
tmk_core/protocol/ps2_mouse.c Outdated Show resolved Hide resolved
@zvecr zvecr requested a review from a team May 27, 2020 00:44
@jkutianski jkutianski requested a review from zvecr May 28, 2020 00:36
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.

Not a fan of the new proposed logic. Its far more complex than just swapping two bits. I dont have time to profile it, but i cant imagine its much faster/smaller.

@jkutianski
Copy link
Author

jkutianski commented May 28, 2020

Yes I tried to found a simple solution but I cant see it. I'm not a fan of complexity too.

buttons ^= 1 << PS2_MOUSE_BTN_LEFT; 
buttons ^= 1 << PS2_MOUSE_BTN_RIGHT;

This code doesn't work as intented. Just works with 2 buttons mouses partially.
If you press left and right at the same time the result is no press at all.

With 3 buttons:
If you press just the middle button is like press all the buttons (left, middle and right)
If you press the 3 buttons is like press only the middle one.

If you try to implement the 4th and 5th button of MS Intellimouse Explorer this mess up the Z3, Z1 & Z0.

/*
 * Data format:
 * byte|7       6       5       4       3       2       1       0
 * ----+----------------------------------------------------------------
 *    0|[Yovflw][Xovflw][Ysign ][Xsign ][ 1    ][Middle][Right ][Left  ]
 *    1|[                    X movement(0-255)                         ]
 *    2|[                    Y movement(0-255)                         ]
 *    3|[ 0    ][ 0    ][ 5th  ][ 4th  ][  Z3  ][  Z2  ][  Z1  ][  Z0  ]
 */

Z0-Z3 is a 2's complement number which represents the amount
of movement that has occurred since the last data report. 
Valid values range from -8 to +7.

I think the most performant and simple solution is a look up table. Something like:

static const __swap_btn_lut uint8_t swap_btn[8] = {0b000, 0b010, 0b001, 0b011, 0b100, 0b101, 0b110, 0b111};

@jkutianski jkutianski requested a review from zvecr May 28, 2020 19:21
@jkutianski
Copy link
Author

I changed the logic to a LUT.
Looks easy to understand and more cleaner than use binary operators.

tmk_core/protocol/ps2_mouse.h Outdated Show resolved Hide resolved
tmk_core/protocol/ps2_mouse.h Outdated Show resolved Hide resolved
@zvecr
Copy link
Member

zvecr commented May 28, 2020

Looks easy to understand and more cleaner than use binary operators.

However the lookup table still suffers from assumptions in the value of PS2_MOUSE_BTN_LEFT and PS2_MOUSE_BTN_RIGHT.

@jkutianski
Copy link
Author

I return to the idea of operators. I tried to think another approach.
This new one looks more clear and no macros needed.
Hope this fits.
Tell me if this is OK and I will clean up this a little bit before to merge.

@jkutianski jkutianski closed this May 29, 2020
@jkutianski jkutianski reopened this May 29, 2020
@jkutianski jkutianski requested a review from zvecr May 29, 2020 06:22
@noroadsleft noroadsleft requested a review from a team August 5, 2020 22:14
@drashna
Copy link
Member

drashna commented Aug 20, 2020

Also, (@noroadsleft ) I think this should be retargeted to the develop branch, and rebased.

@drashna drashna requested a review from a team December 17, 2020 04:09
@tzarc tzarc closed this Feb 27, 2021
@tzarc tzarc deleted the branch qmk:develop February 27, 2021 20:27
@noroadsleft noroadsleft reopened this Feb 27, 2021
tmk_core/protocol/ps2_mouse.c Outdated Show resolved Hide resolved
@tzarc tzarc merged commit 07553b4 into qmk:develop Aug 5, 2021
nhongooi pushed a commit to nhongooi/qmk_firmware that referenced this pull request Dec 5, 2021
* [Feature Request] Swap buttons on PS2 Mouse/Trackball

* [Feature Request] Swap buttons on PS2 Mouse/Trackball

* Added id: to the doc

* Missing space

* Solve comment
qmk#9205 (comment)

* Solve comments qmk#9205 (comment) & qmk#9205 (comment)

* Format code more according to https://docs.qmk.fm/#/coding_conventions_c

* change logic to LUT

* WIP: Clean up

* WIP: Solution with xor operators to mask the change

* delete #endif & added the missed xor operator (ahhh)

* Variable (mouse_report->buttons): avoid setting twice qmk#9205 (comment)

* Update tmk_core/protocol/ps2_mouse.c

Co-authored-by: Nick Brassel <[email protected]>

Co-authored-by: juank <[email protected]>
Co-authored-by: Nick Brassel <[email protected]>
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
* [Feature Request] Swap buttons on PS2 Mouse/Trackball

* [Feature Request] Swap buttons on PS2 Mouse/Trackball

* Added id: to the doc

* Missing space

* Solve comment
qmk#9205 (comment)

* Solve comments qmk#9205 (comment) & qmk#9205 (comment)

* Format code more according to https://docs.qmk.fm/#/coding_conventions_c

* change logic to LUT

* WIP: Clean up

* WIP: Solution with xor operators to mask the change

* delete #endif & added the missed xor operator (ahhh)

* Variable (mouse_report->buttons): avoid setting twice qmk#9205 (comment)

* Update tmk_core/protocol/ps2_mouse.c

Co-authored-by: Nick Brassel <[email protected]>

Co-authored-by: juank <[email protected]>
Co-authored-by: Nick Brassel <[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.

6 participants