Reimplements WPM feature to be smaller & precise

[vectorstorm]

Changes the WPM feature to provide a correct WPM value instead of only a rough estimate.

This change is similar to the one in this PR, which I hadn't known about at the time I made this change locally. It's worth checking that one out for comparison to this one!

Description

The original WPM feature worked by measuring the time between two keypresses, estimating a WPM value from that delay between keypresses, and then interpolating a rolling average "WPM" value by 4.87% of the difference between the current average and the new calculation (which we're told is equivalent to averaging over 40 keypresses). If no keys are pressed, then after one second the rolling average would be reduced by 4.87%.

There were some problems with this original calculation; if you were typing at 100wpm and then entirely stopped typing, it took the original implementation approximately 45 seconds for your WPM value to drop back to 0, because it was only dropping by less than 5% per second.

This new implementation instead uses a ring buffer. We define 6 "periods" within this buffer, each representing half a second of time. We track the number of keys pressed during each period, with a timer telling us when to move from one period to the next within the ring buffer, and then we can simply count the number of keys pressed and how long has passed and calculate WPM exactly, without any of the mathematical complexity of the original implementation.

When WPM_ALLOW_COUNT_REGRESSION is defined, we count keys like backspace as "negative" keys. I'm not sure that this is the right behaviour; most real-world WPM calculations work by counting each mistakenly pressed key as a full negative word and ignoring backspaces entirely. However, since we can't actually detect "errors" the way that normal WPM tests do, the implementation in this PR just gives backspace presses a similar impact to what they had in the original feature. (that part of the functionality was less important to me personally, so I figured I'd make it mirror the general impact of the old behaviour as much as I could)

Since we're no longer doing a "smoothing" calculation at all, but instead are actually calculating WPM correctly over an actual period of time, we no longer need the WPM_SMOOTHING #define, and it has been removed both from the code and from the documentation.

This change saves 998 bytes in the firmware, in my tests, compared against the previous implementation, and maintains all the same external interfaces as the original implementation, so it should just work for everything that was already using the WPM feature; they'll just get more accurate WPM numbers now than they did before.

Discussion Points:

• Should the number of measurement periods and/or their duration be exposed to make them configurable in different keymaps?

It'd take a small amount of extra space to store more sampling periods, or the sampling period could be made longer without adding extra periods just by moving those #define values (MAX_PERIODS and PERIOD_DURATION) from wpm.c to wpm.h, with changed names to make it more obvious that they're part of the WPM feature. I didn't do that in the initial PR because I felt like the 3-second measurement period was a pretty good starting point and I'm not sure whether anyone really wants their keyboard storing WPM data for much longer than that, so maybe best to keep configuration options and documentation as light as possible.

But I'm very interested in feedback here and could absolutely amend this PR with those configuration options if anybody feels that they might be useful to somebody!

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Issues Fixed or Closed by This PR

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[vectorstorm]

Ah, the qmk format-c tool wasn't mentioned in any of the documents linked from the PR process. I've run that over the files touched in the commit and updated the PR.

[drashna]

Testing this out, it seems to jump around a lot. It's not a smooth increase or drop, but very drastic and very sudden. I think that the smoothing code was explicitly to prevent this.

[vectorstorm]

Mm, interesting. For my use case, I definitely wanted much faster updates to give me a pretty accurate current-wpm value as I typed.

If the idea is that we want the value to move more smoothly, then there are definitely some strategies we could use to ease the transitions from one value to another (without going so far as taking 45 seconds to drop from 100wpm back to 0wpm, ofc!)

[vectorstorm]

This new commit should not be merged as is!

This updated commit is an experimental revision which values making the returned WPM changes occur much more smoothly. This necessarily introduces some lag into the updates, but I've tried to minimise that. I'm interested in whether this sort of behaviour is more what the maintainers have in mind for the feature.

If so, please let me know and I'll give it a proper spruce and tidy!

[drashna]

Yeah, that feels a lot better.

It feels like it may be updating a bit fast still... but ... it creates a "rolling" sort of effect for the one's place, which is actually neat.

[vectorstorm]

Here's another revision on this update. New in this update:

• Exposes the WPM sampling period as the configurable #define WPM_SAMPLE_SECONDS, with a default value of 5. So it averages your typing speed over the past 5 seconds.
• Exposes a new configurable #define WPM_UNFILTERED (defaults to undefined), which avoids all processing of WPM results and instead just returns them raw. This saves about 70 bytes, and saves a whole bunch of latency. So if you're doing something where you're driving something off WPM values (for example, an OLED animation or RGB matrix effects) you might want to use this raw unfiltered WPM value. If you're displaying it as a number, though, you definitely want the nicer filtering!
• Exposes a new option WPM_LAUNCH_CONTROL (defaults to undefined). If set, then WPM calculations will begin from the moment you start typing; they won't include 0-keypress time when the WPM value being output was 0. (adds about 20 bytes to firmware)

Stuff still to discuss:

'Launch Control' is based on the name of the feature in some electric cars which optimise for fast acceleration from a standing start, but maybe that's too whimsical/esoteric for the name of a feature like this. It's really just intended to avoid the latency you get in accurate WPM readings when you're just starting to type and you have a really large sample buffer which is mostly empty because you haven't been typing that long yet.

I'm not in love with the name 'WPM_UNFILTERED', just because you wind up with code like #if !defined(WPM_UNFILTERED) which feels like a double-negative to me. Maybe 'WPM_RAW'? Or we could go the other direction and have it be an enabled-by-default WPM_SMOOTH to remain similar to the naming of the original feature? (but then people who want to turn it off have to #undef it which always feels ugly, or maybe you need to rules.mk WPM_SMOOTH = no or something.) Interested in feedback!

Finally, it's worth noting that I have not tested that this is working on the slave side of a split keyboard, yet. That absolutely needs to get done before we can even consider merging this in.

[drashna]

Nice. And naming is hard. I think the unfiltered name should be okay, but yeah, codewise, it doesn't read the best. But I'm not sure what the best naming should be here.

I'd also say that the smooth behavior should be the default, IMO. And it should be defines.

As for the slave side, it's not needed. the wpm task is ran solely on the master side, via matrix_scan_quantum. The transport sends the value over to the secondary side, and sets it. No decay is done there.

[vectorstorm]

Agreed; the smooth behaviour should be and already is the default.

Another option would be to remove the ‘unfiltered’ option entirely, and instead just expose a second public function to access the unfiltered values; get_current_wpm_unfiltered(), for example, and then keymaps can use whichever they prefer or even both. Compiling out the smoothing logic only saves something like 70 bytes; maybe it’s not worth fussing over the complexity of having a compile option to optionally exclude it?

[drashna]

Well, to be honest, a lot of the keymaps using WPM... are squeezing every bit out, so that 70 bytes savings may be worth it. Eg, it may be worth hiding get_current_wpm_unfiltered() behind a define.

And I have to say, I really do like the current implementation for it. It's VERY pleasing, graphically.

[vectorstorm]

I've tried this out now and can confirm that this does not work on the slave side of my crkbd; I'm guessing the slave side might still be calling into decay_wpm(), which is overwriting the synchronised wpm value from the master side.

..actually, I don't have wpm being sync'd between halves over my crkbd even using the current develop branch's version of wpm.c/h

Whoops, no, I just hadn't set #define SPLIT_WPM_ENABLE in my config.h; thanks to @tzarc for pointing that out to me on Discord! Works great.

[drashna]

Just a heads up, launch control is bugged. It works initially, but it can get into a state where it just .... cycles numbers randomly, and may or may not stop.

[vectorstorm]

Just a heads up, launch control is bugged. It works initially, but it can get into a state where it just .... cycles numbers randomly, and may or may not stop.

I've reproduced this without the launch control feature; this problem is definitely in the smoothing itself. Working on sorting it out now!

[drashna]

Just a heads up, launch control is bugged. It works initially, but it can get into a state where it just .... cycles numbers randomly, and may or may not stop.

I've reproduced this without the launch control feature; this problem is definitely in the smoothing itself. Working on sorting it out now!

I haven't seen it do it on the smoothing option, but it happens very reliably on the launch control, at least.

[vectorstorm]

I haven't seen it do it on the smoothing option, but it happens very reliably on the launch control, at least.

It happens for me pretty easily if increase the smoothing time window to like ten seconds, even without launch control. I'm pretty certain that it's an integer overflow problem when it's transitioning between very different numbers (which can happen more easily with launch control enabled!)

Still looking for a way to reformulate the smoothing to make it more robust without losing any of its charm.

[drashna]

Any updates on this?

[vectorstorm]

Apologies, life has been absurdly busy! I still really want to get back to this but I'm not sure when I'm going to have the spare time.

[drashna]

Apologies, life has been absurdly busy! I still really want to get back to this but I'm not sure when I'm going to have the spare time.

No worries! I'm throwing the "in progress" label on the PR to make sure stale-bot doesn't close the PR, for now. Look forward to when you have more free time (but it's already working pretty well, as is, aside from stated issues)

[vectorstorm]

This build has made a few changes:

1. Rebased onto latest develop branch.
2. Adds a "WPM_SAMPLE_PERIODS" define, which configures how many bytes we're allowed to use to store keypress data. Higher values lead to smoother decay of displayed wpm values. (since this change overall saves so much space in the firmware I've defaulted it to a value of 50; in previous commits in this PR it had been hardcoded to 4, but I was finding that was too few especially when WPM_SAMPLE_SECONDS was high)
3. Converted both timers to use 32-bit timers, since @tzarc clarified that the 16-bit timers roll over at 0.65 seconds. (in retrospect, maybe the 'latency' timer could go back to 16-bit; I don't think it should ever need to track more than 0.1 seconds)
4. Updated documentation with data for change 2, above.

A couple other notes:

• I'd argue that WPM_ESTIMATED_WORD_SIZE should default to 6 instead of 5, since spaces and punctuation are being counted as key presses. This yields WPM values which are much closer to what you'd get from TypeRacer or similar, in my experience. (I've left it at 5 in this commit so as not to change existing behaviour)
• I'd also argue that WPM_SAMPLE_SECONDS should default to a much lower number than 5. Maybe 2 or 3. (I use 2 for my own keymap) With the default value of five, it takes the WPM display five seconds to get back down to zero after you stop typing, which to me feels like a really long time. And similarly, if you don't have the 'launch control' option enabled, it takes five seconds of continuous typing before it shows a realistic estimate of your typing speed. (I've left it at 5 in this commit so as not to change existing behaviour.. although the old behaviour didn't obey the 5 at all and often took closer to a minute of no typing at all before it would drop back to 0, so maybe it wouldn't matter if I dropped it a little further while I'm here?)

[vectorstorm]

This update has the missing documentation update which was overlooked from the previous commit. No code changes.

[drashna]

This is ready to go? Answered on discord: Yes.