gpioutil.Debounce not actually implemented

[lutzky]

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:

• It doesn't actually denoise nor debounce #9
  • No waiting nor time comparison occurs; the only use of the time package is for referencing the time.Duration type
  • The denoise and debounce 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:
  • Documentation doesn't mention that gpioutil.Debounce is not fully implemented #8
• The parameter in both and is ignored #7
  • It's replaced with gpio.BothEdges
• There is no way to specify desired pull in #6
  • It's unclear whether it should be running In on its own anyway.

(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.

[maruel]

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.

[lutzky]

OK, at the very least I'll do the doc warning.

[lutzky]

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)

[maruel]

#TIL

[lutzky]

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)?

[maruel]

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.

[lutzky]

Understood. So after the docfix is in, I'll see if I can take a whack at implementing the logic for v3.

[lutzky]

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

	// Read implements gpio.PinIO.
	//
	// It is the smoothed out value from the underlying gpio.PinIO.
	func (d *debounced) Read() gpio.Level {

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?

[maruel]

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.

[lutzky]

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?

[maruel]

hence my "I'm not sure it was a good idea" :)

[lutzky]

Fair enough 😅
What do you think of my proposal?

[maruel]

lgtm

[lutzky]

Alright, started on this in #12, LMK what you think.