-
Notifications
You must be signed in to change notification settings - Fork 33
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
gpioioctl:Implement ioctl access to Linux GPIO chips/lines. #59
Conversation
I haven't looked at the code yet but answering a few questions for sake of speed:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. Don't be afraid by the number of comments, it's a chunk change. Thanks again.
Add dependency to sysfs/gpio.go init cleanup submission per reviewer comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notes about JSON.
I changed the GitHub settings so the actions should start immediately upon push, so you don't need to wait for me to click the Approve button.
… to solve another problem.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #59 +/- ##
=======================================
- Coverage 27.4% 26.7% -0.7%
=======================================
Files 89 92 +3
Lines 11011 11538 +527
=======================================
+ Hits 3014 3081 +67
- Misses 7864 8322 +458
- Partials 133 135 +2 ☔ View full report in Codecov by Sentry. |
I'll merge once the checks are green. |
@maruel So i'm pretty sure the reason that netlink was not loading in host init() is because the pipeline blocked infinitely trying receive the message from the netlink socket. I got creative and added a 2s timeout to the onewire.Init(). That seems to stop the pipeline from timing out. It now passes the Linux pipeline build. I tested it on a pi with a couple of DS18B20 units attached and it worked as expected. I've fixed all of the lint issues in gpioioctl, sysfs, and netlink, which is code that I've touched. The rest is all gosec G115 possible overflow errors. I don't think they're false positives. I'd recommend disabling that check in the pipeline. |
@maruel Were you waiting on me for something more? I think I've got everything you asked for completed. |
Yes sorry I just needed to silence gosec, done in 1e513a0. |
Thanks a lot! It's really appreciated. |
Fixes host issue #39
First, let me start by saying I tend to work in lots of languages, and my fluency in golang is not really high. If you see non-idiomatic things you can't live with, just let me know. Similarly, I know you're the maintainer, and it's your call in implementations.
This is a pretty big add. If you feel like a call would be beneficial, I can do a voice call.
Terminology
I'm using the term "Line", and not "Pin". Line does implement conn/v3/gpio.PinIn, PinOut, and PinIO. This is kind of a difference. Line is an attribute of the chip, while pin is an attribute of the board. It seems to me that we want the code to work across boards, regardless of how a board physically presents a GPIO chipset's line. So a pin is a physical element that
the chip's line is connected to. I can see potentially a GPIO line that's not exposed as a physical pin on a board. As an aside, the ioctl()s use the Line terminology. However, if you think that's unnecessarily confusing, I can change it.
Tests
Refer to Tests.md
Interfaces
consumer
In the ioctl(), there's a "consumer" field. The kernel docs say:
I'm writing the program_name@pid into this field. I can see other programs use it too. For example, I see PPS bound to a GPIO Chip on my Pi. You might consider adding
func Consumer() string
to the gpio.Pin interface for the next revision.
PinFunc
I really don't understand the PinFunc interface, so I didn't implement it. I looked it over, and from what I could tell, I didn't like the idea, but perhaps I didn't understand it. If you can give me a better idea of the intent, I'll give it a go.
PWM
PWM isn't really part of the GPIO ioctl User Space API. The kernel docs say:
I've stubbed in a PWM method to satisfy PinOut, but it should probably be deprecated from the PinOut interface. Here's the what the doc says:
MultiPin IO
Multi-Pin IO is handled by the new construct LineSet. The ioctl() interface makes this pretty straight forward. Single line input/output are identical to multi-line. Other than configuration, no clever tricks are required. You just call Read() and it
returns the pin values as a uint64 bitmap. Essentially, a LineSet provides methods for reading or writing all lines in one go. LineSetLine provides convenience methods to allow writing individual pins and implements PinIn, PinOut, and PinIO. Additionally, functions of pins can be different within the set. Some can be input, and others output.
A common use case for a LineSet might be doing a HD44780 LCD display which has 4 or 8 lines + a strobe line, and a backlight line.