Skip to content

fix(write): named GPIO write values (on/high/true) were silently driving pins LOW (gap-hunt round 2) - #3

Open
SuperInstance wants to merge 1 commit into
readme-audit-2026-07-10from
gap-hunt-2026-07-11
Open

fix(write): named GPIO write values (on/high/true) were silently driving pins LOW (gap-hunt round 2)#3
SuperInstance wants to merge 1 commit into
readme-audit-2026-07-10from
gap-hunt-2026-07-11

Conversation

@SuperInstance

Copy link
Copy Markdown
Owner

Gap-hunt pass (goose), independently re-verified by me (rebuilt firmware + test env on the exact pushed commit, hand-traced the fixed logic against the header's documented interface — no real ESP32 hardware is available in this environment, so like round 3's original verification for this repo, this is compile-clean + logic-traced, not device-executed).

Bug: executeWrite() used valueStr.toInt() to decide the GPIO level. On Arduino, String::toInt() returns 0 for any non-numeric input — so write <pin> on, write <pin> high, and write <pin> true all silently drove the pin LOW, directly contradicting the documented interface (both the README and the help text in CommandParser.cpp advertise on/off, high/low as valid values). A real, functional bug: any deployment following the repo's own documented usage for named values would get the opposite of the requested pin state.

Fix: explicitly parses the named aliases (on/high/true/1 -> HIGH, off/low/false/0 -> LOW, case-insensitive) before falling back to numeric parsing — matching the contract already implemented (but previously unused for this path) by parseValue() in CommandParser.cpp.

Verification (independently redone):

  • Pulled the exact pushed commit, ran pio run -e esp32dev (SUCCESS) and pio test -e test --without-uploading --without-testing (compiles clean) myself.
  • Hand-traced the new boolean logic against all 8 named aliases plus 0/1: every truthy alias (on,high,true,1) resolves to HIGH, every falsy alias (off,low,false,0) resolves to LOW — matches the new regression test (test_write_named_values) exactly.
  • Note (same caveat as round 3's original verification of this repo): no real ESP32 device is available in this environment, so Unity test assertions aren't actually executed here, only compiled — this is the same verification standard already established and accepted for this repo.

executeWrite() used String::toInt() to decide the GPIO level. On Arduino,
String::toInt() returns 0 for ANY non-numeric input, so 'write <pin> on',
'high', and 'true' silently drove the pin LOW — directly contradicting the
documented interface (README + help text both advertise on/off, high/low).

Parse the named aliases explicitly before falling back to numeric, matching
the contract already implemented (but unused) by parseValue() in
CommandParser.cpp. Add a regression test covering all eight aliases plus
0/1; update the README test count.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant