periph / conn

Go·Hardware·Lean - Interfaces
https://periph.io
Apache License 2.0
65 stars 11 forks source link

gpio: Implement debounced WaitForEdge logic #12

Closed lutzky closed 2 years ago

lutzky commented 2 years ago

Some progress on #5, #9

This does, however, introduce sleeps into tests. This can be avoided by using https://github.com/jonboulle/clockwork, but I wanted to get early feedback before I dig into that.

codecov-commenter commented 2 years ago

Codecov Report

Merging #12 (6352fe3) into main (de2ac48) will increase coverage by 0.0%. The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main     #12   +/-   ##
=====================================
  Coverage   98.3%   98.3%           
=====================================
  Files         31      31           
  Lines       2699    2708    +9     
=====================================
+ Hits        2652    2661    +9     
  Misses        44      44           
  Partials       3       3           
Impacted Files Coverage Δ
gpio/gpioutil/debounce.go 100.0% <100.0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update de2ac48...6352fe3. Read the comment docs.

lutzky commented 2 years ago

CI failure appears unrelated to my change.

maruel commented 2 years ago

gohci

maruel commented 2 years ago

About CI failure: I discovered that my nice hack to run a go module at a specific commit works if the commit exists on any branch but not on PRs. I need to update the github actions on every of the repository which sucks and I haven't found time to fix this.

The code looks good.

maruel commented 2 years ago

(My brain is fried tonight, will recheck tomorrow)

lutzky commented 2 years ago

Thanks. Progressing a bit further, I've switched from time-based testing to using clockwork, making use use of it for one of the tests so far.

Pros:

Cons:

WDYT?

maruel commented 2 years ago

Other updates:

lutzky commented 2 years ago

I've rebased on top of gosum, I hope this is indeed what you meant.

Moving Clock into gpiotest.Pin was much cleaner than I expected, I must've confused myself pretty badly there. Hopefully this will make converting the rest of the tests easier.

maruel commented 2 years ago

Oops I think I found out why canary test fails, will fix ASAP.

maruel commented 2 years ago

Actually, can you replace the go get ./... in the "Test on foo" in .github/workflows/test.yml with go get -t ./... and try again?

lutzky commented 2 years ago

That doesn't seem to be it... go.sum is... missing somehow?

maruel commented 2 years ago

\o/

It's because devices only depends on gpiotest in its unit tests, do go get ./... didn't get the clockwork dependency.

lutzky commented 2 years ago

Ah! I guess I was looking at a previous run of the checks. I'll adapt the one remaining test that uses time.Sleep.

maruel commented 2 years ago

Thanks!

maruel commented 2 years ago

Thanks for bearing with me! Should I release right away.

lutzky commented 2 years ago

Oh, I figured I'd do the other test that does time.Sleep as clockwork as well, but I guess I can do that separately.

maruel commented 2 years ago

Oh yes please do it as a separate change. I'll fix the other repositories to do go get -t ./...