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

gpioioctl:Implement ioctl access to Linux GPIO chips/lines. #59

Merged
merged 32 commits into from
Sep 18, 2024

Conversation

gsexton
Copy link
Contributor

@gsexton gsexton commented Aug 28, 2024

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:

    a functional name for the consumer of this GPIO line as set by whatever is using it,
    will be empty if there is no current user but may also be empty if the consumer doesn’t
    set this up

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:

Do NOT abuse userspace APIs to control hardware that has proper kernel drivers.
There may already be a driver for your use case, and an existing kernel driver
is sure to provide a superior solution to bitbashing from userspace.

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:

pwm-gpio: drivers/pwm/pwm-gpio.c is used to toggle a GPIO with a high resolution
timer producing a PWM waveform on the GPIO line, as well as Linux high resolution
timers can do.

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.

gpioioctl/Testing.md Outdated Show resolved Hide resolved
@maruel
Copy link
Member

maruel commented Aug 28, 2024

I haven't looked at the code yet but answering a few questions for sake of speed:

  • PinFunc is mostly relevant for MCU provided hardware pins, so it's okay to ignore for now.
  • PWM: sounds good. It's okay to just return an error.
  • Line vs Pin and multi-pins: it's something I wanted to create a conn/v4 for but lost steam sadly as the refactor became big; I want to make all stateful functions like PWM() to accept a context.Context.

Copy link
Member

@maruel maruel left a 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.

gpioioctl/basic_test.go Outdated Show resolved Hide resolved
gpioioctl/basic_test.go Outdated Show resolved Hide resolved
gpioioctl/basic_test.go Outdated Show resolved Hide resolved
gpioioctl/basic_test.go Outdated Show resolved Hide resolved
gpioioctl/example_test.go Show resolved Hide resolved
gpioioctl/ioctl.go Outdated Show resolved Hide resolved
gpioioctl/gpio.go Outdated Show resolved Hide resolved
gpioioctl/gpio.go Outdated Show resolved Hide resolved
gpioioctl/gpio.go Show resolved Hide resolved
gpioioctl/gpio.go Outdated Show resolved Hide resolved
Add dependency to sysfs/gpio.go init
cleanup submission per reviewer comments.
gpioioctl/basic_test.go Outdated Show resolved Hide resolved
gpioioctl/basic_test.go Outdated Show resolved Hide resolved
gpioioctl/basic_test.go Outdated Show resolved Hide resolved
Copy link
Member

@maruel maruel left a 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.

gpioioctl/gpio.go Outdated Show resolved Hide resolved
gpioioctl/gpio.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 12.87879% with 460 lines in your changes missing coverage. Please review.

Project coverage is 26.7%. Comparing base (50d4ed0) to head (9204ccc).

Files with missing lines Patch % Lines
gpioioctl/gpio.go 21.8% 238 Missing and 2 partials ⚠️
gpioioctl/lineset.go 0.0% 173 Missing ⚠️
gpioioctl/ioctl.go 0.0% 47 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@maruel
Copy link
Member

maruel commented Sep 11, 2024

I'll merge once the checks are green.

@gsexton
Copy link
Contributor Author

gsexton commented Sep 12, 2024

@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.

host_linux.go Outdated Show resolved Hide resolved
@gsexton
Copy link
Contributor Author

gsexton commented Sep 18, 2024

@maruel Were you waiting on me for something more? I think I've got everything you asked for completed.

@maruel
Copy link
Member

maruel commented Sep 18, 2024

Yes sorry I just needed to silence gosec, done in 1e513a0.

@maruel maruel merged commit b69b28c into periph:main Sep 18, 2024
5 of 8 checks passed
@maruel
Copy link
Member

maruel commented Sep 18, 2024

Thanks a lot! It's really appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants