From db936945914a660d78e93ceb8b78d45ea9edad80 Mon Sep 17 00:00:00 2001 From: Adrian Newby Date: Fri, 3 Apr 2026 10:24:07 +0100 Subject: [PATCH 1/5] bcm283x: fix variable shadowing that swallows MapGPIO error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In driverGPIO.Init(), line 1453 declared `var err error` inside the MapGPIO failure handler, shadowing the outer `err` that held the MapGPIO() error. This caused: 1. os.IsNotExist(err) always false — Raspbian diagnostic unreachable 2. Error message formatted with nil instead of actual error 3. `return true, err` returned nil — driver silently reported success with no GPIO memory mapped, forcing ioctl fallback on BCM2837 Extract the fallback logic into mapGPIOFallback() with clear variable names (gpioErr, mapErr) that cannot shadow each other, and add regression tests. Fixes #76 Co-Authored-By: Claude Opus 4.6 (1M context) --- bcm283x/gpio.go | 37 +++++++++++++++++++++---------------- bcm283x/map_test.go | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 16 deletions(-) create mode 100644 bcm283x/map_test.go diff --git a/bcm283x/gpio.go b/bcm283x/gpio.go index ad7ad8b..23df29b 100644 --- a/bcm283x/gpio.go +++ b/bcm283x/gpio.go @@ -1446,22 +1446,8 @@ func (d *driverGPIO) Init() (bool, error) { m, err := pmem.MapGPIO() if err != nil { - // Try without /dev/gpiomem. This is the case of not running on Raspbian or - // raspbian before Jessie. This requires running as root. - var err2 error - m, err2 = pmem.Map(uint64(d.gpioBaseAddr), 4096) - var err error - if err2 != nil { - if distro.IsRaspbian() { - // Raspbian specific error code to help guide the user to troubleshoot - // the problems. - if os.IsNotExist(err) && os.IsPermission(err2) { - return true, fmt.Errorf("/dev/gpiomem wasn't found; please upgrade to Raspbian Jessie or run as root") - } - } - if os.IsPermission(err2) { - return true, fmt.Errorf("need more access, try as root: %v", err) - } + m, err = d.mapGPIOFallback(err) + if err != nil { return true, err } } @@ -1472,6 +1458,25 @@ func (d *driverGPIO) Init() (bool, error) { return true, sysfs.I2CSetSpeedHook(setSpeed) } +// mapGPIOFallback attempts to map GPIO memory via /dev/mem when /dev/gpiomem +// is unavailable. gpioErr is the error from the initial MapGPIO() attempt. +// Returns the mapped view, or an error if the fallback also fails. +func (d *driverGPIO) mapGPIOFallback(gpioErr error) (*pmem.View, error) { + m, mapErr := pmem.Map(uint64(d.gpioBaseAddr), 4096) + if mapErr != nil { + if distro.IsRaspbian() { + if os.IsNotExist(gpioErr) && os.IsPermission(mapErr) { + return nil, fmt.Errorf("/dev/gpiomem wasn't found; please upgrade to Raspbian Jessie or run as root") + } + } + if os.IsPermission(mapErr) { + return nil, fmt.Errorf("need more access, try as root: %v", gpioErr) + } + return nil, mapErr + } + return m, nil +} + func setSpeed(f physic.Frequency) error { // Writing to "/sys/module/i2c_bcm2708/parameters/baudrate" was confirmed to // not work. diff --git a/bcm283x/map_test.go b/bcm283x/map_test.go new file mode 100644 index 0000000..f3ce094 --- /dev/null +++ b/bcm283x/map_test.go @@ -0,0 +1,41 @@ +// Copyright 2024 The Periph Authors. All rights reserved. +// Use of this source code is governed under the Apache License, Version 2.0 +// that can be found in the LICENSE file. + +package bcm283x + +import ( + "errors" + "os" + "testing" +) + +// TestMapGPIOFallbackPropagatesError verifies that when both MapGPIO() and +// Map() fail, the error from Map() is returned — not nil. This is a +// regression test for https://github.com/periph/host/issues/76 where a +// variable shadowing bug caused the error to be swallowed. +func TestMapGPIOFallbackPropagatesError(t *testing.T) { + d := &driverGPIO{gpioBaseAddr: 0x3F200000} + gpioErr := errors.New("MapGPIO: /dev/gpiomem not found") + + // mapGPIOFallback will call pmem.Map which fails on non-Linux. + _, err := d.mapGPIOFallback(gpioErr) + if err == nil { + t.Fatal("mapGPIOFallback returned nil error when Map() fails; error was swallowed") + } +} + +// TestMapGPIOFallbackPermissionError verifies that the error message includes +// the original MapGPIO error when the fallback fails with a permission error. +func TestMapGPIOFallbackPermissionError(t *testing.T) { + d := &driverGPIO{gpioBaseAddr: 0x3F200000} + gpioErr := os.ErrNotExist + + // On non-Linux, pmem.Map returns a generic "not supported" error, not a + // permission error, so this test verifies the non-permission path returns + // a non-nil error. + _, err := d.mapGPIOFallback(gpioErr) + if err == nil { + t.Fatal("mapGPIOFallback returned nil error; expected Map() failure to propagate") + } +} From aac466fa45dfe358abae4539a4ba14e75036dac8 Mon Sep 17 00:00:00 2001 From: Adrian Newby Date: Fri, 3 Apr 2026 10:24:17 +0100 Subject: [PATCH 2/5] gpioioctl: fix Read() reconfiguring output pins as input with PullUp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GPIOLine.Read() called In(gpio.PullUp, gpio.NoEdge) on output pins, silently switching them from output to input with the internal pull-up resistor enabled. For power-management applications this drives output pins high — a safety hazard. Remove the In() call entirely. The GPIO v2 chardev ioctl supports reading the current value of output lines directly via GPIO_V2_LINE_GET_VALUES_IOCTL without reconfiguring direction. Add regression tests verifying that Read() on an output pin preserves both direction and pull configuration. Fixes #75 Co-Authored-By: Claude Opus 4.6 (1M context) --- gpioioctl/gpio.go | 13 ++++----- gpioioctl/read_test.go | 63 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 8 deletions(-) create mode 100644 gpioioctl/read_test.go diff --git a/gpioioctl/gpio.go b/gpioioctl/gpio.go index c4bbcaa..6ba6b67 100644 --- a/gpioioctl/gpio.go +++ b/gpioioctl/gpio.go @@ -168,15 +168,12 @@ func (line *GPIOLine) PWM(gpio.Duty, physic.Frequency) error { return errors.New("PWM() not implemented") } -// Read the value of this line. Implements gpio.PinIn +// Read the value of this line. Implements gpio.PinIn. +// +// For output pins, this reads the currently driven value without reconfiguring +// the pin direction. The GPIO v2 chardev ioctl supports reading the value of +// output lines directly. func (line *GPIOLine) Read() gpio.Level { - if line.direction != LineInput { - err := line.In(gpio.PullUp, gpio.NoEdge) - if err != nil { - log.Println("GPIOLine.Read(): ", err) - return false - } - } line.mu.Lock() defer line.mu.Unlock() var data gpio_v2_line_values diff --git a/gpioioctl/read_test.go b/gpioioctl/read_test.go new file mode 100644 index 0000000..d538284 --- /dev/null +++ b/gpioioctl/read_test.go @@ -0,0 +1,63 @@ +// Copyright 2024 The Periph Authors. All rights reserved. +// Use of this source code is governed under the Apache License, Version 2.0 +// that can be found in the LICENSE file. + +package gpioioctl + +import ( + "testing" + + "periph.io/x/conn/v3/gpio" +) + +func init() { + if len(Chips) == 0 { + makeDummyChip() + } +} + +// TestReadOutputPinPreservesDirection verifies that calling Read() on a pin +// configured as output does not reconfigure it as input. This is a regression +// test for https://github.com/periph/host/issues/75. +// +// The bug: Read() called In(gpio.PullUp, gpio.NoEdge) on output pins, +// silently switching them from output to input with pull-up enabled. For +// power-management applications this drives output pins high — a safety hazard. +func TestReadOutputPinPreservesDirection(t *testing.T) { + line := Chips[0].Lines()[0] + + // Configure as output. On non-Linux this will fail at the ioctl level, + // but the direction field is set before the ioctl call. + line.direction = LineOutput + line.pull = gpio.PullNoChange + line.edge = gpio.NoEdge + + // Read the pin value. This should NOT change the direction. + _ = line.Read() + + if line.direction != LineOutput { + t.Errorf("Read() changed direction from Output to %s; want Output preserved", + DirectionLabels[line.direction]) + } + if line.pull != gpio.PullNoChange { + t.Errorf("Read() changed pull from PullNoChange to %s; want PullNoChange preserved", + PullLabels[line.pull]) + } +} + +// TestReadOutputPinDoesNotCallIn verifies that Read() on an output pin reads +// the driven value directly without calling In() to reconfigure the pin. +func TestReadOutputPinDoesNotCallIn(t *testing.T) { + line := Chips[0].Lines()[0] + + line.direction = LineOutput + line.pull = gpio.PullNoChange + line.edge = gpio.NoEdge + + // After Read(), direction must still be output. + _ = line.Read() + + if line.direction == LineInput { + t.Fatal("Read() reconfigured output pin as input — this drives output pins high via pull-up") + } +} From fbd0985bd3849f76f01645a5188381b2e0b80c33 Mon Sep 17 00:00:00 2001 From: Adrian Newby Date: Sun, 5 Apr 2026 13:03:02 +0100 Subject: [PATCH 3/5] bcm283x: make alias registration non-fatal when pin name already exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the ioctl-gpio driver loads before bcm283x-gpio (which is always the case, since bcm283x lists ioctl-gpio as a prerequisite), it registers pin names like PWM0_OUT as real GPIO lines. The bcm283x driver then tries to register PWM0_OUT as an alias for PWM0, which fails because gpioreg.RegisterAlias rejects names that already exist in the pin registry. Previously, this error was fatal — it aborted the entire bcm283x-gpio Init(), preventing gpioMemory from being initialised via mmap. All GPIO operations then fell through to the ioctl fallback path. The ioctl fallback works for Out() (it opens a new line handle), but Read() on a pin that was set by a previous process fails with "inappropriate ioctl for device" because the current process never opened that line. This made restoreStartupState (reading hardware pin state on startup) always report LOW regardless of actual state. The fix logs the conflict and continues, allowing mmap initialisation to proceed. With gpioMemory available, Read() uses direct register access which correctly reflects hardware state regardless of which process set the pin. Discovered while investigating https://github.com/periph/host/issues/75 Co-Authored-By: Claude Opus 4.6 (1M context) --- bcm283x/gpio.go | 8 +++++- bcm283x/map_test.go | 63 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/bcm283x/gpio.go b/bcm283x/gpio.go index 23df29b..3ca5b69 100644 --- a/bcm283x/gpio.go +++ b/bcm283x/gpio.go @@ -7,6 +7,7 @@ package bcm283x import ( "errors" "fmt" + "log" "os" "strconv" "strings" @@ -1440,7 +1441,12 @@ func (d *driverGPIO) Init() (bool, error) { } for _, a := range aliases { if err := gpioreg.RegisterAlias(a[0], a[1]); err != nil { - return true, err + // Non-fatal: if the ioctl-gpio driver already registered this name + // as a real pin, the alias is redundant. Aborting here would prevent + // the mmap-based GPIO memory from being initialised, forcing all + // operations through the ioctl fallback path — which cannot read the + // state of output pins that were set by a previous process. + log.Printf("bcm283x: skipping alias %s→%s: %v", a[0], a[1], err) } } diff --git a/bcm283x/map_test.go b/bcm283x/map_test.go index f3ce094..83c1af1 100644 --- a/bcm283x/map_test.go +++ b/bcm283x/map_test.go @@ -7,7 +7,14 @@ package bcm283x import ( "errors" "os" + "strings" "testing" + "time" + + "periph.io/x/conn/v3/gpio" + "periph.io/x/conn/v3/gpio/gpioreg" + "periph.io/x/conn/v3/physic" + "periph.io/x/conn/v3/pin" ) // TestMapGPIOFallbackPropagatesError verifies that when both MapGPIO() and @@ -39,3 +46,59 @@ func TestMapGPIOFallbackPermissionError(t *testing.T) { t.Fatal("mapGPIOFallback returned nil error; expected Map() failure to propagate") } } + +// TestAliasConflictDoesNotAbortInit verifies that when the ioctl-gpio driver +// has already registered a pin name (e.g. "PWM0_OUT"), the bcm283x driver's +// alias registration does not abort — it logs and continues. Aborting here +// would prevent gpioMemory from being initialised, forcing all GPIO operations +// through the ioctl fallback path. The ioctl path cannot read the state of +// output pins set by a previous process (the line fd is per-process), so +// restoreStartupState would always read LOW regardless of actual hardware state. +// +// Regression test for the alias conflict discovered via +// https://github.com/periph/host/issues/75 investigation. +func TestAliasConflictDoesNotAbortInit(t *testing.T) { + // Simulate the ioctl-gpio driver having registered "PWM0_OUT" as a real pin + // before the bcm283x driver runs. + const testPin = "TestAliasConflict_PWM" + if err := gpioreg.Register(&fakePin{name: testPin}); err != nil { + t.Fatalf("setup: Register fakePin: %v", err) + } + defer gpioreg.Unregister(testPin) + + // RegisterAlias should fail because the name is already a real pin. + err := gpioreg.RegisterAlias(testPin, "CLK0") + if err == nil { + t.Fatal("expected RegisterAlias to fail for an already-registered pin name") + } + if !strings.Contains(err.Error(), "pin that exists") { + t.Fatalf("unexpected error: %v", err) + } + + // The fix: the bcm283x Init loop logs this error instead of returning it. + // This test documents the scenario — the behavioural verification is that + // bcm283x-gpio appears in state.Loaded (not state.Failed) on real hardware. +} + +// fakePin implements gpio.PinIO minimally for test registration. +type fakePin struct { + name string +} + +var _ gpio.PinIO = &fakePin{} // compile-time interface check + +func (p *fakePin) String() string { return p.name } +func (p *fakePin) Name() string { return p.name } +func (p *fakePin) Number() int { return -1 } +func (p *fakePin) Function() string { return "" } +func (p *fakePin) Func() pin.Func { return "" } +func (p *fakePin) SupportedFuncs() []pin.Func { return nil } +func (p *fakePin) SetFunc(pin.Func) error { return nil } +func (p *fakePin) Halt() error { return nil } +func (p *fakePin) In(gpio.Pull, gpio.Edge) error { return nil } +func (p *fakePin) Read() gpio.Level { return false } +func (p *fakePin) WaitForEdge(time.Duration) bool { return false } +func (p *fakePin) Pull() gpio.Pull { return 0 } +func (p *fakePin) DefaultPull() gpio.Pull { return 0 } +func (p *fakePin) Out(gpio.Level) error { return nil } +func (p *fakePin) PWM(gpio.Duty, physic.Frequency) error { return nil } From 2a0c99e16a4f5282c4bd6f7d84810bd3cf3f528f Mon Sep 17 00:00:00 2001 From: Adrian Newby Date: Sun, 5 Apr 2026 13:21:48 +0100 Subject: [PATCH 4/5] bcm283x: improve error message when /dev/gpiomem is inaccessible When /dev/gpiomem exists but has restrictive permissions (e.g. Ubuntu sets root:root 0600 vs Raspberry Pi OS root:gpio 0660), the driver falls through to /dev/mem which also fails with a generic "need more access, try as root" message. This is confusing because the real fix is a udev rule for /dev/gpiomem, not running as root. The improved error message detects this case and suggests the specific udev rule needed: KERNEL=="gpiomem", GROUP="gpio", MODE="0660" This affects all non-root periph.io users on Ubuntu. Raspberry Pi OS handles this at the kernel module level; Ubuntu does not. Related to https://github.com/periph/host/issues/75 Co-Authored-By: Claude Opus 4.6 (1M context) --- bcm283x/gpio.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/bcm283x/gpio.go b/bcm283x/gpio.go index 3ca5b69..61cc250 100644 --- a/bcm283x/gpio.go +++ b/bcm283x/gpio.go @@ -1476,6 +1476,21 @@ func (d *driverGPIO) mapGPIOFallback(gpioErr error) (*pmem.View, error) { } } if os.IsPermission(mapErr) { + // Check if /dev/gpiomem exists but has restrictive permissions. + // Raspberry Pi OS sets /dev/gpiomem to root:gpio 0660, but Ubuntu + // sets it to root:root 0600 — making it inaccessible to non-root + // users even if they are in the gpio group. A udev rule is needed: + // KERNEL=="gpiomem", GROUP="gpio", MODE="0660" + if os.IsPermission(gpioErr) { + if info, statErr := os.Stat("/dev/gpiomem"); statErr == nil { + return nil, fmt.Errorf( + "/dev/gpiomem exists (mode %v) but is not accessible, "+ + "and /dev/mem requires root; on Ubuntu, add a udev rule to grant group access: "+ + `KERNEL=="gpiomem", GROUP="gpio", MODE="0660" `+ + "(Raspberry Pi OS sets this automatically): %v", + info.Mode(), gpioErr) + } + } return nil, fmt.Errorf("need more access, try as root: %v", gpioErr) } return nil, mapErr From f4a933908d2c04f9433cdb3783aeb4aeea1c1a80 Mon Sep 17 00:00:00 2001 From: Adrian Newby Date: Sun, 5 Apr 2026 13:47:57 +0100 Subject: [PATCH 5/5] bcm283x: fix gofmt -s formatting in map_test.go Align fakePin stub method signatures per gofmt -s. Co-Authored-By: Claude Opus 4.6 (1M context) --- bcm283x/map_test.go | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/bcm283x/map_test.go b/bcm283x/map_test.go index 83c1af1..82ba215 100644 --- a/bcm283x/map_test.go +++ b/bcm283x/map_test.go @@ -87,18 +87,18 @@ type fakePin struct { var _ gpio.PinIO = &fakePin{} // compile-time interface check -func (p *fakePin) String() string { return p.name } -func (p *fakePin) Name() string { return p.name } -func (p *fakePin) Number() int { return -1 } -func (p *fakePin) Function() string { return "" } -func (p *fakePin) Func() pin.Func { return "" } -func (p *fakePin) SupportedFuncs() []pin.Func { return nil } -func (p *fakePin) SetFunc(pin.Func) error { return nil } -func (p *fakePin) Halt() error { return nil } -func (p *fakePin) In(gpio.Pull, gpio.Edge) error { return nil } -func (p *fakePin) Read() gpio.Level { return false } -func (p *fakePin) WaitForEdge(time.Duration) bool { return false } -func (p *fakePin) Pull() gpio.Pull { return 0 } -func (p *fakePin) DefaultPull() gpio.Pull { return 0 } -func (p *fakePin) Out(gpio.Level) error { return nil } -func (p *fakePin) PWM(gpio.Duty, physic.Frequency) error { return nil } +func (p *fakePin) String() string { return p.name } +func (p *fakePin) Name() string { return p.name } +func (p *fakePin) Number() int { return -1 } +func (p *fakePin) Function() string { return "" } +func (p *fakePin) Func() pin.Func { return "" } +func (p *fakePin) SupportedFuncs() []pin.Func { return nil } +func (p *fakePin) SetFunc(pin.Func) error { return nil } +func (p *fakePin) Halt() error { return nil } +func (p *fakePin) In(gpio.Pull, gpio.Edge) error { return nil } +func (p *fakePin) Read() gpio.Level { return false } +func (p *fakePin) WaitForEdge(time.Duration) bool { return false } +func (p *fakePin) Pull() gpio.Pull { return 0 } +func (p *fakePin) DefaultPull() gpio.Pull { return 0 } +func (p *fakePin) Out(gpio.Level) error { return nil } +func (p *fakePin) PWM(gpio.Duty, physic.Frequency) error { return nil }