LinuxGPIOPin: throw on rejected GPIO reads, writes and acquisitions

README.md

@@ -25,19 +25,19 @@ framework = arduino
 board = ardulinux
 ```
 
-Hardware support is gated on **libgpiod**: if `pkg-config` finds it, real GPIO/I2C are compiled in — this also links **libi2c**, so install the two together. If libgpiod is absent, the build uses fully simulated GPIO/I2C and needs no hardware libraries.
+Hardware support is gated on **libgpiod**: if `pkg-config` finds it, real GPIO/I2C are compiled in; this also links **libi2c**, so install the two together. If libgpiod is absent, the build uses fully simulated GPIO/I2C and needs no hardware libraries.
 
 ## Building standalone (CMake)
 
 Requires GCC or Clang (C++14), CMake 3.17+, and pkg-config. Hardware GPIO/I2C are enabled when libgpiod is detected; libgpiod also requires libi2c, so install both together (or neither, for a simulated build).
 
-ArduinoCore-API and WiFi are git submodules — clone with them, or initialise them after cloning:
+ArduinoCore-API and WiFi are git submodules. Clone with them, or initialise them after cloning:
 ```sh
 git clone --recurse-submodules https://github.com/l5yth/ardulinux.git
 # already cloned?  →  git submodule update --init --recursive
 ```
 
-Install the build dependencies — on Debian/Ubuntu:
+Install the build dependencies. On Debian/Ubuntu:
 ```sh
 sudo apt-get install build-essential cmake libgpiod-dev libi2c-dev pkg-config
 ```
@@ -107,7 +107,7 @@ The VFS root defaults to `$XDG_DATA_HOME/<app>/default` (i.e. `~/.local/share/ar
 
 ### Customizing program identity
 
-The platform reads four optional weak symbols. Define any of them as plain (non-weak) definitions in an application source file to override the defaults — no header required:
+The platform reads four optional weak symbols. Define any of them as plain (non-weak) definitions in an application source file to override the defaults. No header required:
 
 ```cpp
 const char *ardulinuxAppName        = "meshcored";                          // startup msg, VFS dir, libgpiod label (default "ardulinux")

cores/ardulinux/linux/gpio/LinuxGPIOPin.cpp

@@ -182,7 +182,18 @@ gpiod_line *LinuxGPIOPin::getLine(const char *chipLabel, const char *linuxPinNam
 	struct gpiod_line_config *line_cfg;
 	struct gpiod_request_config *req_cfg = NULL;
 	struct gpiod_line_request *line = NULL;
-    offset = gpiod_chip_get_line_offset_from_name(chip, linuxPinName);
+	// Returns -1 (ENOENT) for an unknown name; assigning that to the unsigned
+	// member would request offset 4294967295 instead of reporting the typo.
+	int named_offset = gpiod_chip_get_line_offset_from_name(chip, linuxPinName);
+	if (named_offset < 0) {
+		gpiod_chip_close(chip);
+		chip = NULL;
+		char msg[128];
+		snprintf(msg, sizeof(msg), "Error, no GPIO line named '%s' on %s",
+			linuxPinName ? linuxPinName : "?", chipLabel ? chipLabel : "?");
+		throw std::invalid_argument(msg);
+	}
+	offset = (unsigned int) named_offset;
 	settings = gpiod_line_settings_new();
 	gpiod_line_settings_set_direction(settings, GPIOD_LINE_REQUEST_DIRECTION_AS_IS);
 	line_cfg = gpiod_line_config_new();
@@ -215,6 +226,8 @@ gpiod_line *LinuxGPIOPin::getLine(const char *chipLabel, const char *linuxPinNam
 		consumer, GPIOD_LINE_REQUEST_DIRECTION_AS_IS, 0};
 	auto result = gpiod_line_request(line, &request, 0);
 	if(result != 0) {
+		gpiod_chip_close(chip);
+		chip = NULL;
 		throw std::invalid_argument("Error, cannot open GPIO chip");
 	}
 	return line;
@@ -229,12 +242,26 @@ gpiod_line *LinuxGPIOPin::getLine(const char *chipLabel, const int linuxPinNum)
   if (!chip)
     throw std::invalid_argument("GPIO chip not found");
 
+  // Guard before either library path: a negative offset (a config parser's
+  // "unset" sentinel, say) is unsigned on both sides -- v2's `offset` member
+  // and v1's gpiod_chip_get_line() -- so it would wrap to 4294967295 and be
+  // requested as if it were a real line.  Deliberately outside the #if: the
+  // check needs no version-specific API.
+  if (linuxPinNum < 0) {
+    gpiod_chip_close(chip);
+    chip = NULL;
+    char msg[128];
+    snprintf(msg, sizeof(msg), "Error, invalid GPIO line offset %d on %s",
+             linuxPinNum, chipLabel ? chipLabel : "?");
+    throw std::invalid_argument(msg);
+  }
+
 #if GPIOD_V == 2
 	struct gpiod_line_settings *settings;
 	struct gpiod_line_config *line_cfg;
 	struct gpiod_request_config *req_cfg = NULL;
 	struct gpiod_line_request *line = NULL;
-    offset = linuxPinNum;
+	offset = (unsigned int) linuxPinNum;
 	settings = gpiod_line_settings_new();
 	gpiod_line_settings_set_direction(settings, GPIOD_LINE_REQUEST_DIRECTION_AS_IS);
 	line_cfg = gpiod_line_config_new();
@@ -262,12 +289,16 @@ gpiod_line *LinuxGPIOPin::getLine(const char *chipLabel, const int linuxPinNum)
 	}
 	return line;
 #else
-	auto line = gpiod_chip_get_line(chip, linuxPinNum);
+	// The negative guard above makes this conversion safe; make it explicit so
+	// the intent is not mistaken for the sign bug that guard exists to prevent.
+	auto line = gpiod_chip_get_line(chip, (unsigned int) linuxPinNum);
 
 	struct gpiod_line_request_config request = {
 		consumer, GPIOD_LINE_REQUEST_DIRECTION_AS_IS, 0};
 	auto result = gpiod_line_request(line, &request, 0);
 	if(result != 0) {
+		gpiod_chip_close(chip);
+		chip = NULL;
 		throw std::invalid_argument("Error, cannot open GPIO chip");
 	}
 	return line;
@@ -296,10 +327,32 @@ LinuxGPIOPin::~LinuxGPIOPin() {
     gpiod_chip_close(chip);
 }
 
+/**
+ * Report a libgpiod failure on this line as an exception.
+ *
+ * assert() is not usable here: it is compiled out under NDEBUG, which is what
+ * release builds define, so a runtime gpiod error would go unreported.
+ */
+void LinuxGPIOPin::throwLineError(const char *op) const {
+  char msg[160];
+#if GPIOD_V == 2
+  snprintf(msg, sizeof(msg), "Error, cannot %s GPIO line %u ('%s', pin %u): %s",
+           op, offset, getName(), (unsigned) getPinNum(), strerror(errno));
+#else
+  snprintf(msg, sizeof(msg), "Error, cannot %s GPIO '%s' (pin %u): %s",
+           op, getName(), (unsigned) getPinNum(), strerror(errno));
+#endif
+  log(SysGPIO, LogError, "%s", msg);
+  throw std::runtime_error(msg);
+}
+
 /// Read the low level hardware for this pin
 PinStatus LinuxGPIOPin::readPinHardware() {
     int res = gpiod_line_get_value(line);
-    assert(res == 0 || res == 1); // FIXME throw instead
+    // gpiod reports failure as GPIOD_LINE_VALUE_ERROR (-1). Returning it would
+    // cache -1 as the pin state and fire a phantom ISR from refreshState().
+    if (res != 0 && res != 1)
+        throwLineError("read");
 
     // log(SysGPIO, LogDebug, "readPinHardware(%s, %d)", getName(), res); 
     return (PinStatus) res;
@@ -309,13 +362,26 @@ void LinuxGPIOPin::writePin(PinStatus s) {
   // some libraries have been observed failing to set the pin mode to output.
   if (GPIOPin::getPinMode() != OUTPUT)
 	setPinMode(OUTPUT);
-  GPIOPin::writePin(s); // update status
 
+  // Drive the hardware before caching. GPIOPin::writePin() records `s` as the
+  // pin's state, and once the mode is OUTPUT refreshState() stops re-reading
+  // the hardware, so a value cached for a write that never landed would be
+  // returned by digitalRead() forever.
   int res = gpiod_line_set_value(line, s);
-  assert(res == 0);
+  if (res != 0)
+    throwLineError("write");
+
+  GPIOPin::writePin(s); // update status
 }
 
 void LinuxGPIOPin::setPinMode(PinMode m) {
+#if GPIOD_V == 2
+  // Cache the mode up front: the output-value seed below reads readPin(), which
+  // must return the cached level rather than touching the hardware. If the
+  // reconfigure then fails, the cache is rolled back to `previous` -- leaving it
+  // moved would gate refreshState() on a direction the line does not have.
+  const PinMode previous = GPIOPin::getPinMode();
+#endif
   GPIOPin::setPinMode(m);
 #if GPIOD_V == 1
   // The gpiod call below does not play well with an already claimed GPIO
@@ -357,11 +423,17 @@ void LinuxGPIOPin::setPinMode(PinMode m) {
 	}
 	line_cfg = gpiod_line_config_new();
 	ret = gpiod_line_config_add_line_settings(line_cfg, &offset, 1, settings);
-	if (ret != 0)
-		log(SysGPIO, LogError, "gpiod_line_config_add_line_settings failed: %d", ret);
+	int add_ret = ret;
+	if (add_ret != 0)
+		log(SysGPIO, LogError, "gpiod_line_config_add_line_settings failed: %d", add_ret);
 	ret = gpiod_line_request_reconfigure_lines(line, line_cfg);
 	if (ret != 0)
 		log(SysGPIO, LogError, "gpiod_line_request_reconfigure_lines failed: %d", ret);
+	// Either failure means the line kept its old direction, so the cache must
+	// too.  add_line_settings is checked in its own right rather than trusting
+	// the reconfigure to fail on an empty config: the two are independent.
+	if (add_ret != 0 || ret != 0)
+		GPIOPin::setPinMode(previous);
 
 	gpiod_line_config_free(line_cfg);
 	gpiod_line_settings_free(settings);

cores/ardulinux/linux/gpio/LinuxGPIOPin.h

@@ -59,7 +59,15 @@
  */
 class LinuxGPIOPin : public GPIOPin {
   gpiod_line *line;   ///< Acquired GPIO line handle (type aliased for v1/v2)
-  gpiod_chip *chip;   ///< GPIO chip handle (kept open for reconfiguration)
+  /**
+   * GPIO chip handle.
+   *
+   * Under gpiod v1 the chip is held open for the lifetime of the pin and
+   * closed by the destructor.  Under gpiod v2 a line request outlives the
+   * chip it came from, so getLine() closes the chip as soon as the request
+   * succeeds and resets this to NULL; the destructor's close is then a no-op.
+   */
+  gpiod_chip *chip;
 
 public:
 
@@ -73,6 +81,11 @@ class LinuxGPIOPin : public GPIOPin {
    * @param linuxPinName    Name of the GPIO line within the chip.
    * @param ardulinuxPinName Human-readable name for log messages (defaults to
    *                        linuxPinName if NULL).
+   * @throws std::invalid_argument if the chip is not found, the chip has no
+   *         line by that name, or the line cannot be acquired.  Acquisition is
+   *         deliberately a construction-time failure: a pin that cannot be
+   *         claimed is a configuration error, and the caller should learn that
+   *         when it binds the pin rather than on first use.
    */
   LinuxGPIOPin(pin_size_t n, const char *chipLabel, const char *linuxPinName, const char *ardulinuxPinName = NULL);
 
@@ -83,21 +96,41 @@ class LinuxGPIOPin : public GPIOPin {
    * @param chipLabel       Label of the gpiochip device.
    * @param linuxPinNum     Zero-based offset of the GPIO line within the chip.
    * @param ardulinuxPinName Human-readable name for log messages.
+   * @throws std::invalid_argument if the chip is not found, the offset is
+   *         negative, or the line cannot be acquired.  See the by-name
+   *         constructor for why this fails at construction time.
    */
   LinuxGPIOPin(pin_size_t n, const char *chipLabel, const int linuxPinNum, const char *ardulinuxPinName);
 
   /** Release the GPIO line and close the chip. */
   ~LinuxGPIOPin();
 
 protected:
-  /** Read the current hardware pin level via gpiod_line_get_value(). */
+  /**
+   * Read the current hardware pin level via gpiod_line_get_value().
+   *
+   * @return LOW or HIGH.
+   * @throws std::runtime_error if libgpiod reports an error.  The value is
+   *         never passed through: gpiod signals failure with
+   *         GPIOD_LINE_VALUE_ERROR (-1), which is not a valid PinStatus and
+   *         would otherwise be cached as pin state and fire a spurious ISR.
+   */
   virtual PinStatus readPinHardware();
 
   /**
    * Write a logic level to the pin, auto-switching to OUTPUT mode if needed.
    *
    * Some libraries omit the pinMode(OUTPUT) call; this method silently
    * promotes the pin to output to avoid a silent no-op.
+   *
+   * The hardware is driven before the new level is cached, so a rejected
+   * write leaves the cached state untouched.  That ordering matters: once the
+   * mode is OUTPUT, refreshState() stops re-reading the hardware, so a value
+   * cached for a write that never landed would be returned by digitalRead()
+   * for the rest of the process's life.
+   *
+   * @param s Logic level to drive.
+   * @throws std::runtime_error if libgpiod rejects the write.
    */
   virtual void writePin(PinStatus s);
 
@@ -106,18 +139,36 @@ class LinuxGPIOPin : public GPIOPin {
    *
    * Uses gpiod_line_release + gpiod_line_request_* (v1) or
    * gpiod_line_request_reconfigure_lines (v2).
+   *
+   * A failed reconfiguration is logged at LogError and does not throw: this is
+   * reached from writePin()'s auto-promotion path, where throwing would turn a
+   * recoverable reconfiguration into a lost write.  The cached mode is rolled
+   * back instead, because the line kept its old direction and `mode` is what
+   * gates refreshState() -- a stale OUTPUT would stop all hardware reads and
+   * freeze digitalRead() at its last cached level.
+   *
+   * @param m Direction and bias to apply.
    */
   virtual void setPinMode(PinMode m);
 
-  unsigned int offset; ///< Line offset within the chip (used by gpiod v2)
+  /**
+   * Line offset within the chip.
+   *
+   * Assigned by the gpiod v2 paths in getLine(); the v1 paths address the
+   * line through its own handle and never read this.  Initialised anyway so
+   * the member is never indeterminate, since it is declared unconditionally.
+   */
+  unsigned int offset = 0;
 
 private:
   /**
    * Locate and acquire a GPIO line by numeric offset.
    *
    * @param chipLabel    gpiochip label or device name.
    * @param linuxPinNum  Line offset within the chip.
-   * @return Acquired line handle; throws std::invalid_argument on failure.
+   * @return Acquired line handle.
+   * @throws std::invalid_argument if the chip is not found, the offset is
+   *         negative, or the line cannot be acquired.
    */
   gpiod_line *getLine(const char *chipLabel, const int linuxPinNum);
 
@@ -126,10 +177,24 @@ class LinuxGPIOPin : public GPIOPin {
    *
    * @param chipLabel    gpiochip label or device name.
    * @param linuxPinName Line name as reported by the kernel.
-   * @return Acquired line handle; throws std::invalid_argument on failure.
+   * @return Acquired line handle.
+   * @throws std::invalid_argument if the chip is not found, the chip has no
+   *         line by that name, or the line cannot be acquired.
    */
   gpiod_line *getLine(const char *chipLabel, const char *linuxPinName);
 
+  /**
+   * Throw a std::runtime_error identifying this pin and the failed operation.
+   *
+   * Shared by readPinHardware() and writePin() so both report a libgpiod
+   * failure in the same form, including errno, the line offset (gpiod v2),
+   * the pin name and the Arduino pin number.
+   *
+   * @param op Verb naming the failed operation, e.g. "read" or "write".
+   * @throws std::runtime_error always; the function never returns.
+   */
+  [[noreturn]] void throwLineError(const char *op) const;
+
   /** @defgroup gpiod_v2_compat gpiod v2 compatibility shims
    *
    * gpiod v2 replaced the gpiod_line / gpiod_line_request split with a

tests/unit/CMakeLists.txt

@@ -84,3 +84,32 @@ if(LIBGPIOD_FOUND)
     endif()
     catch_discover_tests(ardulinux-i2c-tests)
 endif()
+
+# ─── LinuxGPIOPin tests ──────────────────────────────────────────────────────
+# LinuxGPIOPin drives real GPIO character devices, which a unit test cannot
+# open (/dev/gpiochip* needs hardware, or gpio-sim plus root).  fake_gpiod.cpp
+# defines the libgpiod symbols LinuxGPIOPin.cpp references; because those
+# definitions come from an object file linked directly into this executable,
+# they win over the shared library and no device is ever touched.
+#
+# A separate executable is required so the fake's gpiod_* symbols cannot
+# displace the real ones in ardulinux-tests, which links libgpiod for the I2C
+# and SPI paths.  Built only when LIBGPIOD_FOUND, since LinuxGPIOPin.cpp is
+# guarded by ARDULINUX_HARDWARE and needs <gpiod.h>.
+if(LIBGPIOD_FOUND)
+    add_executable(ardulinux-gpio-tests
+        test_linux_gpio.cpp
+        fake_gpiod.cpp
+        ${CMAKE_SOURCE_DIR}/cores/ardulinux/linux/gpio/LinuxGPIOPin.cpp
+    )
+    target_link_libraries(ardulinux-gpio-tests PRIVATE ardulinux-base Catch2::Catch2WithMain)
+    target_include_directories(ardulinux-gpio-tests PRIVATE
+        ${CMAKE_SOURCE_DIR}/cores/ardulinux
+        ${CMAKE_CURRENT_SOURCE_DIR}
+    )
+    if(CMAKE_BUILD_TYPE STREQUAL "Coverage")
+        target_compile_options(ardulinux-gpio-tests PRIVATE --coverage -O0 -g)
+        target_link_options(ardulinux-gpio-tests PRIVATE --coverage)
+    endif()
+    catch_discover_tests(ardulinux-gpio-tests)
+endif()