adafruit / adafruit/Adafruit_Wippersnapper_Arduino

digitalio: is_inverted ignored + first-read change detection uses guessed baseline

Open
#951 1 comment 0 reactions 1 assignee Claimed by @tyeth View on GitHub
Dominant language
C++
Stars
54
Forks
56
Avg merge
2d 12h
Merged PRs (30d)
11

Description

### Component

digitalio (`DigitalIOHardware`) — affects native GPIO and expander (seesaw/MCP/etc.) pins

### Summary

Two digitalio pin-correctness bugs surfaced while bringing up a Seesaw expander (Adafruit LED Arcade Button 1x4, PID 5296) on a LilyGO T-Dongle C5. Both live in [`src/components/digitalIO/hardware.cpp`](https://github.com/adafruit/Adafruit_Wippersnapper_Arduino/blob/api-v2-pins-as-strings/src/components/digitalIO/hardware.cpp) and apply equally to native and expander pins.

### Bug 1 — `is_inverted` is silently ignored

`ws_digitalio_Add.is_inverted` is decoded but never applied. `Handle_DigitalIO_Add()` constructs the pin without it, so there is no way to request an active-low pin — `isInverted: true` in the Add is a no-op for both reads and writes.

```cpp
// controller.cpp (before)
new DigitalIOHardware(pin_num, msg->gpio_direction, msg->sample_mode,
initial_value, (ulong)(msg->period * 1000.0f), expander_drv);
// msg->is_inverted dropped on the floor
```

**Fix:** thread `is_inverted` through the constructor and apply it consistently — `ReadValue()` reports the logical (inverted) value, `Write()` and `SetMode()`'s initial OUTPUT drive emit the inverted electrical level. `_value` now holds the logical value; the electrical level is inverted for active-low pins.

### Bug 2 — first-read change detection uses a guessed baseline

`_prv_value` is seeded from `initial_value` (which is `false` for an input with no initial write). The first `CheckEvent()` therefore compares a *real* reading against a *fabricated* "previous" value:

```cpp
bool DigitalIOHardware::CheckEvent() {
ReadValue();
if (_value == _prv_value) return false; // _prv_value is a guess on the first call
_prv_value = _value;
return true;
}
```

- If the guess is the opposite of the real initial level → a **phantom edge** is emitted.
- If the guess happens to match the real level → the **true initial state is swallowed** and the baseline is (coincidentally) seeded.

Either way the first comparison is meaningless because `_prv_value` can't be known before the first read.

**Fix:** add a `_first_read` flag; the first `CheckEvent()` seeds `_prv_value` from the actual reading and reports it as the pin's initial state, then subsequent reads do normal change detection.

### Fix / verification

- Commit: https://github.com/adafruit/Adafruit_Wippersnapper_Arduino/commit/dcb1f047
- Included in #943 (base `api-v2-pins-as-strings`)
- Carried by #949 (LilyGO T-Dongle C5 board PR)

HIL-tested on a T-Dongle C5 with an Adafruit LED Arcade Button 1x4 (seesaw @ 0x3A): the switch on a seesaw GPIO pin (`EXP_0x3a_18`, `INPUT_PULLUP`, event mode) now reports clean `value: 0` on press / `value: 1` on release.

### Note

The seesaw driver has no pull-down direction available because the digitalio `Direction` enum only defines `D_INPUT`, `D_INPUT_PULL_UP`, `D_OUTPUT` — no `D_INPUT_PULL_DOWN`. Not needed for this active-low button, but worth considering for active-high inputs (seesaw and ESP32 both support `INPUT_PULLDOWN`).

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.