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.

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

Minor carry timeout, formula letters, fixes and doc nitpicks #10

Open
wants to merge 11 commits into
base: maccel-dev
Choose a base branch
from
Open

Minor carry timeout, formula letters, fixes and doc nitpicks #10

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
39c6a97
doc(maccel) explain Debug-enable sample code
ankostis Mar 10, 2024
51f1691
doc(maccel) explain why low DPIs need throttling >
ankostis Mar 10, 2024
6f56b7b
doc(maccel) denote letter-symbols & and desmos on sample params
ankostis Mar 2, 2024
b8662f3
feat(maccel) stabilize debug print float widths
ankostis Mar 2, 2024
ec76f8b
feat(maccel) debug-dump V as diffs to view carry at work and >
ankostis Mar 2, 2024
9f9693b
fix(maccel) don't dump param-keys if DEBUG is off
ankostis Mar 4, 2024
e8c6d29
feat(maccel) don's reset rounding carry if not stationary
ankostis Mar 3, 2024
1d285de
refact(maccel) demote timer from module-static -> function, >
ankostis Mar 11, 2024
0817331
refact(maccel) explain x1000 factor as curve-fitting, >
ankostis Mar 12, 2024
e164bc3
feat(maccel) debug-dump D as diffs to hook the eye on 0s
ankostis Mar 12, 2024
426ba61
style(maccel) minor comment move
ankostis Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions maccel/maccel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,16 @@
static uint32_t maccel_timer;

#ifndef MACCEL_TAKEOFF
# define MACCEL_TAKEOFF 2.0 // lower/higher value = curve starts more smoothly/abruptly
# define MACCEL_TAKEOFF 2.0 // (K) lower/higher value = curve starts more smoothly/abruptly
#endif
#ifndef MACCEL_GROWTH_RATE
# define MACCEL_GROWTH_RATE 0.25 // lower/higher value = curve reaches its upper limit slower/faster
# define MACCEL_GROWTH_RATE 0.25 // (G) lower/higher value = curve reaches its upper limit slower/faster
#endif
#ifndef MACCEL_OFFSET
# define MACCEL_OFFSET 2.2 // lower/higher value = acceleration kicks in earlier/later
# define MACCEL_OFFSET 2.2 // (S) lower/higher value = acceleration kicks in earlier/later
#endif
#ifndef MACCEL_LIMIT
# define MACCEL_LIMIT 0.2 // lower limit of accel curve (minimum acceleration factor)
# define MACCEL_LIMIT 0.2 // (M) lower limit of accel curve (minimum acceleration factor)
burkfers marked this conversation as resolved.
Show resolved Hide resolved
#endif
#ifndef MACCEL_CPI_THROTTLE_MS
# define MACCEL_CPI_THROTTLE_MS 200 // milliseconds to wait between requesting the device's current DPI
Expand All @@ -27,7 +27,7 @@ static uint32_t maccel_timer;
# define MACCEL_LIMIT_UPPER 1 // upper limit of accel curve, recommended to leave at 1; adjust DPI setting instead.
#endif
#ifndef MACCEL_ROUNDING_CARRY_TIMEOUT_MS
# define MACCEL_ROUNDING_CARRY_TIMEOUT_MS 200 // milliseconds after which to reset quantization error correction (forget rounding remainder)
# define MACCEL_ROUNDING_CARRY_TIMEOUT_MS 300 // Elapsed time after which quantization error gets reset.
Comment on lines -30 to +28
Copy link
Owner

Choose a reason for hiding this comment

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

Why increase the timeout by 100 ms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 200ms, inherited from the get-cpi timeout when i split the two timeouts separate, is a device-specific value while the 300ms is a human factor.
The 300ms was like that in my original code and forgot to reason about when i cherry-picked it, had even tested higher values with no unpleasant behavior, while reducing it, it had.
Ie, when moving really slow, it's pretty common to see consecutive 0s as input for more than 200ms, either because of hw's internal buffering or because the standard bkb-mouse casing doetn't have ball-bearings but a ring ones, so there is a minor friction pinning the track-ball momentarily. Any such reset would be unwanted because the hand was still moving.

Copy link
Collaborator

@Wimads Wimads Mar 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm good with 300. Makes sense if you've observed 200ms being exceeded in tests. Might depend on mouse throttling as well I guess, so going to 300 to be safe makes sense. Though it would make sense for the CPI call timeout to be 300 then as well.

#endif

maccel_config_t g_maccel_config = {
Expand Down Expand Up @@ -110,15 +110,13 @@ report_mouse_t pointing_device_task_maccel(report_mouse_t mouse_report) {
const uint16_t delta_time = timer_elapsed32(maccel_timer);
// skip maccel maths if report = 0, or if maccel not enabled.
if ((mouse_report.x == 0 && mouse_report.y == 0) || !g_maccel_config.enabled) {
if (delta_time > MACCEL_ROUNDING_CARRY_TIMEOUT_MS) {
rounding_carry_x = rounding_carry_y = 0;
}
return mouse_report;
}
// reset timer:
maccel_timer = timer_read32();
// Reset carry if too much time passed
if (delta_time > MACCEL_ROUNDING_CARRY_TIMEOUT_MS) {
rounding_carry_x = 0;
rounding_carry_y = 0;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Quote @Wimads:

don't reset carry if not stationary: ambiguous. The result is the same (as discussed on discord). I can't judge which is more efficient, but in the grand scheme of things (heavy float maths) I don't think it matters much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still it saves x1 conditional in the significant branch, so will keep it.

Copy link
Owner

Choose a reason for hiding this comment

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

I don't see the point of this change. Doing it inside the early exit resets the carry sooner, but since it isn't used until a report passes the early exit check, it makes no difference.

Additionally, double assignments in one line do not fit qmk style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I does save a conditional on non-zero reports, which it the code path we care to optimize.

  2. Sure i can change the double assignments, but i always knew that "chained" assignments are the recommended practice when the variables are related, as it is here (x & y coordinates) because they avoids a source of errors: assigning, no?

  • But why didn't vscode gave me any warnings?
  • Which is the offending rule in clang-format?

// Reset carry when pointer swaps direction, to follow user's hand.
if (mouse_report.x * rounding_carry_x < 0) rounding_carry_x = 0;
if (mouse_report.y * rounding_carry_y < 0) rounding_carry_y = 0;
Expand Down Expand Up @@ -157,7 +155,7 @@ report_mouse_t pointing_device_task_maccel(report_mouse_t mouse_report) {
#ifdef MACCEL_DEBUG
const float distance_out = sqrtf(x * x + y * y);
const float velocity_out = velocity * maccel_factor;
printf("MACCEL: DPI:%4i Tko: %.3f Grw: %.3f Ofs: %.3f Lmt: %.3f | Fct: %.3f v.in: %.3f v.out: %.3f d.in: %3i d.out: %3i\n", device_cpi, g_maccel_config.takeoff, g_maccel_config.growth_rate, g_maccel_config.offset, g_maccel_config.limit, maccel_factor, velocity, velocity_out, CONSTRAIN_REPORT(distance), CONSTRAIN_REPORT(distance_out));
printf("MACCEL: DPI:%5i Tko:%6.3f Grw:%6.3f Ofs:%6.3f Lmt:%6.3f | Acc:%7.3f V.in:%7.3f V.out:%+8.3f D.in:%3i D.out:%3i\n", device_cpi, g_maccel_config.takeoff, g_maccel_config.growth_rate, g_maccel_config.offset, g_maccel_config.limit, maccel_factor, velocity, velocity_out - velocity, CONSTRAIN_REPORT(distance), CONSTRAIN_REPORT(distance_out));
#endif // MACCEL_DEBUG

// report back accelerated values
Expand Down Expand Up @@ -187,22 +185,30 @@ bool process_record_maccel(uint16_t keycode, keyrecord_t *record, uint16_t toggl
}
if (keycode == takeoff) {
maccel_set_takeoff(maccel_get_takeoff() + get_mod_step(MACCEL_TAKEOFF_STEP));
# ifdef MACCEL_DEBUG
printf("MACCEL:keycode: TKO: %.3f gro: %.3f ofs: %.3f lmt: %.3f\n", g_maccel_config.takeoff, g_maccel_config.growth_rate, g_maccel_config.offset, g_maccel_config.limit);
# endif // MACCEL_DEBUG
return false;
}
if (keycode == growth_rate) {
maccel_set_growth_rate(maccel_get_growth_rate() + get_mod_step(MACCEL_GROWTH_RATE_STEP));
# ifdef MACCEL_DEBUG
printf("MACCEL:keycode: tko: %.3f GRO: %.3f ofs: %.3f lmt: %.3f\n", g_maccel_config.takeoff, g_maccel_config.growth_rate, g_maccel_config.offset, g_maccel_config.limit);
# endif // MACCEL_DEBUG
return false;
}
if (keycode == offset) {
maccel_set_offset(maccel_get_offset() + get_mod_step(MACCEL_OFFSET_STEP));
# ifdef MACCEL_DEBUG
printf("MACCEL:keycode: tko: %.3f gro: %.3f OFS: %.3f lmt: %.3f\n", g_maccel_config.takeoff, g_maccel_config.growth_rate, g_maccel_config.offset, g_maccel_config.limit);
# endif // MACCEL_DEBUG
return false;
}
if (keycode == limit) {
maccel_set_limit(maccel_get_limit() + get_mod_step(MACCEL_LIMIT_STEP));
# ifdef MACCEL_DEBUG
printf("MACCEL:keycode: tko: %.3f gro: %.3f ofs: %.3f LMT: %.3f\n", g_maccel_config.takeoff, g_maccel_config.growth_rate, g_maccel_config.offset, g_maccel_config.limit);
# endif // MACCEL_DEBUG
Comment on lines +194 to +217
Copy link
Owner

Choose a reason for hiding this comment

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

Quote @Wimads:

don't dump if debug is off: accept. makes sense

I disagree with Wimads here: These were specifically intended as feedback while using the step keys, so you can turn all the spammy outputs off by not defining MACCEL_DEBUG, yet still receive non-spammy feedback for the step keys.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh makes sense. I think I wondered this before and concluded the same, but forgot :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will skip the commit

return false;
}
}
Expand Down
26 changes: 19 additions & 7 deletions maccel/readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,11 @@ Before configuring maccel, make sure you have turned off your OS acceleration se

Several characteristics of the acceleration curve can be tweaked by adding relevant defines to `config.h`:
```c
#define MACCEL_TAKEOFF 2.0 // lower/higher value = curve takes off more smoothly/abruptly
#define MACCEL_GROWTH_RATE 0.25 // lower/higher value = curve reaches its upper limit slower/faster
#define MACCEL_OFFSET 2.2 // lower/higher value = acceleration kicks in earlier/later
#define MACCEL_LIMIT 0.2 // lower limit of accel curve (minimum acceleration factor)
/** Mouse acceleration curve parameters: https://www.desmos.com/calculator/g6zxh5rt44 */
#define MACCEL_TAKEOFF 2.0 // (K) lower/higher value = curve takes off more smoothly/abruptly
#define MACCEL_GROWTH_RATE 0.25 // (G) lower/higher value = curve reaches its upper limit slower/faster
#define MACCEL_OFFSET 2.2 // (S) lower/higher value = acceleration kicks in earlier/later
#define MACCEL_LIMIT 0.2 // (M) upper limit of accel curve (maximum acceleration factor)
burkfers marked this conversation as resolved.
Show resolved Hide resolved
```
[![](assets/accel_curve.png)](https://www.desmos.com/calculator/k9vr0y2gev)

Expand All @@ -105,14 +106,25 @@ A good starting point for tweaking your settings, is to set your default DPI sli

**Debug console**: To aid in dialing in your settings just right, a debug mode exists to print mathy details to the console. The debug console will print your current DPI setting and variable settings, as well as the acceleration factor, the input and output velocity, and the input and output distance. Refer to the QMK documentation on how to *enable the console and debugging*, then enable mouse acceleration debugging in `config.h`:
```c
#define MACCEL_DEBUG
/*
* Requires enabling float support for printf!
/**
* To view mouse's distance/velocity and curve parameters while configuring maccel,
* set `CONSOLE_ENABLE = yes` in `rules.mk`, uncomment the lines below,
* and run `qmk console` in the shell.
* Note: requires enabling float support for printf!
*/
#define MACCEL_DEBUG
Comment on lines 108 to +115
Copy link
Owner

@burkfers burkfers Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Quote @Wimads:

Explain debug: accept with change. I agree that the little extra explanation is useful. Would add that explanation outside the code block though.

Agreed, it's a good change, but for legibility, please place the text before the code block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean not to be included in the user's config file?

My purpose was exactly that. for the user to view in his/her file the instructions to enable logging.

Copy link
Owner

Choose a reason for hiding this comment

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

That is a good point! Placing it in the code block encourages the user to place these instructions in their config, for easy context later down the road.
I'm convinced.

@Wimads?

Copy link
Collaborator

Choose a reason for hiding this comment

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

hm, I guess that makes some kind of sense. But it also feels weird to have the readme explain one part in the text, and then have the rest of the explanation in the code block :P But do as you see fit, no strong opinion from my side.

#undef PRINTF_SUPPORT_DECIMAL_SPECIFIERS
#define PRINTF_SUPPORT_DECIMAL_SPECIFIERS 1
```

Finally, linearity for low CPI settings works better when pointer task throttling enforces a lower frequency from the default 1ms, to have more time to gather "dots", ie. add something like this in your `config.h`:

```c
// Reduce pointer-task frequency (1ms --> 5ms) for consistent acceleration on lower CPIs.
#undef POINTING_DEVICE_TASK_THROTTLE_MS
#define POINTING_DEVICE_TASK_THROTTLE_MS 5
```

Comment on lines +120 to +127
Copy link
Owner

Choose a reason for hiding this comment

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

Quote @Wimads:

Explain Low DPI need throttling: don't accept. This has become irrelevant with the scale down curve; no one will still set a DPI of 100 or 200, so no need to elaborate on this in the readme imho (the more concise the readme the better).

Agreed, this is not neccessary at realistic DPI ranges.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will drop it.

## Runtime adjusting of curve parameters by keycodes (optional)

### Additional required installation steps
Expand Down