Refactoring of TIMER_DIFF

[boessu]

Issue Description

I have the feeling that TIMER_DIFF has some sort of a problem with the attribte "b" overflow. Before requesting a Pull, I would like to know the opinion of the maintainers:

starting position in file timer.h:

#define TIMER_DIFF(a, b, max) ((max == UINT8_MAX) ? ((uint8_t)((a) - (b))) : ((max == UINT16_MAX) ? ((uint16_t)((a) - (b))) : ((max == UINT32_MAX) ? ((uint32_t)((a) - (b))) : ((a) >= (b) ? (a) - (b) : (max) + 1 - (b) + (a)))))
#define TIMER_DIFF_8(a, b) TIMER_DIFF(a, b, UINT8_MAX)
#define TIMER_DIFF_16(a, b) TIMER_DIFF(a, b, UINT16_MAX)
#define TIMER_DIFF_32(a, b) TIMER_DIFF(a, b, UINT32_MAX)
#define TIMER_DIFF_RAW(a, b) TIMER_DIFF_8(a, b)

In my opinion, only the last part of TIMER_DIFF( handles the possible overflow of attribute b correctly. All other variants do have some inconsistencies if the attribute b gets an overflow. E.g. in debounce, the function will be used to figure out the difference between a as stored timestamp and b as actual timestamp. In the case of an overflow in b, it is possible that debounce does miss the delay, which can result in a bouncing / chattering. maybe that's only theory and it seems to me that this last part of TIMER_DIFF( is the part which is at the end never used anywhere... which is the reason to ask for an opinion before I try to change that. ;-)

The proposal for the change would be like this:

#define TIMER_DIFF(a, b, max) (((a) >= (b)) ? ((a) - (b)) : ((max) - (b) + (a) + 1))
#define TIMER_DIFF_8(a, b) TIMER_DIFF((uint8_t)(a), (uint8_t)(b), UINT8_MAX)
#define TIMER_DIFF_16(a, b) TIMER_DIFF((uint16_t)(a), (uint16_t)(b), UINT16_MAX)
#define TIMER_DIFF_32(a, b) TIMER_DIFF((uint32_t)(a), (uint32_t)(b), UINT32_MAX)
#define TIMER_DIFF_RAW(a, b) TIMER_DIFF_8(a, b)

[tzarc]

Honestly, seems the proposed solution is far simpler and more maintainable.

Perhaps write a quick few unit tests to validate, with values around the datatype max boundaries? I'm sure it'd be useful in the long run to prevent any future issues if someone else decides to change it in the future.

[boessu]

Hello @tzarc

Thanks for your comment. After taking a deeper look at the code and what it produces, my proposal is finally the following:

#define TIMER_DIFF_8(a, b) (uint8_t)((a) - (b))
#define TIMER_DIFF_16(a, b) (uint16_t)((a) - (b))
#define TIMER_DIFF_32(a, b) (uint32_t)((a) - (b))
#define TIMER_DIFF_RAW(a, b) TIMER_DIFF_8(a, b)

At the end it produces the exact same behavior like the actual code, which doesn't have an overflow problem I supposed (because of the casting to uint8_t, uint16_t, uint32_t).
According to my code research in this repository, the definition TIMER_DIFF( itself is just a helper definition for the definitions above, is not used everywhere else and even shouldn't be used directly (please correct me if I'm wrong). So my proposal seems to me more understandable, produces less boilerplate code through the preprocessor and prevents the use of the method TIMER_DIFF( directly, as it doesn't exist anymore.
Even though that didn't help me find a bug as I hoped (I had the hope to find a bug because of my bouncy Q6 spacebar, which seems to be more often than normal on brown switches, also on other keyboard brands...), I will make a pull request for you with this small refactoring if you agree with me.