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

ankostis · 2024-03-12T15:48:20Z

I guess the only vetted commit would be ec76f8b - feat(maccel) debug-dump V as diffs ... and e164bc3 - feat(maccel) debug-dump D as diffs ..., that i used to introspect the algo when i came up with the need for the carry, to me it still seems easier to spot when a 1 is actually emitted, or how much curry is added :-D

As always, review first commit-by-commit where reason is given, and then in total.

as shown by the notebook @ 4d9235b.

hook the eye on +0s, use & capitalize formula letters.

doc: minor explain-docs in main func.

cluster configurable/curve params.

Wimads · 2024-03-14T16:08:04Z

My thoughts commmit by commit:

Explain debug: accept with change. I agree that the little extra explanation is useful. Would add that explanation outside the code block though.
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).
Letter symbols on params: accept. I guess this could be useful, also in the comments in maccel.c
stabilize debug float prints: accept. I will pull my "not a coder" card here :P I'm not sure how this works, but I'll trust your coding knowledge over mine.
debug-dump V-diffs: don't accept. V.in and V.out are self explanatory, changing that to denote the difference will confuse users and require us explaining things. I strongly favor things being as self explanatory as possible. --- one nice thing you did though is capitalizing the start of the print labels, I think that does make it more legible at a glance.
don't dump if debug is off: accept. makes sense
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.
demote timer: unsure pulling non-coder card again. My logic would say that placing the timer here would reset it to 0 on every mouse report, but maybe I'm misunderstanding the code?
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.
debug-dump D-diffs: don't accept. Same as V-diffs.
minor comment move: ambiguous

burkfers · 2024-03-14T16:32:03Z

maccel/readme.md

 ```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


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.

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.

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?

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.

burkfers · 2024-03-14T16:33:02Z

maccel/readme.md

+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
+``` 
+


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.

ok, will drop it.

maccel/maccel.c

maccel/readme.md

burkfers · 2024-03-14T16:37:05Z

maccel/maccel.c

-    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 | Fct:%7.3f v.in:%7.3f v.out:%7.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));


Quote @Wimads:

stabilize debug float prints: accept. I will pull my "not a coder" card here :P I'm not sure how this works, but I'll trust your coding knowledge over mine.

Very good; I had done the same, but that must have gotten lost somewhere along the way.
Would you change the prints in the keycodes and in via to match? Thanks!

will do it.

maccel/maccel.c

burkfers · 2024-03-14T16:49:53Z

maccel/maccel.c

@@ -125,7 +132,7 @@ report_mouse_t pointing_device_task_maccel(report_mouse_t mouse_report) {
        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;


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.

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.

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)

burkfers · 2024-03-14T16:51:08Z

maccel/maccel.c

@@ -161,7 +161,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:%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));
+    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));


Quote @Wimads:

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

I do not have a strong opinion on this.

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.

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

maccel/maccel.c

ankostis added 11 commits March 12, 2024 17:07

doc(maccel) explain Debug-enable sample code

39c6a97

doc(maccel) explain why low DPIs need throttling >

51f1691

as shown by the notebook @ 4d9235b.

doc(maccel) denote letter-symbols & and desmos on sample params

6f56b7b

feat(maccel) stabilize debug print float widths

b8662f3

feat(maccel) debug-dump V as diffs to view carry at work and >

ec76f8b

hook the eye on +0s, use & capitalize formula letters.

fix(maccel) don't dump param-keys if DEBUG is off

9f9693b

feat(maccel) don's reset rounding carry if not stationary

e8c6d29

refact(maccel) demote timer from module-static -> function, >

1d285de

doc: minor explain-docs in main func.

refact(maccel) explain x1000 factor as curve-fitting, >

0817331

cluster configurable/curve params.

feat(maccel) debug-dump D as diffs to hook the eye on 0s

e164bc3

style(maccel) minor comment move

426ba61

burkfers requested changes Mar 14, 2024

View reviewed changes

burkfers changed the base branch from maccel-scaledowncurve+roundingcarry to maccel-dev April 9, 2024 09:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ankostis commented Mar 12, 2024

Wimads commented Mar 14, 2024 •

edited

Loading

burkfers Mar 14, 2024 •

edited

Loading

ankostis Mar 19, 2024

burkfers Mar 19, 2024

Wimads Mar 19, 2024

burkfers Mar 14, 2024

ankostis Mar 19, 2024

burkfers Mar 14, 2024

ankostis Mar 19, 2024

burkfers Mar 14, 2024 •

edited

Loading

ankostis Mar 19, 2024

Wimads Mar 19, 2024 •

edited

Loading

burkfers Mar 14, 2024

ankostis Mar 19, 2024

Wimads Mar 19, 2024

		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 \| Fct:%7.3f v.in:%7.3f v.out:%7.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));

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

Are you sure you want to change the base?

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

Conversation

ankostis commented Mar 12, 2024

Wimads commented Mar 14, 2024 • edited Loading

burkfers Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burkfers Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wimads Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wimads commented Mar 14, 2024 •

edited

Loading

burkfers Mar 14, 2024 •

edited

Loading

burkfers Mar 14, 2024 •

edited

Loading

Wimads Mar 19, 2024 •

edited

Loading