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

Create hcsr04_driver.go #872

Closed
wants to merge 6 commits into from
Closed

Conversation

Juan1ll0
Copy link

Trying to translate HCSR04 from another lib that uses go-rpio.

Trting to translate HCSR04 from another lib that uses go-rpio.
@Juan1ll0 Juan1ll0 marked this pull request as draft October 21, 2022 17:30
@gen2thomas
Copy link
Collaborator

related to #865

Comment on lines 116 to 123
for {
// readedValue, _ := pin.DigitalRead()
readedValue := 1
// fmt.Println("Lectura: ", readedValue, state, *quit)
if readedValue != state && !*quit {
break
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this loop waits for "an ongoing measurement is finished", so it will possibly not work, if the pin.DigitalRead() is substituted by a constant of "1" (especially for the first call)

Comment on lines 92 to 95
readedValue, _ := hcsr04.echoPin.DigitalRead()
if readedValue == 1 {
return 0, errors.New("already receiving echo")
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this part looks "duplicated" to me (because the go - routine already waits for pin != 1), please try without them to save some time

Copy link
Collaborator

@gen2thomas gen2thomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I would remove all monitoring stuff first. This simplifies further reviews and also gobot normally use other methods for continuous reading.

Start working. I thought it needs to be more stable. Good reads.
@Juan1ll0
Copy link
Author

Juan1ll0 commented Oct 29, 2022

Thank you @gen2thomas I make all suggested changes and change a condition and starts working. I think it needs a review for clean code and write some tests, but it start working. I upload changes to my branch If you want to review.

@gen2thomas
Copy link
Collaborator

@Juan1ll0 I have added some new comments.

Move private functions to end of file
@Juan1ll0
Copy link
Author

Juan1ll0 commented Nov 6, 2022

@Juan1ll0 I have added some new comments.

I make all your suggested changes.

@gen2thomas
Copy link
Collaborator

gen2thomas commented Nov 6, 2022

@Juan1ll0 thanks - I will have a look in the next days, meanwhile:

  • the appveyor build is not working, maybe you need to rebase your branch?
  • you can check your code format with "gofmt -l "
  • in addition you should check your code with "golint"
  • try to add some unit tests by your own - I will support you, if a basic set is there
  • please also add an example

don't care on errors in other files, fix your issues is sufficient

type HCSR04 struct {
name string
connection gobot.Adaptor
triggerPin *gpio.DirectPinDriver
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the file is already part of package "gpio", all this "gpio." needs to be removed. Otherwise an import cycle failure occurs.

@gen2thomas gen2thomas deleted the branch hybridgroup:dev May 15, 2023 16:25
@gen2thomas gen2thomas closed this May 15, 2023
@gen2thomas
Copy link
Collaborator

accidentally closed - now reopen

@gen2thomas
Copy link
Collaborator

closed in favor of #1012 , thanks @Juan1ll0

@gen2thomas gen2thomas closed this Oct 27, 2023
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