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

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
52 changes: 32 additions & 20 deletions maccel/maccel.c
Original file line number Diff line number Diff line change
Expand Up @@ -6,29 +6,34 @@
#include "maccel.h"
#include "math.h"

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)
#endif
#ifndef MACCEL_CPI_THROTTLE_MS
# define MACCEL_CPI_THROTTLE_MS 200 // milliseconds to wait between requesting the device's current DPI
# define MACCEL_LIMIT 0.2 // (M) lower limit of accel curve (minimum acceleration factor)
#endif
#ifndef MACCEL_LIMIT_UPPER
# define MACCEL_LIMIT_UPPER 1 // upper limit of accel curve, recommended to leave at 1; adjust DPI setting instead.
#endif
#ifndef MACCEL_CPI_THROTTLE_MS
# define MACCEL_CPI_THROTTLE_MS 200 // milliseconds to wait between requesting the device's current DPI
#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

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
/**
* Scale acceleration curve's v-input so that its params are not uncannily small
* and floats do not overflow in the accel formula (eg. `exp(709.8)` for doubles).
* Eventually the accel formula is calculated as if the pointer reports @ 1000 CPI,
* but "counts" are expressed as float (vs integer for the hw "counts").
*/
#define MACCEL_MAGNIFICATION_DPI 1000.0

maccel_config_t g_maccel_config = {
// clang-format off
Expand Down Expand Up @@ -103,32 +108,31 @@ void maccel_toggle_enabled(void) {
#define CONSTRAIN_REPORT(val) (mouse_xy_report_t) _CONSTRAIN(val, XY_REPORT_MIN, XY_REPORT_MAX)

report_mouse_t pointing_device_task_maccel(report_mouse_t mouse_report) {
static uint32_t maccel_timer = 0;
burkfers marked this conversation as resolved.
Show resolved Hide resolved
// rounding carry to recycle dropped floats from int mouse reports, to smoothen low speed movements (credit @ankostis)
static float rounding_carry_x = 0;
static float rounding_carry_y = 0;
// time since last 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:
// Reset timer on non-zero reports.
burkfers marked this conversation as resolved.
Show resolved Hide resolved
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.

// 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;
// Limit expensive calls to get device cpi settings only when mouse stationary for > 200ms.
// Limit expensive calls to get device cpi settings only when mouse stationary for some time.
burkfers marked this conversation as resolved.
Show resolved Hide resolved
static uint16_t device_cpi = 300;
if (delta_time > MACCEL_CPI_THROTTLE_MS) {
device_cpi = pointing_device_get_cpi();
}
// calculate dpi correction factor (for normalizing velocity range across different user dpi settings)
const float dpi_correction = (float)1000.0f / device_cpi;
const float dpi_correction = (float)MACCEL_MAGNIFICATION_DPI / device_cpi;
Copy link
Owner

@burkfers burkfers Mar 14, 2024

Choose a reason for hiding this comment

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

Quote @Wimads:

explain x1000 factor: don't accept I don't think this should be turned into a variable, and thus also doesn't require any extra explanation. It scales the v.in range to equivalent of DPI 1000, but that is a fairly arbitrary number - might as well have set it to 20000, and the curve would still work the same. The only thing it would affect is the usable range of the main variables, which is enough to discourage people from playing around with it imho.

I approve of turning a magic number into a preprocessor constant. However, it should have a more meaningful name, for example MACCEL_CPI_EQUIVALENT_SCALE. Something that implies this is the dpi-equivalence we're scaling to.
Further comments should not be placed to discourage modification, as it's not intended as a user-tunable parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will rename it into MACCEL_MAGNIFICATION_CPI_EQUIVALENT and add the trimmed-down description proposed in discord along with a discouragement admonition:

Scale acceleration curve's V-input @ 1000 CPI so that its parameters
are not uncannily small and/or formula floats overflow.
Any modified value will invalidate curve parameters.

Copy link
Collaborator

@Wimads Wimads Mar 19, 2024

Choose a reason for hiding this comment

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

Why magnification? I don't understand that wording in this context. Its normalizing (or scaling if you prefer) v-in to an equivalent of 1000 CPI. So MACCEL_CPI_EQUIVALENT_SCALE as burkfers suggested makes more sense to me. Or MACCEL_NORMALIZING_CPI_EQUIVALENT perhaps.

As for the comment, something like this covers my intention better I think:
// Normalizing input velocity to an equivalent of CPI=1000, for consistent accel behavior across different CPI settings.
// Modifying CPI_EQUIVALENT will invalidate the curve parameters.
^^ which should then replace this existing comment:
// calculate dpi correction factor (for normalizing velocity range across different user dpi settings)

// calculate euclidean distance moved (sqrt(x^2 + y^2))
const float distance = sqrtf(mouse_report.x * mouse_report.x + mouse_report.y * mouse_report.y);
// calculate delta velocity: dv = distance/dt
Expand All @@ -153,11 +157,11 @@ report_mouse_t pointing_device_task_maccel(report_mouse_t mouse_report) {
const mouse_xy_report_t x = CONSTRAIN_REPORT(new_x);
const mouse_xy_report_t y = CONSTRAIN_REPORT(new_y);

// console output for debugging (enable/disable in config.h)
#ifdef MACCEL_DEBUG
// console output for debugging (enable/disable in config.h)
burkfers marked this conversation as resolved.
Show resolved Hide resolved
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:%+8.3f\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), distance_out - CONSTRAIN_REPORT(distance));
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:

debug-dump D-diffs: don't accept. Same as V-diffs.

I do not have a strong opinion on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will separate it in x2 commits, one capitalizing letters, another one showing the outputs as ± diffs.

You really have to run the code to judge the change, by watching the debug-print as you slowly move the mouse to see wether you like the results.
In the end, whether the label precisely describes the value or not is less of importance that helping us tune the curve parameterss.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps make it V.diff instead of V.out? That would seem more intuitive to me.

#endif // MACCEL_DEBUG

// report back accelerated values
Expand Down Expand Up @@ -187,22 +191,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
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

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