gpioutil.Debounce not actually implemented
lutzky opened this issue ยท 14 comments
Apologies if I'm misreading. I have implemented manual debouncing and denoising for my own project, before realizing that gpioutil.Debounce
exists. It would provide a cleaner API to do the same thing, so I looked into switching to using it. However, observing the current version of debounce.go
, it appears that:
- #9
- No waiting nor time comparison occurs; the only use of the
time
package is for referencing thetime.Duration
type - The
denoise
anddebounce
properties are only ever compared to 0 or copied around - The file appears mostly unchanged since google/periph@97f750e, which has the commit message explicitly saying "No algorithm yet."; it is nonetheless no longer in an
experimental
path, so: - #8
- No waiting nor time comparison occurs; the only use of the
- #7
- It's replaced with
gpio.BothEdges
- It's replaced with
- #6
- It's unclear whether it should be running
In
on its own anyway.
- It's unclear whether it should be running
(I've tried to keep the checkbox entries in actual tasks, so they can be used with Task lists).
Am I missing something? I think, at least, that the documentation should clarify the state of this code.
Side-note: While I was aware of the importance of debouncing, denoising was newer to me, and I found out about it the annoying way (I have phantom GPIO "button presses" about every 8 hours; super annoying to debug). Once Debounce is implemented correctly, it's probably worth mentioning in the documentation of WaitForEdge.
There's historical reasons for this. 3 years ago I started looking at options to recreate the conn package as I don't feel the current interfaces are optimal. Then go modules were forced and I spent all my free time on splitting the repositories into real go packages.
You can find a bit more at:
- https://docs.google.com/document/d/1ltZHq1Az6kLH3vrPS3XFAxT2XOOStKgHqwtnT5o9j6k/edit
- https://docs.google.com/document/d/1vQdZdoOMaIan7dKwcAzqHbfM_LCnnSMUQvqypRkjohM/edit
- https://docs.google.com/spreadsheets/d/1BG4QDS6cWM0eIdYInjfoNccHI63w5BXAdwSeCYNbPu4/edit#gid=0
Because of the uncertainty with how the GPIO functions would look like, I didn't spend time to fix the Debounce code when I carried it over from experimental at https://github.com/google/periph/tree/main/experimental/conn/gpio/gpioutil when I removed experimental when I created the new repositories. There's more at https://periph.io/news/2020/a_new_start/.
So that was a lot of words and links to say that help would be very appreciated.
OK, at the very least I'll do the doc warning.
Could you please hit the "Convert to issue" button on those tasks? (That should make it link up nicely and I can reference that in the pull request)
#TIL
After #10:
So, to clarify, should the conn package be considered stable enough for it to be worth actually implementing this now (I mean, it's been 3 years)? Or is v4 right around the corner (...and then v4 needs an implementation of this)?
I started multiple changes for v4 but given my current work workload it's unlikely for me to have a v4 in the next year.
Understood. So after the docfix is in, I'll see if I can take a whack at implementing the logic for v3.
Looking at the current documentation, the Read
method is also documented as being affected by debouncing:
conn/gpio/gpioutil/debounce.go
Lines 59 to 62 in 9ee6d81
This is significantly more complicated to implement than the WaitForEdge
variant, which would boil down to something like this:
func (d *debounced) WaitForEdge(timeout time.Duration) bool {
prev := d.PinIO.Read()
for {
if !d.PinIO.WaitForEdge(timeout) {
return false
}
time.Sleep(d.denoise)
if d.PinIO.Read() != prev {
return true
}
}
}
Looking around, there's a C library called pigpio which has a set_glitch_filter
that seems to have similar functionality, and indeed only applies to their equivalent for WaitForEdge
(although the implementation seems more complicated).
Getting this to work for Read
as well is... tricky? I mean, I guess it could do this:
func (d *debounced) Read() gpio.Level {
for {
prev := d.PinIO.Read()
time.Sleep(d.denoise)
if d.PinIO.Read() == prev {
return prev
}
}
}
Is that what you had in mind?
My thinking was to save one read if the last read occurred within d.denoise, but in hindsight I'm not sure it was a good idea.
I'm not sure how that would work... and I don't think it can work in the case of "user calls Read
just once". Am I missing something?
hence my "I'm not sure it was a good idea" :)
Fair enough ๐
What do you think of my proposal?
lgtm