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: pointer movement/scrolling #2027

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

petejohanson
Copy link
Contributor

@petejohanson petejohanson commented Nov 17, 2023

Continuation of the incremental integration of the great work from @krikun98 in #778 with the movement and scrolling pieces next. Based on the rebased work from @caksoylar and and the split acceleration from @bryanforbes

TODO

  • Docs update
  • Review acceleration curves/functions configurability
  • Review @manna-harbour Discord discussion on per-device scaling factors.
  • Review @manna-harbour Discord note on storing float values, and on send of the report discard whole values, but retain the fractional remainder to be included in the follow up calculations, to avoid jerkiness/jumpiness.

@petejohanson petejohanson added enhancement New feature or request pointers Pointer related functionality labels Nov 17, 2023
@petejohanson petejohanson self-assigned this Nov 17, 2023
k_work_submit_to_queue(zmk_mouse_work_q(), &mouse_tick);
}

K_TIMER_DEFINE(mouse_timer, mouse_timer_cb, mouse_clear_cb);
Copy link
Contributor

Choose a reason for hiding this comment

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

The part that initially made me bang my head against the wall with all this is the fact that Zephyr sometimes seems to de-prioritize these timers if the system is under load.
This is (as far as I was able to understand) the primary cause of the instability with mouse movement - reports can be sent out at random intervals or dropped entirely.
I was exploring re-writing this system to use a dedicated thread, but I wasn't skilled enough at C at the time.
I'm not sure this basis for the tick system is robust enough for general use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So... timers should be using the hardware timers to trigger, and happen in ISR context. The queue has a certain priority, can be preempted, etc. Tweaking that thread priority might help, but I'll play a bit to see what I can determine. Thanks for the insight!

Copy link
Contributor

Choose a reason for hiding this comment

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

This is most certainly not definitive - it's a half-remembered conclusion from an investigation I wasn't skilled enough to perform two years ago, just food for thought. I distinctly remember reports being delayed or dropped and that causing instability. hog.c:335 might also be worth taking a look at.


/* Mouse move behavior */
#define MOVE_Y(vert) ((vert)&0xFFFF)
#define MOVE_Y_DECODE(encoded) (int16_t)((encoded)&0x0000FFFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might make sense to move these decode defines outside the dt-bindings header, and to either plain mouse.h or the corresponding event headers they are used in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO, they are complementary and should live together.... I think keeping them here is appropriate.

@petejohanson petejohanson force-pushed the feat/pointers-move-scroll branch 9 times, most recently from 32ed133 to c2bf21b Compare November 25, 2023 00:46
englmaxi added a commit to englmaxi/zmk that referenced this pull request Nov 26, 2023
englmaxi added a commit to englmaxi/zmk that referenced this pull request Nov 26, 2023
@krikun98
Copy link
Contributor

krikun98 commented Dec 4, 2023

Could you please change the target to https://github.com/petejohanson/zmk/tree/core/zephyr-3.5-update here? The review is unnecessarily cluttered.

@petejohanson
Copy link
Contributor Author

Could you please change the target to https://github.com/petejohanson/zmk/tree/core/zephyr-3.5-update here? The review is unnecessarily cluttered.

Unfortunately, that will move the PR into a different GH fork, and make a bit of a mess. I recommend reviewing each commit for clarity for now, until the Zephyr 3.5 bits are merged into ZMK main.

@petejohanson petejohanson force-pushed the feat/pointers-move-scroll branch 5 times, most recently from 505343b to d837c95 Compare December 4, 2023 23:01
app/src/mouse/main.c Outdated Show resolved Hide resolved
Copy link
Contributor

@caksoylar caksoylar left a comment

Choose a reason for hiding this comment

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

I think these are some missed updates for the rebase to account for 36eda571b

app/src/behaviors/behavior_mouse_key_press.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_mouse_key_press.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_input_two_axis.c Outdated Show resolved Hide resolved
app/src/behaviors/behavior_input_two_axis.c Outdated Show resolved Hide resolved
@fiplox
Copy link

fiplox commented Jun 13, 2024

I'd probably try a combination of the two; e.g. try a tap-ms of 50 and see if scroll speed needs increasing.

Adding tap-ms = <100>; does the trick. Thanks.

while adding tap-ms = <100>; makes it work, I find a delay being too big. Playing with parameters, found that tap-ms of 30 with #define ZMK_MOUSE_DEFAULT_SCRL_VAL 35 works almost perfect.

While using this branch I found out that if I use scroll with an encoder and I scroll too fast, the scroll is never released (infinite scroll) and I have to disconnect my keyboard.

molnarg added a commit to molnarg/Adv360-Pro-ZMK that referenced this pull request Jul 8, 2024
@eteq
Copy link

eteq commented Jul 14, 2024

Has @petejohanson abandoned this PR? Or is it awaiting review? (If abandoned I might try to pick it up in a follow-on PR but not if @petejohanson is still working on it)

@caksoylar
Copy link
Contributor

This isn't abandoned, I am guessing Studio work is getting more priority right now. Pete mentioned on Discord that he is planning to refactor the endpoints a bit (the current split one has some issues on some OS, like Ubuntu 22.04).

Meanwhile if anyone needs a rebased branch in the very near future, you can cherry-pick from https://github.com/caksoylar/zmk/commits/caksoylar/experimental.

caksoylar and others added 14 commits August 3, 2024 21:07
Co-authored-by: Alexander Krikun <[email protected]>
Co-authored-by: Robert U <[email protected]>
Co-authored-by: Shawn Meier <[email protected]>
* Add ability to swap X/Y, invert X and Y values, and apply a
  scalar multiplier/divisor.
* Remove now-unused mouse work queue and related mouse main file.
* Move ticks config into a DTS property on the two axis input behavior.
* Corrected logging for two-axis input timestamps.
* Buffer data from input devices and only surface to HID once synd'd.
* Always import mouse keys behavior and their associated listeners.
* Tweak listener code to only add listener nodes when
  listener and the associated input device are enabled.
@petejohanson petejohanson requested a review from a team as a code owner August 12, 2024 18:55
@proostas
Copy link

proostas commented Aug 31, 2024

I built a new firmware for my keyboard after the recent changes had been pushed and I noticed a very strange behavior with certain keys.

I have the following settings for mouse behaviors:

&mmv {
    acceleration-exponent = <1>;  // 1
    time-to-max-speed-ms = <300>; // 300
    delay-ms = <0>;
    trigger-period-ms = <8>;      // 16
};
&msc {
    acceleration-exponent = <1>;  // 0
    time-to-max-speed-ms = <40>;  // 300
    delay-ms = <0>;
    trigger-period-ms = <8>;      // 16
};

In both nodes the delay-ms is set to 0. But I clearly have a delay with (and only with) &msc SCRL_DOWN and &mmv MOVE_LEFT now. Could it be caused by some regression?

Sorry. Just tested it on another keyboard and the issue is most likely caused by #2042

Yep. I can confirm it now. Once I add at least one Antecedent Morph behavior to the keymap I end up with a very small delay for &msc SCRL_DOWN and &mmv MOVE_LEFT.

After some more digging I discovered that the key positions of &msc SCRL_DOWN and &mmv MOVE_LEFT in my keymap are shared with these combos:

            combo_key_repeat {
                timeout-ms = <U_COMBO_TIMEOUT>; // = 75
                key-positions = <17 18>;
                bindings = <&key_repeat>;
                require-prior-idle-ms = <U_COMBO_STREAK_DECAY>; // = 300
            };
            combo_caps_word {
                timeout-ms = <U_COMBO_TIMEOUT>;
                key-positions = <16 19>;
                bindings = <&caps_word>;
                require-prior-idle-ms = <U_COMBO_STREAK_DECAY>;
            };

And it appears that the combos are the (possibly main) contributors of these lags. But the funny thing is that the lag was non-existent even with combos until antecedent_morph_keycode_state_changed_listener started to run. So, it looks like both combos and Antecedent Morph listener contribute to these lags.

Excluding Mouse layer from global combos, like:

            combo_key_repeat {
                timeout-ms = <U_COMBO_TIMEOUT>; // = 75
                key-positions = <17 18>; 
                layers = <ENG DIK QWR>; // <-------
                bindings = <&key_repeat>;
                require-prior-idle-ms = <U_COMBO_STREAK_DECAY>; // = 300
            };
            combo_caps_word {
                timeout-ms = <U_COMBO_TIMEOUT>;
                key-positions = <16 19>;
                layers = <ENG DIK QWR>; // <-------
                bindings = <&caps_word>;
                require-prior-idle-ms = <U_COMBO_STREAK_DECAY>;
            };

completely fixes this lag. So, I guess global combos shouldn't overlap with Mouse behaviors.


### Examples

The following will send a scroll down event to the host when pressed/held:

Choose a reason for hiding this comment

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

Did you mean mouse movement down event?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request pointers Pointer related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.