8000 Support secondary button by usr-sse2 · Pull Request #445 · VoodooI2C/VoodooI2C · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Support secondary button #445

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 3, 2022
Merged

Support secondary button #445

merged 1 commit into from
Feb 3, 2022

Conversation

usr-sse2
Copy link
Contributor

This was meant to be a part of VoodooI2C/VoodooI2CHID#39, just forgot to make a PR in this repo.

@kprinssu
Copy link
Collaborator
kprinssu commented Feb 20, 2021 •
edited
Loading

Looks good! Thanks @usr-sse2, this feature ha been requested quite frequently and will be useful to those with dedicated physical buttons.

@kprinssu kprinssu requested review from kprinssu and ben9923 February 20, 2021 16:31
ben9923
ben9923 reviewed Feb 20, 2021
View reviewed changes
@ben9923
Copy link
Member
ben9923 commented Feb 20, 2021

@usr-sse2 Thanks!
I figured changes for VoodooI2C were necessary back then.

@usr-sse2 @kprinssu Shouldn't the isPhysicalButtonDown field be completely removed from the VoodooInput Simulator message?
I've been thinking it shouldn't be part of it for a long time, as it's not an MT2 feature.
Seems like VoodooPS2 should still be migrated before it can happen, anyway.

BTW, the button bitmask is parsed by IOHIPointing, right?

@kprinssu
Copy link
Collaborator
kprinssu commented Feb 23, 2021 •
edited
Loading

@ben9923 It's still defined here, https://github.com/acidanthera/VoodooInput/blob/master/VoodooInput/VoodooInputMultitouch/VoodooInputTransducer.h#L43

I believe pressure values were added, perhaps that's what you are thinking of?

@usr-sse2
Copy link
Contributor Author

Button is a field in MT2 input report: https://github.com/acidanthera/VoodooInput/blob/bc7fdb5c0c152dcd8c3ba33b5e67bde00b2c853b/VoodooInput/VoodooInputSimulator/VoodooInputSimulatorDevice.hpp#L69
Is this value used by Apple's MT2 drivers? Is it really boolean or a button bitmask?

@ben9923
Copy link
Member
ben9923 commented Feb 26, 2021

@usr-sse2 Gotcha.
So this PR intends to pretty much ignore it. Nothing should be broken because of that, right?

I really do wonder what Apple's driver do with that value, and whether it's a bitmask (Each finger should have its own pressure value, what can it be used for then?)
@kprinssu Do you have an idea?

Goshin
Goshin reviewed Mar 2, 2021
View reviewed changes
Goshin referenced this pull request in acidanthera/VoodooInput Mar 27, 2021
@Goshin @usr-sse2
Update touch state definition
a1e92d7
@vit9696
Copy link
vit9696 commented Mar 28, 2021

I had a brief conversation with @usr-sse2 regarding this change, and we came with a conclusion that while refactoring all the satellites is possible to support value tracking, this is a breaking change that we should not do right now in a minor release.

I actually request this to be merged for the next release, as apparently the changes on the VoodooInput side made one of the issues more apparent for @lvs1974 acidanthera/VoodooInput@a1e92d7.

ben9923
ben9923 reviewed Apr 1, 2021
View reviewed changes
@ben9923
Copy link
Member
ben9923 commented Apr 1, 2021 •
edited
Loading

@vit9696 Thanks for the input.
Proper tracking is probably only needed for VoodooI2CHID, but as I replied earlier it might be a bigger change than we thought, indeed making it something for a further release.

I do wish to at least have a proper answer for this comment I've made. Not sure if it's really the desired functionality.

Also, this line (either with/without last value check) mean secondary button will only work if force touch is disabled in SysPrefs, right?
Do we really want it supported just when it's disabled?

I believe that with those changes when force touch is enabled, buttons (actually just primary) will only work when there's a finger on the touchpad.

This PR can lead to a lot of user issues, so I do think it should be closely reviewed even if it doesn't have many changes. I'm sorry it may cause delays with shipping it, but once we sort everything in this PR we can make a release right away.

I also have a question - shouldn't isPhysicalButtonDown field of VoodooInput be eliminated once VoodooI2C, VoodooRMI and VoodooPS2 migrate to Trackpoint device?

@1Revenger1
Copy link
Contributor

I also have a question - shouldn't isPhysicalButtonDown field of VoodooInput be eliminated once VoodooI2C, VoodooRMI and VoodooPS2 migrate to Trackpoint device?

Do we know if macOS does anything with the button field?
VoodooRMI currently does use the trackpoint device for all the buttons other than the clickpad button. I don't see any reason to remove that field since clicking down on the clickpad is analogous to clicking down on a magic trackpad.

@theroadw
Copy link
theroadw commented Apr 12, 2021 •
edited
Loading

I have been testing this PR for the last week on my ZBook G5 17 ALPS trackpad and everything works perfectly.
(Force Touch disabled)
Thank you!

@chilledHamza
Copy link
chilledHamza commented Apr 24, 2021 •
edited
Loading

Tested this on two devices:-
SYNA2B46 - Lenovo Rescuer Y7000 (Chinese variant of Legion Y530) : Both Buttons work properly
ELAN061B - Lenovo Legion Y530 : primary working as secondary (Left button -> Right button), secondary works as tertiary (Right button -> Middle button)

"Primary Button Element" and "Secondary Button Element" are missing in IOReg under VoodooI2CPrecisionTouchpadHIDEventDriver -> Degitizer (IORegDump)

properties->setObject("Contact Count Element",         digitiser.contact_count);
properties->setObject("Input Mode Element",            digitiser.input_mode);
properties->setObject("Contact Count Maximum Element", digitiser.contact_count_maximum);
properties->setObject("Primary Button Element",        digitiser.primaryButton);
properties->setObject("Secondary Button Element",      digitiser.secondaryButton);
setOSDictionaryNumber(properties, "Transducer Count",  digitiser.transducers->getCount());

That makes me thing that following piece of code (from VoodooI2C/VoodooI2CHID/pull/39) is never executed, and primaryButton/secondaryButton is possiblynullptr when ::setDigitizerProperties() is called

// On Dell Latitude 7390 2-in-1 the left button has kHIDUsage_Button_2,
// and the right button has kHIDUsage_Button_3. So, there is no button 1.
if (element->conformsTo(kHIDPage_Button)) {
    if (digitiser.primaryButton == nullptr) {
        digitiser.primaryButton = element;
    }
    else if (element->getUsage() > digitiser.primaryButton->getUsage()) {
        // Candidate for a secondary button
        if (digitiser.secondaryButton == nullptr || element->getUsage() < digitiser.secondaryButton->getUsage()) {
            digitiser.secondaryButton = element;
        }
    }
    else if (element->getUsage() < digitiser.primaryButton->getUsage()) {
        // This is the new primary button. Old primary becomes secondary.
        digitiser.secondaryButton = digitiser.primaryButton;
        digitiser.primaryButton = element;
    }
}

I don't have SYNA2B46 anymore to test and compare IOReg, so I might be overthinking here.

setButtonState(&transducer->physical_button, (usage - 1), value, timestamp);

As a temporary workaround I'm using :

setButtonState(&transducer->physical_button, (usage - 2), value, timestamp);

Fixes button issue for ELAN061B, but will technically break button functionality for other devices.
Just thought I should share my finding with you guys, so that we come up with some proper solution.

Edit : replaced html- links with static code snippet

@kprinssu kprinssu mentioned this pull request Jun 28, 2021
Closed
@ghost
Copy link
ghost commented Aug 6, 2021

Hello,

I'm sorry to report that this branch doesn't work on Dell ALPS U1 touchpad, which supports the PTP protocol. After using this branch, I still got only a responsive left button while there are active inputs, and the middle and right buttons are still not working. Please tell me that if there is anything I could do to report bugs. Thanks!

@ben9923
Copy link
Member
ben9923 commented Nov 13, 2021

@usr-sse2 As we plan on making a release soon, do you think this is ready for merge?
Is sending the button report on every touch efficient enough, or it would impact performance/energy usage in a noticeable way?

Sorry it took a long while.

@usr-sse2
Copy link
Contributor Author

@ben9923 I use it since making this PR and haven't noticed any impact on the performance or energy consumption.

@ben9923
Copy link
Member
ben9923 commented Dec 12, 2021

@usr-sse2 In that case, we can go ahead and merge :)

CC @kprinssu

ben9923
ben9923 approved these changes Dec 12, 2021
View reviewed changes
Copy link
Member
@ben9923 ben9923 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There are some optimizations & improvements I'd like to see in this code, as discussed in the review threads, but it looks fine for now.

We probably want to open an issue for those, before merging this in.

@kprinssu kprinssu merged commit 9ed1dcd into VoodooI2C:master Feb 3, 2022
@Qonfused Qonfused mentioned this pull request Jan 14, 2023
Closed
@Qonfused Qonfused mentioned this pull request Feb 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Goshin Goshin Goshin left review comments

@kprinssu kprinssu kprinssu approved these changes

@ben9923 ben9923 ben9923 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@usr-sse2 @kprinssu @ben9923 @vit9696 @1Revenger1 @theroadw @chilledHamza @Goshin
0