periph / conn

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

Add PulseIn function #18

Closed 1995parham closed 2 years ago

1995parham commented 2 years ago
codecov-commenter commented 2 years ago

Codecov Report

Merging #18 (19e8744) into main (c23f9e5) will decrease coverage by 0.2%. The diff coverage is 68.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main     #18     +/-   ##
=======================================
- Coverage   98.4%   98.2%   -0.2%     
=======================================
  Files         31      32      +1     
  Lines       3725    3750     +25     
=======================================
+ Hits        3666    3683     +17     
- Misses        56      62      +6     
- Partials       3       5      +2     
Impacted Files Coverage Δ
gpio/gpioutil/pulsein.go 68.0% <68.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 c23f9e5...19e8744. Read the comment docs.

1995parham commented 2 years ago

Thanks for your time and complete review, I will do the comments and also add the unit tests and let you know.

1995parham commented 2 years ago

I have resolved all comments and also add tests.

maruel commented 2 years ago

Please check the failure in macos-latest, probably a race condition under high workload.

1995parham commented 2 years ago

Sure, I will check. thanks for mentioning it.

1995parham commented 2 years ago

There is a complexity in the tests because there is a section in the In function in gpiotest which removes all the edges from the edges channel so I must feed it with a dummy variable and then continue.

https://github.com/periph/conn/blob/c23f9e55ee68df7d86b376cc553a661d5aa4f38b/gpio/gpiotest/gpiotest.go#L98

I hope I have fixed the tests by the last commit.

maruel commented 2 years ago

Your comment on the PWM issue reminded me that I'd like to start using context.Context more. This is generally a breaking change so it's slotted for v4 but here we could start using it right away. wdyt?

1995parham commented 2 years ago

Yes, I can consider the context for this function but again we have a timeout on WaitForEdge which makes it difficult I think. But if you agree I can start to switch the low-level pin functions to use context in separate PR.

maruel commented 2 years ago

Let's punt on the context thing. Please double check to make the test race condition free. Thanks!

1995parham commented 2 years ago

Sure, I will check the race conditions again, and if you agree I can create a PR after this for creating a context on gpio low-level functions.

1995parham commented 2 years ago

Sorry for taking so longr. I have pushed the last version which I think can pass the tests.

1995parham commented 2 years ago

I have fixed the test by implementing a new Pin for PulseIn tests. I think this hasn't race parts.

1995parham commented 2 years ago

@maruel sorry for bothering you, I have fixed the last issues.

maruel commented 2 years ago

Thanks!