Various fixes to how timer differences are calculated

[purdeaandrei]

Description

This PR makes 3 main changes

1. The TIMER_DIFF* macros calculated wrong values after timer overflow (1 less)
2. Improves code generation efficiency for TIMER_DIFF* macros (smaller, faster code)
3. The eager_pr, and eager_pk debounce algorithms had a serious problem with how they tracked time.

For details please see each commit message in turn.

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.
• 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).

NOTE: I have tested the changes using a atmega32u4 devboard, with some synthetic tests. I have not tested with an actual keyboard.

[purdeaandrei]

I've sent a subset of these changes to TMK as well: tmk/tmk_keyboard#646

[alex-ong]

I have arrived!

I can understand why a +1 might be necessary (fenceposting)
But i'm not understanding the difference between

static uint16_t custom_wrap_timer_reference = 0;
static uint8_t custom_wrap_timer_last_value = 0;

static uint8_t custom_wrap_timer_read(void) {
    uint16_t custom_wrap_timer_new_reference = timer_read();
    uint16_t diff = custom_wrap_timer_new_reference - custom_wrap_timer_reference;
    custom_wrap_timer_reference = custom_wrap_timer_new_reference;
    custom_wrap_timer_last_value = (custom_wrap_timer_last_value + diff) % (MAX_DEBOUNCE + 1);
    return custom_wrap_timer_last_value;
}

and timer_read() % (MAX_DEBOUNCE +1 if necessary)

(timer_read() % MAX_DEBOUNCE) simply makes the timer go from 0->max_debounce-1 and loop instead of 0->65535.

With any timer, TIMER_DIFF is guaranteed to give the time difference between two timestamps, assuming of course that you didn't double overflow.

Am I missing something here?

[purdeaandrei]

@alex-ong Have you read my commit message from here? 84eb44d

It explains it pretty clearly.
Please read all of my commit messages, they are intended as clear documentation for the reason of the change.

[purdeaandrei]

MAX_DEBOUNCE is 250, but the counter only goes up to 35 when the 16-bit timer value overflows (every second or so)

[alex-ong]

@alex-ong Have you read my commit message from here? 84eb44d

It explains it pretty clearly.
Please read all of my commit messages, they are intended as clear documentation for the reason of the change.

That diagram is well written and 100% explains it!

For performance reasons, do you think there's a way of changing max_DEBOUNCE to be a friendlier multiple of 65535? That way it will work sans the potentially expensive "finding the real difference and adding it back again"

[alex-ong]

divisor 1 numerator 65535
divisor 3 numerator 21845
divisor 5 numerator 13107
divisor 15 numerator 4369
divisor 17 numerator 3855
divisor 51 numerator 1285
divisor 85 numerator 771
divisor 255 numerator 257
divisor 257 numerator 255
divisor 771 numerator 85
divisor 1285 numerator 51
divisor 3855 numerator 17
divisor 4369 numerator 15
divisor 13107 numerator 5
divisor 21845 numerator 3

So (timer_read() % 85(+1?)) could work?
As long as your debounce is sub 85 milliseconds it should be fine.

[purdeaandrei]

For performance reasons, do you think there's a way of changing max_DEBOUNCE to be a friendlier multiple of 65535? That way it will work sans the potentially expensive "finding the real difference and adding it back again"

Yes, but that will only work with powers of two.
That is MAX_DEBOUNCE = 127 (with %128 in the formula) is the biggest that would work, and would leave space for the ELAPSED value.
I don't think it's that much performance loss though. This code only executes once per scan,
so it might not even be measureable.

[alex-ong]

For performance reasons, do you think there's a way of changing max_DEBOUNCE to be a friendlier multiple of 65535? That way it will work sans the potentially expensive "finding the real difference and adding it back again"

Yes, but that will only work with powers of two.
That is MAX_DEBOUNCE = 127 (with %128 in the formula) is the biggest that would work, and would leave space for the ELAPSED value.
I don't think it's that much performance loss though. This code only executes once per scan,
so it might not even be measureable.

Oh right, i was dumb and did 65535 instead of 65536.

I also think that % MAX_DEBOUNCE of a po2 with a comment explaining it needing to be a power of 2, due to timer overflow alignment

is clearer than getting the timer, calculating the real timer difference and adding back to the modulo'ed counter

[purdeaandrei]

matrix scan rate on a 10x10 fake atmega32u4 board:
eager PK with fix: 1171 Hz
eager PK with %128: 1190 Hz

[purdeaandrei]

Maybe I would do something like

#if DEBOUNCE > (127 - 50)
....my code from current state of this PR....
#else
#define MAX_DEBOUNCE 127
#define custom_wrap_timer_read() (timer_read() % (MAX_DEBOUNCE + 1))
#end

Note the -50 in there.
MAX_DEBOUNCE doesn't just limit the maximum amount of debounce available, because you also need some padding after the debounce has elapsed (50ms is an arbitrary number I just chose), in case the debounce algorithm doesn't get called in time. Otherwise, you might overflow, and the DIFF will return a value smaller then DEBOUNCE, even if it's larger

[purdeaandrei]

@alex-ong Does that sound okay to you? You get the benefit of both compatibility with high debounce values, and the speed of low debounce values.

[alex-ong]

eager PK with fix: 1171 Hz
eager PK with %128: 1190 Hz

Fast research!

Yeah, so probably not measurably slower. I can't do the math right now because my math's been failing me all day, the "slow" version takes 0.02ms (or 0.000002s) / cycle?

If i was an approver, I would approve it as is, and also totally put a PR to change it to %128 for speeeeed meeemes. (note i am not an approver on this)

Note the -50 in there.

Do you have an example of why the -50 buffer is required?

The case you're talking about is if between updates, it takes longer than MAX_DEBOUNCE right? so you're saying if the real-time is > 128ms, it will report a number between 0 and 128ms? (i.e double overflow). I'm assuming that no keyboard scans slower than 10hz, which i think is reasonable.

I think it's also reasonable to assume DEBOUNCE would be < 100?, I don't see much reason for having both solutions, since they both work and i doubt there's a use-case with DEBOUNCE > ~50. Someone who isn't me can adjudicate which option is better, just putting forward my side.

[purdeaandrei]

Yeah, so probably not measurably slower. I can't do the math right now because my math's been failing me all day, the "slow" version takes 0.02ms (or 0.000002s) / cycle?

It means, that the current PR version is this much slower per scan:

>>> (1/1171. - 1/1190.) * 1000 * 1000
13.634830533408863  (microseconds)

Do you have an example of why the -50 buffer is required?

Let's say you have a keyboard at 10 Hz scan rate, that's 100ms
Let's say you set DEBOUNCE = 120
the times you will read, in order will be:
100
200 % 128 = 72 -- debounce elapsed but to you it looks like time is going backwards
300 % 128 = 44
400 % 128 = 16
500 % 128 = 116
600 % 128 = 88
... 900 % 128 = 4
1000 % 128 = 104
1100 % 128 = 76
Looks like it would never deem debounce to be elapsed, and it would never send keycodes anymore.
I know it's an unlikely situation, but it is support that we would be dropping by just changing the value of MAX_DEBOUNCE

(note i am not an approver on this)

Alright, let's see what an official approver would say. I'm on the side of implementing that #if type code above for speed, but I don't wanna implement it then have it be reverted if an official approver thinks it's way too complicated.

I'm also fine with only using %128, as long as it's not determined to be a breaking change.

So any approvers would like to review?

[purdeaandrei]

Example in previous comment edited

[alex-ong]

Right i can see how your example works; it only occurs when the scanrate is extremely low / the debounce is high, and a double overflow occurs.

The slowest keyboard in qmk still scans at 300hz, here's the same numbers but with 100hz scanning + 100ms debounce. I don't really forsee something as pitiful as 10hz scanning (remember normal keyboards are in 300-1000 range; mine is 14000 lmao)

Raw Timer	Modulo timer	TIMER_DIFF(modulo,initial_modulo,128)	Timer expired? (TIMER_DIFF > 100)
120	120	0 (start value)	no
130	2	10	no
140	12	20	no
...	...	...	...
220	92	100	yes

I understand that the %128 is not "correct", but it is correct in real use, and simple to digest.

[purdeaandrei]

purdeaandrei removed the request for review from qmk/collaborators 1 minute ago

That's strange, I didn't intend to remove any request to review. I only clicked the re-request review for @vomindoraan , but that for some reason removed qmk/collaborators, and I can't edit at all the list of review requests.

[vomindoraan]

Looks good! One final change I'd make (sorry for not catching this earlier) would be to rename wrap_timer_read() to wrapping_timer_read(), since “wrap” could be interpreted as an action at a casual glance, whereas the latter name clearly conveys that you're reading something from a timer that wraps (wrapping timer).

[purdeaandrei]

@vomindoraan Done

[purdeaandrei]

Thanks!