Skip to content

feat: Context-Aware Input Policy Pipeline with Decoupled UI Command Dispatch#314

Open
mverch67 wants to merge 86 commits into
masterfrom
input-policy
Open

feat: Context-Aware Input Policy Pipeline with Decoupled UI Command Dispatch#314
mverch67 wants to merge 86 commits into
masterfrom
input-policy

Conversation

@mverch67

@mverch67 mverch67 commented May 18, 2026

Copy link
Copy Markdown
Collaborator
  • input handling is now policy-driven and context-aware, so the same keys can adapt to focused UI state instead of hard-coded per-driver behavior

  • the new pipeline cleanly separates key source, key-to-action mapping, policy evaluation, and UI command dispatch, which reduces coupling and makes behavior easier to extend

  • it also adds support for dedicated command keys and structured event decisions (pass, remap, consume, emit command/sequence) for predictable, testable input behavior

E.g. a keyboard with cursor keys can now use the cursor keys to navigate the map (context-aware) or move inside text input areas. ESC key to leave this mode.

Note:
As a usage example the new classes are used within the KeyMatrixInputDriver (PICOmputer). See also here.

Summary by CodeRabbit

  • New Features
    • Added a documented, policy-driven input pipeline with focus-aware remapping and UI command dispatch.
    • Introduced a WebDAV server with WiFi/MDNS start-stop, running/status reporting, and transfer progress tracking.
    • Expanded I2C keyboard support with new TM9 and STC8H drivers and pipeline-based matrix event handling.
  • Bug Fixes
    • Improved LVGL touch responsiveness with a configurable read timer (default 20ms) and safer timer setup.
    • Enhanced widget overlay/input grouping behavior and added WebDAV UI status update support.
    • Improved LVGL styling consistency (selector casting) and keyboard device probing reliability.
  • Tests
    • Added automated tests covering input policy and UI command dispatch behavior.

mverch67 and others added 30 commits May 13, 2026 03:23
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
source/input/I2CKeyboardInputDriver.cpp (1)

628-628: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Preserve all emitted pipeline events.

InputPipeline::process can return multiple outEvents for EmitSequence, but this path forwards only output.front() and silently drops the rest. Queue the remaining events and drain one per LVGL read, or explicitly reject sequence decisions for this driver path.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@source/input/I2CKeyboardInputDriver.cpp` at line 628, The
I2CKeyboardInputDriver event forwarding path is dropping all but the first event
from InputPipeline::process output, so preserve the full emitted sequence
instead of only consuming output.front(). Update the logic around the affected
read/poll handling in I2CKeyboardInputDriver to either enqueue the remaining
outEvents and drain them across subsequent LVGL reads, or explicitly block
EmitSequence decisions for this driver so no events are silently lost.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@source/input/I2CKeyboardInputDriver.cpp`:
- Line 628: The I2CKeyboardInputDriver event forwarding path is dropping all but
the first event from InputPipeline::process output, so preserve the full emitted
sequence instead of only consuming output.front(). Update the logic around the
affected read/poll handling in I2CKeyboardInputDriver to either enqueue the
remaining outEvents and drain them across subsequent LVGL reads, or explicitly
block EmitSequence decisions for this driver so no events are silently lost.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6ed152c2-af46-42eb-8178-a428feda2e18

📥 Commits

Reviewing files that changed from the base of the PR and between 9a89158 and b354140.

⛔ Files ignored due to path filters (1)
  • generated/ui_320x240/screens.c is excluded by !**/generated/**
📒 Files selected for processing (11)
  • docs/input-policy-strategy-design.md
  • include/graphics/driver/LGFXDriver.h
  • include/input/policy/InputTypes.h
  • include/input/policy/UICommandDispatcher.h
  • include/util/SdFatFileWrapper.h
  • source/graphics/TFT/TFTView_320x240.cpp
  • source/input/I2CKeyboardInputDriver.cpp
  • source/input/I2CKeyboardScanner.cpp
  • source/input/KeyMatrixInputDriver.cpp
  • source/input/policy/InputPipeline.cpp
  • studio/320x240/TFT320x240.eez-project
✅ Files skipped from review due to trivial changes (1)
  • docs/input-policy-strategy-design.md
🚧 Files skipped from review as they are similar to previous changes (7)
  • include/input/policy/UICommandDispatcher.h
  • include/graphics/driver/LGFXDriver.h
  • source/input/policy/InputPipeline.cpp
  • include/input/policy/InputTypes.h
  • include/util/SdFatFileWrapper.h
  • source/input/KeyMatrixInputDriver.cpp
  • source/input/I2CKeyboardScanner.cpp

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (1)
source/graphics/common/SdCard.cpp (1)

79-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Leftover debug conditional and misleading "SD_MMC" log strings in the SPI-based init path.

#if 1 // #ifdef SDCARD_INIT_SPI looks like a debug scaffold left permanently enabled; if the intent was to gate this block, use #ifdef SDCARD_INIT_SPI directly. Separately, the log strings ("No SD_MMC card detected/attached", "SD_MMC Card Type") are copy-pasted from the SD_MMC path but this branch initializes via SD.begin() over SPI, not SD_MMC — worth correcting to avoid confusing diagnostics when SDCARD_SHARE_SPI is in use.

♻️ Proposed cleanup
-#if 1 // `#ifdef` SDCARD_INIT_SPI
+#ifdef SDCARD_INIT_SPI
     SDHandler.end();
     SDHandler.begin(SPI_SCK, SPI_MISO, SPI_MOSI);
     SD.end();
     if (!SD.begin(SDCARD_CS, SDHandler, SD_SPI_FREQUENCY)) {
-        ILOG_DEBUG("No SD_MMC card detected");
+        ILOG_DEBUG("No SD card detected");
         return false;
     }
 `#endif`
     uint8_t cardType = SD.cardType();
     if (cardType == CARD_NONE) {
-        ILOG_DEBUG("No SD_MMC card attached");
+        ILOG_DEBUG("No SD card attached");
         return false;
     }
-    ILOG_DEBUG("SD_MMC Card Type: %s", cardType == CARD_MMC    ? "MMC"
+    ILOG_DEBUG("SD Card Type: %s", cardType == CARD_MMC    ? "MMC"
                                        : cardType == CARD_SD   ? "SDSC"
                                        : cardType == CARD_SDHC ? "SDHC"
                                                                : "UNKNOWN");
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@source/graphics/common/SdCard.cpp` around lines 79 - 96, The SPI-based SD
initialization in SdCard.cpp still has a leftover debug scaffold and misleading
SD_MMC diagnostics. Replace the temporary `#if` 1 // `#ifdef` SDCARD_INIT_SPI guard
around the SD.begin() path with the intended compile-time gate, and update the
log messages in this block (including the cardType logging) to refer to the SPI
SD/SD card path rather than SD_MMC. Use the existing SdCard initialization flow
and SD.begin()/SD.cardType() symbols to locate and correct the affected
messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@include/graphics/common/SdCard.h`:
- Around line 3-15: The SDCARD_SHARE_SPI branch in SdCard.h is using
FS_NO_GLOBALS, but SD.h on arduino-esp32 ignores that guard and still exposes
the global SD instance. Update the include guard in this branch to use the
supported NO_GLOBAL_INSTANCES or NO_GLOBAL_SD macro before including SD.h,
keeping the shared SPI setup while preventing the extra global from being
declared.

In `@include/util/ILog.h`:
- Around line 45-79: The log-level guards in ILOG.h are comparing against
ESP_LOG_* enum values, so the preprocessor checks do not gate the
ILOG_DEBUG/INFO/WARN/ERROR/TRACE macros correctly. Update the guard logic in the
ILOG_* definitions to use numeric compile-time levels instead of ESP_LOG_* enum
constants, or remove these wrappers and rely on ESP-IDF’s native log filtering;
keep the existing macro names and DEVICEUI_ESP_LOG_LEVEL usage so the change
stays localized.

In `@source/comms/WebDAVServer.cpp`:
- Around line 144-147: The initMDNS() flow in WebDAVServer currently logs
success unconditionally and lets the caller treat mDNS as initialized even when
MDNS.begin() fails. Update initMDNS() to detect the MDNS.begin(WEBDAV_HOSTNAME)
result, report failure explicitly, and return a status (or otherwise signal
error) so the caller only sets the initialized flag when startup succeeds; use
the WebDAVServer::initMDNS and its caller’s mDNS-initialized handling to locate
the fix.
- Around line 22-24: `WebDAVServer::RSSI()` is returning an unsigned type, which
wraps normal negative WiFi dBm readings into large positive values and breaks
the UI logic. Change the RSSI API to a signed type in `WebDAVServer::RSSI()` and
update all matching implementations/call sites, including the threshold handling
in `TFTView_320x240`, so the value stays negative when appropriate (use
`int32_t` or `int8_t` consistently).

In `@source/graphics/common/SdCard.cpp`:
- Around line 237-239: The SdFsCard::init() implementation has an empty
SDCARD_USE_EXT_SDHANDLER branch, so the non-void function can fall through
without returning a value. Update the SdFsCard::init() logic in the
SDCARD_USE_EXT_SDHANDLER path to return a valid result just like the other
branches, and ensure every conditional path in init() ends with an explicit
return.
- Around line 80-86: The SD card initialization path in SdCard.cpp is tearing
down and reinitializing the shared SPI bus via SDHandler.end() and
SDHandler.begin(), which can disrupt other active peripherals. Update the SDCard
initialization logic in the SdCard::begin/initialization flow to avoid touching
the shared bus state here, and rely on the existing SPI coordination/lock
mechanism instead of reinitializing the bus before SD.begin().

---

Nitpick comments:
In `@source/graphics/common/SdCard.cpp`:
- Around line 79-96: The SPI-based SD initialization in SdCard.cpp still has a
leftover debug scaffold and misleading SD_MMC diagnostics. Replace the temporary
`#if` 1 // `#ifdef` SDCARD_INIT_SPI guard around the SD.begin() path with the
intended compile-time gate, and update the log messages in this block (including
the cardType logging) to refer to the SPI SD/SD card path rather than SD_MMC.
Use the existing SdCard initialization flow and SD.begin()/SD.cardType() symbols
to locate and correct the affected messages.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6b65275f-0ece-4070-bef8-8ffd2be00919

📥 Commits

Reviewing files that changed from the base of the PR and between 6cbed39 and fabccc2.

📒 Files selected for processing (8)
  • include/comms/WebDAVServer.h
  • include/graphics/common/SdCard.h
  • include/util/ILog.h
  • source/comms/WebDAVServer.cpp
  • source/graphics/TFT/TFTView_320x240.cpp
  • source/graphics/common/SdCard.cpp
  • source/graphics/map/SdFatService.cpp
  • source/input/I2CKeyboardInputDriver.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
  • include/comms/WebDAVServer.h
  • source/input/I2CKeyboardInputDriver.cpp

Comment on lines +3 to +15
#define FS_NO_GLOBALS

#if defined(ARCH_PORTDUINO)
#include "PortduinoFS.h"
extern fs::FS &SDFs;

#elif defined(HAS_SD_MMC)
#include "SD_MMC.h" // TODO: replace by SdFat SDIO
#include "SD_MMC.h" // TODO: replace by SdFat SDIO
extern fs::SDMMCFS &SDFs;

#elif defined(SDCARD_SHARE_SPI) // share SPI instance with main project
#include "SD.h"
#include <stdint.h>
extern fs::SDFS &SDFs;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

Does espressif arduino-esp32 SD.h library check for a macro named FS_NO_GLOBALS to suppress the global SD instance, or only NO_GLOBAL_SD/NO_GLOBAL_INSTANCES?

💡 Result:

The Espressif arduino-esp32 SD.h library checks for the macros NO_GLOBAL_INSTANCES and NO_GLOBAL_SD to suppress the global SD instance [1][2]. It does not check for a macro named FS_NO_GLOBALS. Specifically, the library uses the following preprocessor directive in SD.h to conditionally declare the external global SD object: #if!defined(NO_GLOBAL_INSTANCES) &&!defined(NO_GLOBAL_SD) extern fs::SDFS SD; #endif Defining either NO_GLOBAL_INSTANCES or NO_GLOBAL_SD will prevent the global SD instance from being declared [1][2].

Citations:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== SdCard.h ==\n'
git ls-files include/graphics/common/SdCard.h src/graphics/common/SdCard.cpp include/graphics/common/SdCard.cpp 2>/dev/null || true

printf '\n== Search for FS_NO_GLOBALS / NO_GLOBAL_SD / NO_GLOBAL_INSTANCES ==\n'
rg -n --hidden --glob '!**/.git/**' 'FS_NO_GLOBALS|NO_GLOBAL_SD|NO_GLOBAL_INSTANCES|NO_FS_GLOBALS' .

printf '\n== Relevant includes/usages around SDFs ==\n'
rg -n --hidden --glob '!**/.git/**' 'SDFs|`#include` "SD.h"|`#include` "SD_MMC.h"|`#include` "PortduinoFS.h"' include src

printf '\n== File excerpt: include/graphics/common/SdCard.h ==\n'
sed -n '1,120p' include/graphics/common/SdCard.h

printf '\n== File excerpt: src/graphics/common/SdCard.cpp ==\n'
sed -n '1,120p' src/graphics/common/SdCard.cpp 2>/dev/null || true

Repository: meshtastic/device-ui

Length of output: 4263


Use NO_GLOBAL_INSTANCES/NO_GLOBAL_SD here SD.h on arduino-esp32 ignores FS_NO_GLOBALS, so this branch still exposes the global SD instance. If the intent is to share the SPI instance without the extra global, switch to one of the supported guards before including SD.h.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@include/graphics/common/SdCard.h` around lines 3 - 15, The SDCARD_SHARE_SPI
branch in SdCard.h is using FS_NO_GLOBALS, but SD.h on arduino-esp32 ignores
that guard and still exposes the global SD instance. Update the include guard in
this branch to use the supported NO_GLOBAL_INSTANCES or NO_GLOBAL_SD macro
before including SD.h, keeping the shared SPI setup while preventing the extra
global from being declared.

Comment thread include/util/ILog.h
Comment on lines +22 to +24
uint32_t WebDAVServer::RSSI() const
{
return 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify all WebDAV RSSI declarations/callers are updated consistently.
rg -n -C2 '\bRSSI\s*\(' include source

Repository: meshtastic/device-ui

Length of output: 1734


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== WebDAVServer declarations =="
sed -n '1,120p' include/comms/WebDAVServer.h | cat -n

echo
echo "== WebDAVServer implementations =="
sed -n '1,240p' source/comms/WebDAVServer.cpp | cat -n | sed -n '1,220p'

echo
echo "== TFTView RSSI call site =="
sed -n '5954,5968p' source/graphics/TFT/TFTView_320x240.cpp | cat -n

Repository: meshtastic/device-ui

Length of output: 12238


Keep WebDAV RSSI signed. WebDAVServer::RSSI() is exposed as uint32_t, but WiFi RSSI values are negative dBm. That wraps normal readings into huge positives and also breaks the UI threshold check in source/graphics/TFT/TFTView_320x240.cpp; change the API and both implementations to int32_t (or int8_t if you want to mirror Arduino-ESP32).

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@source/comms/WebDAVServer.cpp` around lines 22 - 24, `WebDAVServer::RSSI()`
is returning an unsigned type, which wraps normal negative WiFi dBm readings
into large positive values and breaks the UI logic. Change the RSSI API to a
signed type in `WebDAVServer::RSSI()` and update all matching
implementations/call sites, including the threshold handling in
`TFTView_320x240`, so the value stays negative when appropriate (use `int32_t`
or `int8_t` consistently).

Comment on lines +144 to +147
void WebDAVServer::initMDNS(void)
{
MDNS.begin(WEBDAV_HOSTNAME);
ILOG_INFO("[WebDAV] MDNS started. Advertising %s", WEBDAV_HOSTNAME ".local");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify whether callers rely on initMDNS() always succeeding or need surfaced status.
rg -n -C3 '\binitMDNS\s*\(' include source

Repository: meshtastic/device-ui

Length of output: 1950


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the relevant file and surrounding declarations/uses.
wc -l source/comms/WebDAVServer.cpp include/comms/WebDAVServer.h
sed -n '1,220p' source/comms/WebDAVServer.cpp | cat -n
printf '\n--- HEADER ---\n'
sed -n '1,120p' include/comms/WebDAVServer.h | cat -n

Repository: meshtastic/device-ui

Length of output: 11531


🏁 Script executed:

#!/bin/bash
set -euo pipefail

wc -l source/comms/WebDAVServer.cpp include/comms/WebDAVServer.h
sed -n '1,220p' source/comms/WebDAVServer.cpp | cat -n
printf '\n--- HEADER ---\n'
sed -n '1,120p' include/comms/WebDAVServer.h | cat -n

Repository: meshtastic/device-ui

Length of output: 11531


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show the local definition/use context for WebDAVServer::initMDNS and MDNS includes.
rg -n -C4 'initMDNS|MDNS\.' source/comms/WebDAVServer.cpp include/comms/WebDAVServer.h

Repository: meshtastic/device-ui

Length of output: 1836


Handle MDNS.begin() failure here.
initMDNS() always logs success, and the caller marks mDNS initialized unconditionally, so a failed start leaves the app believing meshtastic.local is advertised when it isn’t. Return a status or log the failure and avoid setting the initialized flag on error.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@source/comms/WebDAVServer.cpp` around lines 144 - 147, The initMDNS() flow in
WebDAVServer currently logs success unconditionally and lets the caller treat
mDNS as initialized even when MDNS.begin() fails. Update initMDNS() to detect
the MDNS.begin(WEBDAV_HOSTNAME) result, report failure explicitly, and return a
status (or otherwise signal error) so the caller only sets the initialized flag
when startup succeeds; use the WebDAVServer::initMDNS and its caller’s
mDNS-initialized handling to locate the fix.

Comment thread source/graphics/common/SdCard.cpp
Comment thread source/graphics/common/SdCard.cpp Outdated
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