search

brian-armstrong / gpio

Go library to do GPIO on systems with /sys/class/gpio (sysfs)
BSD 3-Clause "New" or "Revised" License
140 stars 50 forks source link

Unexport GPIO on Pin closing #8

Closed andreacioni closed 5 years ago

andreacioni commented 6 years ago

Hi @brian-armstrong,

I made this little PR to fix the gpio.Close() method, ensuring Pin is unexported when we close it.

brian-armstrong commented 6 years ago

Thanks for the PR.

Although I agree that this is cleaner, I'm not sure that I want this to happen in Close(). There's two issues I could see here. One is that other programs may be using the pin still, and I'm not sure how unexport interacts with open handles on the file. The other issue is just sort of practicality. It can take a long, seemingly undeterministic amount of time to export pins. It's long enough that sometimes opening a pin twice fails the first time but succeeds the second, and it's just because the pin was still exporting. Once it's exported, I'd generally prefer to leave it.

I think I'd rather see this as a function we export to the user so that they can opt in to unexporting. That's a little hacky but from a practical standpoint I think it'd be better.

andreacioni commented 6 years ago

One is that other programs may be using the pin still, and I'm not sure how unexport interacts with open handles on the file.

This shouldn't occur. Use of one GPIO should be handled by only one process at time to avoid confusion and unexpected behaviors. I think we could assume that this situation will not happen in well-designed environment.

It can take a long, seemingly undeterministic amount of time to export pins. It's long enough that sometimes opening a pin twice fails the first time but succeeds the second

That is true. About this point, I noticed also a timing issue in my tests related to time.Sleep(10 * time.Millisecond) inside NewOutput (but I saw it also in NewInput). In particular, sometimes, setDirection fails because the corresponding file is not created yet. I'll try to take a closer look to these issues because them could be related.

I think I'd rather see this as a function we export to the user so that they can opt in to unexporting. That's a little hacky but from a practical standpoint I think it'd be better.

The io.go is a simple and very good interface to access GPIO. Adding an Unexport method probably will create confusion (also because user doesn't have the corresponding Export). I think the best place for unexporting logic is Close. Let me find a better solution :wink:

brian-armstrong commented 6 years ago

I disagree on the first point - I think it's actually reasonable for two programs to read from the same pin simultaneously, since that has no harmful side effects.

Maybe it makes sense to just offer a .Cleanup() which will unexport everything exported so far. I suspect most users do not care about this, and a good godoc should make it clear why they might.

andreacioni commented 6 years ago

Ok, we have two different point of view. Not a problem :smile: I'll add .Cleanup() to io.go.

chmorgan commented 5 years ago

Does this look like it will be merged?

brian-armstrong commented 5 years ago

My bad :)

brian-armstrong commented 5 years ago

Thanks for the PR!