From 77aa31257d883e9a27692e5701f59b5a1eb94c63 Mon Sep 17 00:00:00 2001 From: George Sexton Date: Thu, 21 Nov 2024 20:50:12 -0700 Subject: [PATCH 1/3] Improve registration to handle vagaries of how the Linux kernel handles GPIO Chips, particularly on the Pi 5. Improved handling of duplicate pin names. Code now prefixes the pin name with the chip name if it's duplicated. --- gpioioctl/basic_test.go | 25 ------------ gpioioctl/dummy.go | 1 - gpioioctl/gpio.go | 84 ++++++++++++++++++++++++++++++++++------- 3 files changed, 70 insertions(+), 40 deletions(-) diff --git a/gpioioctl/basic_test.go b/gpioioctl/basic_test.go index 875ca29..90585be 100644 --- a/gpioioctl/basic_test.go +++ b/gpioioctl/basic_test.go @@ -9,41 +9,16 @@ package gpioioctl import ( - "log" "testing" - "periph.io/x/conn/v3/gpio" "periph.io/x/conn/v3/gpio/gpioreg" ) var testLine *GPIOLine func init() { - var err error - if len(Chips) == 0 { makeDummyChip() - line := GPIOLine{ - number: 0, - name: "DummyGPIOLine", - consumer: "", - edge: gpio.NoEdge, - pull: gpio.PullNoChange, - direction: LineDirNotSet, - } - - chip := GPIOChip{name: "DummyGPIOChip", - path: "/dev/gpiochipdummy", - label: "Dummy GPIOChip for Testing Purposes", - lineCount: 1, - lines: []*GPIOLine{&line}, - } - Chips = append(Chips, &chip) - if err = gpioreg.Register(&line); err != nil { - nameStr := chip.Name() - lineStr := line.String() - log.Println("chip", nameStr, " gpioreg.Register(line) ", lineStr, " returned ", err) - } } } diff --git a/gpioioctl/dummy.go b/gpioioctl/dummy.go index 7f3c3ea..0b86a5f 100644 --- a/gpioioctl/dummy.go +++ b/gpioioctl/dummy.go @@ -36,7 +36,6 @@ func makeDummyChip() { lines: []*GPIOLine{&line}, } Chips = append(Chips, &chip) - Chips = append(Chips, &chip) if err := gpioreg.Register(&line); err != nil { nameStr := chip.Name() lineStr := line.String() diff --git a/gpioioctl/gpio.go b/gpioioctl/gpio.go index 3407e6f..101b74c 100644 --- a/gpioioctl/gpio.go +++ b/gpioioctl/gpio.go @@ -14,6 +14,7 @@ import ( "path" "path/filepath" "runtime" + "sort" "strings" "sync" "time" @@ -402,6 +403,9 @@ func newGPIOChip(path string) (*GPIOChip, error) { chip.name = strings.Trim(string(info.name[:]), "\x00") chip.label = strings.Trim(string(info.label[:]), "\x00") + if len(chip.label) == 0 { + chip.label = chip.name + } chip.lineCount = int(info.lines) var line_info gpio_v2_line_info for line := 0; line < int(info.lines); line++ { @@ -591,24 +595,76 @@ func (d *driverGPIO) After() []string { // // https://docs.kernel.org/userspace-api/gpio/chardev.html func (d *driverGPIO) Init() (bool, error) { - if runtime.GOOS == "linux" { - items, err := filepath.Glob("/dev/gpiochip*") - if err != nil { - return true, err - } - if len(items) == 0 { - return false, errors.New("no GPIO chips found") + Chips = make([]*GPIOChip, 0) + if runtime.GOOS != "linux" { + return true, nil + } + items, err := filepath.Glob("/dev/gpiochip*") + if err != nil { + return true, fmt.Errorf("gpioioctl: %w", err) + } + if len(items) == 0 { + return false, errors.New("no GPIO chips found") + } + // First, get all of the chips on the system. + chips := make([]*GPIOChip, 0) + for _, item := range items { + chip, err := newGPIOChip(item) + if err == nil { + chips = append(chips, chip) + } else { + log.Println("gpioioctl.driverGPIO.Init() Error", err) } - Chips = make([]*GPIOChip, 0) - for _, item := range items { - chip, err := newGPIOChip(item) - if err != nil { - log.Println("gpioioctl.driverGPIO.Init() Error", err) - return false, err + } + // Now, sort the chips so that those labeled with pinctrl- ( a Pi kernel standard) + // come first. Otherwise, sort them by label. This _should_ protect us from any + // random changes in chip naming/ordering. + sort.Slice(chips, func(i, j int) bool { + I := chips[i] + J := chips[j] + if I.Label()[:8] == "pinctrl-" { + if J.Label()[:8] == "pinctrl-" { + return I.Label() < J.Label() + } else { + return true } + } else if J.Label()[:8] == "pinctrl-" { + return false + } else { + return I.Label() < J.Label() + } + }) + + mName := make(map[string]bool, 0) + // Get a list of already registered GPIO Line names. + registeredPins := make(map[string]bool) + for _, pin := range gpioreg.All() { + registeredPins[pin.Name()] = true + } + + // Now, iterate over the chips we found and add their lines to conn/gpio/gpioreg + for _, chip := range chips { + // On a pi, gpiochip0 is also symlinked to gpiochip4, checking the map + // ensures we don't duplicate the chip. + if _, found := mName[chip.Name()]; !found { Chips = append(Chips, chip) + mName[chip.Name()] = true + // Now, iterate over the lines on this chip. for _, line := range chip.lines { + // If the line has some sort of reasonable name... if len(line.name) > 0 && line.name != "_" && line.name != "-" { + // See if the name is already registered. On the Pi5, there are at + // least two chips that export "2712_WAKE" as the line name. + if _, ok := registeredPins[line.Name()]; ok { + // This is a duplicate name. Prefix the line name with the + // chip name. + line.name = chip.Name() + "-" + line.Name() + if _, found := registeredPins[line.Name()]; found { + // It's still not unique. Skip it. + continue + } + } + registeredPins[line.Name()] = true if err = gpioreg.Register(line); err != nil { log.Println("chip", chip.Name(), " gpioreg.Register(line) ", line, " returned ", err) } @@ -616,7 +672,7 @@ func (d *driverGPIO) Init() (bool, error) { } } } - return true, nil + return len(Chips) > 0, nil } var drvGPIO driverGPIO From 63d802cffcc2aed79e06dc5218927ba4ebb5d4ab Mon Sep 17 00:00:00 2001 From: George Sexton Date: Thu, 21 Nov 2024 21:01:45 -0700 Subject: [PATCH 2/3] Fix lint issue with variable shadowing. --- gpioioctl/gpio.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/gpioioctl/gpio.go b/gpioioctl/gpio.go index 101b74c..1ebbe20 100644 --- a/gpioioctl/gpio.go +++ b/gpioioctl/gpio.go @@ -608,8 +608,9 @@ func (d *driverGPIO) Init() (bool, error) { } // First, get all of the chips on the system. chips := make([]*GPIOChip, 0) + var chip *GPIOChip for _, item := range items { - chip, err := newGPIOChip(item) + chip, err = newGPIOChip(item) if err == nil { chips = append(chips, chip) } else { From 9d985ecf3318a07d206d84d5355c2cb59d9d431a Mon Sep 17 00:00:00 2001 From: George Sexton Date: Sat, 23 Nov 2024 08:57:59 -0700 Subject: [PATCH 3/3] Fix possible out of bounds error. --- gpioioctl/gpio.go | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/gpioioctl/gpio.go b/gpioioctl/gpio.go index 1ebbe20..fd2fb0d 100644 --- a/gpioioctl/gpio.go +++ b/gpioioctl/gpio.go @@ -595,7 +595,6 @@ func (d *driverGPIO) After() []string { // // https://docs.kernel.org/userspace-api/gpio/chardev.html func (d *driverGPIO) Init() (bool, error) { - Chips = make([]*GPIOChip, 0) if runtime.GOOS != "linux" { return true, nil } @@ -607,7 +606,7 @@ func (d *driverGPIO) Init() (bool, error) { return false, errors.New("no GPIO chips found") } // First, get all of the chips on the system. - chips := make([]*GPIOChip, 0) + var chips []*GPIOChip var chip *GPIOChip for _, item := range items { chip, err = newGPIOChip(item) @@ -623,24 +622,22 @@ func (d *driverGPIO) Init() (bool, error) { sort.Slice(chips, func(i, j int) bool { I := chips[i] J := chips[j] - if I.Label()[:8] == "pinctrl-" { - if J.Label()[:8] == "pinctrl-" { + if strings.HasPrefix(I.Label(), "pinctrl-") { + if strings.HasPrefix(J.Label(), "pinctrl-") { return I.Label() < J.Label() - } else { - return true } - } else if J.Label()[:8] == "pinctrl-" { + return true + } else if strings.HasPrefix(J.Label(), "pinctrl-") { return false - } else { - return I.Label() < J.Label() } + return I.Label() < J.Label() }) - mName := make(map[string]bool, 0) + mName := make(map[string]struct{}) // Get a list of already registered GPIO Line names. - registeredPins := make(map[string]bool) + registeredPins := make(map[string]struct{}) for _, pin := range gpioreg.All() { - registeredPins[pin.Name()] = true + registeredPins[pin.Name()] = struct{}{} } // Now, iterate over the chips we found and add their lines to conn/gpio/gpioreg @@ -649,7 +646,7 @@ func (d *driverGPIO) Init() (bool, error) { // ensures we don't duplicate the chip. if _, found := mName[chip.Name()]; !found { Chips = append(Chips, chip) - mName[chip.Name()] = true + mName[chip.Name()] = struct{}{} // Now, iterate over the lines on this chip. for _, line := range chip.lines { // If the line has some sort of reasonable name... @@ -665,7 +662,7 @@ func (d *driverGPIO) Init() (bool, error) { continue } } - registeredPins[line.Name()] = true + registeredPins[line.Name()] = struct{}{} if err = gpioreg.Register(line); err != nil { log.Println("chip", chip.Name(), " gpioreg.Register(line) ", line, " returned ", err) }