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

Debouncing #171

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

Debouncing #171

wants to merge 7 commits into from

Conversation

birefringence
Copy link

The switches in the Byteboi are rather bouncy and unreliable. First, I tried to improve the debounce code. There was some effect, but I was not completely satisfied. Then, I replaced the switches (now: Omron B3F-4050), which had a much better effect. With this improvement, the old code works fine as well.

Still, I would like to offer the debounce code, which may have some benefit for someone not wanting to solder. It also has less delay, because it reacts immediately if the button was in a stable position before.

I do understand if you reject this if you find it unnecessary!

The delay after display wakeup had to be increased, because 5ms had a very high probability of causing glitches that persisted during use (small vertical offset and incorrect pixel values). I hope this small increase does not hurt any other devices.

Audio output via the internal DAC using only one channel was completely broken. I tried to fix this in a generic way.
@ducalex ducalex force-pushed the dev branch 2 times, most recently from d49dbb1 to a983bf8 Compare November 29, 2024 23:28
@ducalex
Copy link
Owner

ducalex commented Nov 29, 2024

I'm not convinced that your code truly improves the situation, but I will try it this weekend and see how it feels!

One problem with our current debouncing is that the scheduler works at 100hz so we only get values every 10ms.

10ms is bad because it doesn't detect minor bounce and it forces a 20ms lag before the input is registered or released.

Perhaps we should try to set freertos' tick rate to 250hz or 500hz (and change the input task' rg_task_delay) and see if it's feels more responsive/accurate.

@ducalex
Copy link
Owner

ducalex commented Nov 30, 2024

I do agree that debounce should be configurable by the target (maybe even per button, but that's too much for now).

So I stole the idea and added it to config.h :). Your #ifdef in rg_input.h is no longer necessary.

@ducalex ducalex force-pushed the dev branch 5 times, most recently from f2d4a67 to 1069d3a Compare November 30, 2024 19:11
@birefringence
Copy link
Author

Well, having a reduced sample rate already is a debouncing strategy, so I'm not sure if it makes sense to increase it ;-)

My code already reduces the lag to 10ms. It limits the "repeat rate" of the keys to 10ms * debounce_level.

In any case, I'm happy with my new switches, which reduce the bounce time from sometimes of the order of 100ms to <5ms. This is has an enormous impact on the feeling of the game play.

@ducalex
Copy link
Owner

ducalex commented Dec 4, 2024

I've tested the changes but they produce bad results on devices with resistor dividers. For example on the ODROID-GO pressing UP often results in a phantom DOWN.

I don't know if the ADC is just very slow to react to voltage changes or if it's the resistors value causing a slow ramp up, but evidently sometimes we sample before it has reached its full potential so the wrong button ends up being registered.

It might be possible to have a different debounce mechanism for analog vs digital, so that digital buttons still benefit from the proposed change. But as is this patch is a no-go...


As a side note, wouldn't this simpler change effectively achieve the same thing as your patch?:

  debounce[i] = ((debounce[i] << 1) | ((state >> i) & 1));
  debounce[i] &= debounce_level;

+ // Button becomes pressed as soon as state goes from low to high and it gets released once
+ // state is low for two consecutive cycles.
+ if (debounce[i] == 0x01) // Pressed
- if (debounce[i] == debounce_level) // Pressed
  {
      local_gamepad_state |= (1 << i);
  }
  else if (debounce[i] == 0x00) // Released
  {
      local_gamepad_state &= ~(1 << i);
  }

@birefringence
Copy link
Author

This code has a delay on the release event, even if the button was stable before. However, since the release is less timing sensitive for most types of game play, I guess the effect is about the same and I agree that it is much simpler.

Anyway, I'm not mad if you just leave it as is ;-)

Thanks for explaining how the ODROID-GO works. I wasn't aware of of this kind of implementation.

@ducalex
Copy link
Owner

ducalex commented Dec 4, 2024

I've done some measurements, when I press UP the voltage takes ~4ms to meet the UP threshold and almost 10ms to reach max voltage. Releasing it takes about 1-4ms to get back to 0. Both values vary a bit depending on how abruptly I press/release, so contact resistance is apparently part of it.

This is a lot slower than I would have expected but would definitely explain why we often end up measuring in the middle of the curve and why we can't trust single readings like we can for digital. I'm attaching an example log, reading the ADC every 100us whilst I press UP. adc.txt

I think narrowing valid ranges might help a little, but won't fix the issue entirely. So the only way to improve the ADC without breaking any device out there would be to read much more frequently and only accept a value once it's been stable for 2-3ms. Problem is we can't do that without blocking the core, unless we increase the tick rate.

In summary:
I think the main goal would be to make debounce configurable per button, so that special buttons (analog) can have special debouncing. A secondary goal is to improve ADC and that might be via increasing the sampling rate through tick rate increase if performance allows.

ducalex added a commit that referenced this pull request Dec 5, 2024
Also now the constants use an actual level rather than a bitmask value, it's more user friendly.

This change is related to #171 but isn't a replacement/solution for the discussed changes over there.
@ducalex ducalex force-pushed the dev branch 4 times, most recently from 93c53f4 to 48a164a Compare December 24, 2024 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants