feat: Context-Aware Input Policy Pipeline with Decoupled UI Command Dispatch#314
Open
mverch67 wants to merge 80 commits into
Open
feat: Context-Aware Input Policy Pipeline with Decoupled UI Command Dispatch#314mverch67 wants to merge 80 commits into
mverch67 wants to merge 80 commits into
Conversation
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>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds an input policy pipeline with new interfaces, policies, dispatcher, driver integration, docs, and tests; introduces a WebDAV server singleton; and expands TFT debug/view support, filesystem adapters, and LVGL style and timing adjustments. ChangesInput Policy Pipeline
WebDAV Server
TFT View, Debug Tooling, and Filesystem/Style Fixes
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant KeyMatrixInputDriver
participant InputPipeline
participant PolicyChain
participant UICommandDispatcher
KeyMatrixInputDriver->>InputPipeline: process(InputEvent, capabilities)
InputPipeline->>PolicyChain: evaluate(event, context, capabilities)
PolicyChain-->>InputPipeline: PolicyDecision
alt EmitCommand
InputPipeline->>UICommandDispatcher: dispatch(command, payload)
else Remap/Pass
InputPipeline-->>KeyMatrixInputDriver: outEvents
end
sequenceDiagram
participant WebDAVServer
participant WiFiServer
participant ESPWebDAV
WebDAVServer->>WiFiServer: begin() port 80
WebDAVServer->>ESPWebDAV: begin(tcpServer, filesystem)
WebDAVServer->>WebDAVServer: spawn serverThread task
loop while running
WebDAVServer->>ESPWebDAV: handleClient()
end
Estimated code review effort: 4 (Complex) | ~75 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (10)
tests/test_InputPolicy.cpp (2)
69-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for uncovered branches.
Both tests only exercise
PressKind::Press. Consider adding a case forPressKind::Release(to confirmCommandDispatchPolicycorrectly returns an empty decision, perCommandDispatchPolicy::evaluate's early return) and a case with a nulldispatcherpassed tofactory.build(to confirmCommandDispatchPolicyis correctly omitted perDefaultInputPolicyFactory's conditional).🤖 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 `@tests/test_InputPolicy.cpp` around lines 69 - 94, The current InputPolicy tests only cover the PressKind::Press path in DefaultInputPolicyFactory and CommandDispatchPolicy. Add a Release case in test_InputPolicy.cpp to verify CommandDispatchPolicy::evaluate returns no decision on PressKind::Release, and add a factory.build scenario with a null dispatcher to verify DefaultInputPolicyFactory omits CommandDispatchPolicy when no dispatcher is provided.
81-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMagic key-code values obscure test intent.
0x11(raw/resolved input) and11(expected output) aren't self-explanatory without cross-referencingFocusTraversalPolicy. A short comment or named constant (e.g., referencingLV_KEY_UP/LV_KEY_PREVif that's the intended semantic) would make the assertion's purpose clear without needing to consult another file.♻️ Suggested clarification
InputEvent event{}; event.sourceId = "test"; - event.rawKeyCode = 0x11; - event.resolvedKeyCode = 0x11; + event.rawKeyCode = 0x11; // LV_KEY_UP + event.resolvedKeyCode = 0x11; event.pressKind = PressKind::Press; std::vector<InputEvent> output; const bool forwarded = pipeline.process(event, InputCapabilities{}, output); CHECK(forwarded); REQUIRE(output.size() == 1); - CHECK(output.front().resolvedKeyCode == 11); + CHECK(output.front().resolvedKeyCode == 11); // remapped to LV_KEY_PREV CHECK(dispatcher->dispatchCount == 0);🤖 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 `@tests/test_InputPolicy.cpp` around lines 81 - 94, Clarify the key-code intent in the InputPolicy test by replacing the magic values in the InputEvent setup and expected assertion with named constants or an inline comment tied to FocusTraversalPolicy semantics. Use the existing test case around pipeline.process and output.front().resolvedKeyCode to make it obvious whether the values represent navigation keys like LV_KEY_UP or LV_KEY_PREV, so the assertion reads as intentional rather than arbitrary.docs/input-policy-strategy-design.md (1)
114-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
StrategyDecisionnaming drifted from implementation.The design doc names the decision struct
StrategyDecision, but the actual implementation (include/input/policy/PolicyDecision.h) names itPolicyDecision. Worth aligning terminology to avoid confusion for readers cross-referencing doc and code.🤖 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 `@docs/input-policy-strategy-design.md` around lines 114 - 122, Align the terminology in the design doc with the implementation by renaming the documented `StrategyDecision` concept to `PolicyDecision`, matching `include/input/policy/PolicyDecision.h`; update the section title and field description references so readers can cross-reference the doc with the actual `PolicyDecision` type and its members without confusion.include/input/policy/InputTypes.h (1)
54-63: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
UICommandenum ordering is a hidden contract for the dispatcher's handler array.
UICommandDispatcher(seeinclude/input/policy/UICommandDispatcher.h) sizes its handler storage asstd::array<CommandHandler, static_cast<size_t>(UICommand::LeaveEditMode) + 1>, implicitly relying onLeaveEditModeremaining the last enumerator. Inserting a new command afterLeaveEditMode, or reordering, silently breaks that sizing with no compile-time signal. Consider adding an explicit sentinel (e.g.,Count) as the last enumerator and using it for the array size, so any future insertion is safe by construction.♻️ Suggested pattern
enum class UICommand { None, GoHome, OpenChats, QuickChat, OpenMap, ToggleGps, SendPing, LeaveEditMode, + Count, };And update
UICommandDispatcherto sizehandlersasstatic_cast<size_t>(UICommand::Count).🤖 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/input/policy/InputTypes.h` around lines 54 - 63, The UICommand enum in InputTypes.h is being used as a hidden size contract by UICommandDispatcher’s handler array. Add an explicit terminal sentinel such as Count as the last enumerator in UICommand, keep LeaveEditMode before it, and update UICommandDispatcher to size handlers from static_cast<size_t>(UICommand::Count) instead of relying on LeaveEditMode being last. This makes future command insertions/reordering safe and keeps the enum-to-array mapping stable.include/graphics/view/TFT/TFTView_320x240.h (2)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider marking the single-argument constructor
explicit.Prevents accidental implicit conversion from
lv_group_t*toGroupGuardat call sites.♻️ Proposed tweak
- GroupGuard(lv_group_t *target_group) : saved_group(lv_group_get_default()) + explicit GroupGuard(lv_group_t *target_group) : saved_group(lv_group_get_default())🤖 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/view/TFT/TFTView_320x240.h` at line 21, The single-argument constructor in GroupGuard should be marked explicit to prevent unintended implicit conversions from lv_group_t* at call sites. Update the GroupGuard constructor that takes target_group so it cannot be used for implicit construction, while keeping the existing saved_group initialization behavior unchanged.
236-236: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFriend grant is unconditional even though
TFTView_Debug's body is gated byLVGL_DEBUG_FOCUS.
TFTView_Debug(perTFTView_Debug.h) only has members whenLVGL_DEBUG_FOCUSis defined; otherwise it's an empty class. Granting friendship here unconditionally couples this view class to a debug-only type even in release builds where the friendship is unused. Consider gating the friend declaration the same way to keep the coupling scoped to debug builds.♻️ Proposed tweak
+#if defined(LVGL_DEBUG_FOCUS) friend class TFTView_Debug; +#endif🤖 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/view/TFT/TFTView_320x240.h` at line 236, The unconditional friend declaration for TFTView_Debug in TFTView_320x240 should be scoped to the same debug guard used by TFTView_Debug itself. Update the TFTView_320x240 class declaration so the friend relationship is only declared when LVGL_DEBUG_FOCUS is enabled, keeping the debug-only coupling out of release builds and matching the conditional structure in TFTView_Debug.include/input/policy/InputContextState.h (1)
30-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing explicit includes for
uint32_t/std::string.
setActivePanelId(uint32_t panelId)andsetFocusedClassHint(const std::string &hint)useuint32_tandstd::stringdirectly, but the file only includesIInputContextProvider.h. These types are presumably pulled in transitively, but that's fragile — an unrelated change to the transitive include chain could break this header's compilation.🔧 Suggested fix
`#pragma` once `#include` "input/policy/IInputContextProvider.h" +#include <cstdint> +#include <string>Also applies to: 32-32
🤖 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/input/policy/InputContextState.h` at line 30, Add the missing direct type includes in InputContextState.h so it does not rely on transitive headers for uint32_t and std::string. Update the header around InputContextState, setActivePanelId, and setFocusedClassHint to include the proper standard definitions explicitly, keeping IInputContextProvider.h as the interface dependency but making the parameter types self-contained.source/input/policy/InputPipeline.cpp (1)
46-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExhaustive switch with unreachable trailing fallback.
The
switchonDecisionTypecoversPass,Remap,Consume,EmitCommand, andEmitSequence— if that's the complete enum, lines 72-73 are unreachable dead code. This is likely intentional defensive coding against future enum extensions without a compiler-Wswitchwarning being noticed, so leaving as-is is reasonable; just flagging for awareness.include/util/SdFatFileWrapper.h (2)
34-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
overridespecifiers on virtual overrides.None of the overridden virtuals in
SdFatFSImpl(andSdFat32FSImpl/SdFsExFatImplbelow) useoverride. Adding it lets the compiler catch signature drift againstfs::FileImpl/fs::FSImpl(e.g. if a base signature changes in a library update), which is otherwise silent since these methods would just become new, non-overriding overloads. since correctness hinges on the exact upstreamfs::FileImpl/fs::FSImplsignatures matching, not shown here.🤖 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/util/SdFatFileWrapper.h` around lines 34 - 107, Add override to every virtual method in SdFatFSImpl and the related SdFat32FSImpl/SdFsExFatImpl classes so the compiler verifies they still match fs::FileImpl/fs::FSImpl. Update the method declarations like write, read, flush, seek, position, size, close, name, isDirectory, openNextFile, seekDir, getNextFileName, rewindDirectory, and operator bool to use override, keeping the signatures aligned with the base interfaces.
109-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate logic between
SdFat32FSImplandSdFsExFatImpl.The two classes are identical except for the referenced type (
SdFat&vsSdFs&). Consider a shared template base to avoid maintaining two copies ofopen/exists/rename/remove/mkdir/rmdir.♻️ Suggested refactor sketch
template <typename SdT> class SdFsImplBase : public fs::FSImpl { protected: SdT &sd; public: SdFsImplBase(SdT &sd) : sd(sd) {} virtual fs::FileImplPtr open(const char *path, const char *mode, const bool create) { return std::make_shared<SdFatFSImpl>(sd.open(path, _convert_access_mode_to_flag(mode, create))); } virtual bool exists(const char *path) { return sd.exists(path); } virtual bool rename(const char *pathFrom, const char *pathTo) { return sd.rename(pathFrom, pathTo); } virtual bool remove(const char *path) { return sd.remove(path); } virtual bool mkdir(const char *path) { return sd.mkdir(path); } virtual bool rmdir(const char *path) { return sd.rmdir(path); } }; using SdFat32FSImpl = SdFsImplBase<SdFat>; using SdFsExFatImpl = SdFsImplBase<SdFs>;🤖 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/util/SdFatFileWrapper.h` around lines 109 - 157, `SdFat32FSImpl` and `SdFsExFatImpl` duplicate the same `open`, `exists`, `rename`, `remove`, `mkdir`, and `rmdir` logic, differing only by the wrapped storage type. Refactor these classes in `SdFatFileWrapper.h` into a shared template base (for example, a generic `SdFsImplBase` that stores the `sd` reference and implements the common `fs::FSImpl` methods), then make the two existing implementations thin aliases or specializations for `SdFat` and `SdFs`. Keep `open` using the existing `_convert_access_mode_to_flag` and `SdFatFSImpl` behavior unchanged.
🤖 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 `@docs/input-policy-strategy-design.md`:
- Around line 296-312: The raw key code table in the policy doc is out of sync
with DefaultBindingResolver’s actual mapping, so update the documented
KeyMap/DefaultBindingResolver relationship to match the implementation. Fix the
table entries and surrounding text to reflect the real raw-code-to-InputAction
cases used by DefaultBindingResolver.cpp, including the CommandQuickChat entry
and the correct ordering for CommandHome, CommandOpenChats, CommandOpenMap,
CommandToggleGps, and CommandSendPing. Ensure the description of the resolver
path remains consistent with UICommandDispatcher::instance() and the emitted
DecisionType::EmitCommand flow.
In `@include/comms/WebDAVServer.h`:
- Line 51: The WebDAV entrypoint in WebDAVServer::start(fs::FS*) currently
exposes the filesystem without any access-control gating. Add an explicit
security gate to this API by either wiring in an auth layer before serving the
filesystem or requiring a clearly named unsecured-sharing flag/mode before
start() can expose the SD card. Make the restriction visible in the WebDAVServer
interface and enforce it in the start implementation so the server cannot be
launched anonymously by default.
In `@include/graphics/driver/LGFXDriver.h`:
- Line 339: The comment on the touch timer period is stale and conflicts with
defaultTouchReadPeriodMs being 20ms. Update the inline note near
lv_timer_set_period in LGFXDriver.h to match the current 50Hz rate, or remove
the frequency claim if it is no longer needed, so the comment accurately
reflects the code.
In `@include/input/policy/UICommandDispatcher.h`:
- Line 41: The handler array sizing in UICommandDispatcher is still tied to the
last enum value, so update the UICommand enum to include a Count sentinel and
use that for the std::array size in UICommandDispatcher::handlers. Make sure any
indexing or iteration over UICommand values uses the new Count-based bound
rather than relying on LeaveEditMode as the tail.
In `@include/util/SdFatFileWrapper.h`:
- Around line 1-6: Add a header include guard to SdFatFileWrapper.h so the class
and inline definitions are only seen once per translation unit. Place a guard at
the top of the header before the existing includes, or use `#pragma` once, and
make sure it wraps the current contents without changing the SdFat-related
declarations.
- Around line 8-32: The helper _convert_access_mode_to_flag currently has an
operator-precedence bug in the 'w' and 'a' branches, where O_WRONLY/O_APPEND is
combined with create incorrectly and the write/append access bits are lost.
Update those return expressions in _convert_access_mode_to_flag so the
conditional applies only to O_CREAT while preserving O_WRONLY or O_APPEND, and
verify the same fix for the append path. Keep the behavior consistent for all
mode cases handled by this function.
- Around line 101-105: The directory traversal stubs in SdFatFileWrapper are
returning a bogus filename and not resetting iteration, which breaks consumers
of the Arduino FS-style API. Update the getNextFileName overloads to return the
conventional empty-string sentinel instead of a placeholder, and make
rewindDirectory actually reset the wrapped file/directory state so listing can
restart correctly. Use the existing SdFatFileWrapper methods and _file member to
keep the behavior aligned with directory-listing semantics.
- Around line 86-93: The name() implementation in SdFatFileWrapper uses a shared
static _name buffer, so successive calls across files or instances can overwrite
previously returned names. Change SdFatFileWrapper::name() to use a per-instance
buffer or otherwise return storage owned by the object instead of a shared
static, and update any related construction/destruction logic so each file
wrapper preserves its own name across calls. Keep the existing _file.getName()
flow, but remove the aliasing risk introduced by the static buffer.
In `@source/comms/WebDAVServer.cpp`:
- Around line 130-153: Reset the WebDAV transfer state so the UI can’t get stuck
in “Transfer in progress...”: in WebDAVServer::stop() clear transferInProgress
and lastTransfer100Time, and in WebDAVServer::onTransferProgress() /
WebDAVServer::isTransferInProgress() add activity tracking for non-100% progress
so stalled or cancelled transfers can time out instead of relying only on a 100%
callback.
- Around line 68-102: initWiFi currently returns success immediately after
WiFi.begin() in WebDAVServer::initWiFi, even though the station may still be
connecting or may fail later. Update the method so it returns actual connection
status by waiting with a bounded timeout for WL_CONNECTED, or otherwise adjust
the caller/UI flow to treat this as “connecting” instead of “ready.” Use the
existing WiFi setup and WiFiEventHandler flow to keep the logic localized.
- Around line 255-296: The WebDAV server shutdown path has two competing
task-termination owners: WebDAVServer::stop() force-deletes serverTaskHandle
after a delay while the task wrapper also self-deletes with
vTaskDelete(nullptr), which can race and double-delete the same task. Update the
teardown flow so only one side owns task destruction, and have stop() wait for
an explicit task-exited signal before cleaning up dav/ownedTcpServer and
clearing serverTaskHandle, using the existing running/serverReady state and
WebDAVServer::stop() as the place to coordinate the shutdown.
- Around line 10-13: WebDAVServer::instance() in the fallback build currently
returns nullptr, which breaks callers that expect a usable object; change it to
return a no-op singleton stub instead so the UI toggle path can safely call
initWiFi(). Also add the missing initMDNS() stub on WebDAVServer so the fallback
public API remains link-consistent when ESPWebDAV.h is unavailable under
HAS_SDCARD.
- Around line 198-252: The successful path in WebDAVServer::start() deadlocks
because notifyStatusChange() is called while mutex is still held, and that
method re-enters the same lock through isWiFiConnected(). Move the
notifyStatusChange() call out of the locked section in start() (after the
std::lock_guard scope ends), keeping the state setup and xTaskCreatePinnedToCore
handling inside the mutex-protected block.
- Around line 91-95: Store the wifi_event_id_t values returned by
WebDAVServer::initWiFi() when registering WiFi.onEvent handlers, instead of
discarding them. Then update WebDAVServer::deinitWiFi() to call
WiFi.removeEvent(id) for each stored ID so the existing callbacks are explicitly
unregistered; do not rely on WiFi.onEvent(nullptr, ...) for cleanup.
In `@source/input/I2CKeyboardInputDriver.cpp`:
- Around line 626-638: The pipeline output in I2CKeyboardInputDriver::readInput
is only updating data->key, so STC8H remapped events can keep a stale LVGL state
from earlier processing. In the outEvent handling block, assign data->state from
outEvent.pressKind (pressed vs released) before returning, and keep the existing
outKey==0 early return behavior unchanged. Use the readInput pipeline output
section and the outEvent.pressKind mapping as the place to fix this.
- Around line 216-225: The CardKB read path is ignoring the injected I2C bus and
still using the global Wire instance. Update CardKBInputDriver::readKeyboard to
perform the request/availability/read operations through the stored wire member
initialized in CardKBInputDriver::CardKBInputDriver, so devices registered on
Wire1 use the correct bus. Keep the fix localized to the CardKBInputDriver
methods and ensure all I2C access in this flow uses the injected bus
consistently.
In `@source/input/I2CKeyboardScanner.cpp`:
- Around line 137-139: Remove the stale multi-pass retry logic from
I2CKeyboardScanner::scan, since the rewritten scanner no longer declares or uses
pass. Update the conditional block that checks
I2CKeyboardInputDriver::getI2CKeyboardList() and kScanPasses so the single-pass
flow compiles cleanly, and remove any related delay(kInterPassDelayMs) retry
handling that depends on the missing pass variable.
In `@source/input/KeyMatrixInputDriver.cpp`:
- Around line 199-210: The KeyMatrixInputDriver pipeline output handling in the
KeyMatrix input path updates data->key but never applies outEvent.pressKind to
data->state, so remapped release events can be reported with the wrong LVGL
state. Update the post-pipeline branch in the input processing logic around the
existing outEvent logging to set data->state from outEvent.pressKind (pressed vs
released) before returning, while preserving the current no-output fallback
behavior.
- Around line 68-70: KeyMatrixInputDriver::init() is currently inheriting the
wrong read callback from I2CKeyboardInputDriver::init(), so the matrix path
never uses KeyMatrixInputDriver::keyboard_read. Update
KeyMatrixInputDriver::init() to register the matrix-specific read callback
instead of the I2C scanner callback, using the
KeyMatrixInputDriver::keyboard_read symbol so the matrix keyboard input is
dispatched correctly.
In `@source/input/policy/DefaultBindingResolver.cpp`:
- Around line 11-18: The raw command key constants in DefaultBindingResolver are
out of sync with the design doc’s “Current raw code assignments” table. Update
the KEY_CMD_* values and labels in the resolver to match the documented mapping,
and remove or rename the undocumented CommandQuickChat/0x102 assignment if it is
not part of the current spec. Make sure the raw key table in
DefaultBindingResolver.cpp is aligned with docs/input-policy-strategy-design.md
so integrators wiring new keyboard sources see the correct values.
---
Nitpick comments:
In `@docs/input-policy-strategy-design.md`:
- Around line 114-122: Align the terminology in the design doc with the
implementation by renaming the documented `StrategyDecision` concept to
`PolicyDecision`, matching `include/input/policy/PolicyDecision.h`; update the
section title and field description references so readers can cross-reference
the doc with the actual `PolicyDecision` type and its members without confusion.
In `@include/graphics/view/TFT/TFTView_320x240.h`:
- Line 21: The single-argument constructor in GroupGuard should be marked
explicit to prevent unintended implicit conversions from lv_group_t* at call
sites. Update the GroupGuard constructor that takes target_group so it cannot be
used for implicit construction, while keeping the existing saved_group
initialization behavior unchanged.
- Line 236: The unconditional friend declaration for TFTView_Debug in
TFTView_320x240 should be scoped to the same debug guard used by TFTView_Debug
itself. Update the TFTView_320x240 class declaration so the friend relationship
is only declared when LVGL_DEBUG_FOCUS is enabled, keeping the debug-only
coupling out of release builds and matching the conditional structure in
TFTView_Debug.
In `@include/input/policy/InputContextState.h`:
- Line 30: Add the missing direct type includes in InputContextState.h so it
does not rely on transitive headers for uint32_t and std::string. Update the
header around InputContextState, setActivePanelId, and setFocusedClassHint to
include the proper standard definitions explicitly, keeping
IInputContextProvider.h as the interface dependency but making the parameter
types self-contained.
In `@include/input/policy/InputTypes.h`:
- Around line 54-63: The UICommand enum in InputTypes.h is being used as a
hidden size contract by UICommandDispatcher’s handler array. Add an explicit
terminal sentinel such as Count as the last enumerator in UICommand, keep
LeaveEditMode before it, and update UICommandDispatcher to size handlers from
static_cast<size_t>(UICommand::Count) instead of relying on LeaveEditMode being
last. This makes future command insertions/reordering safe and keeps the
enum-to-array mapping stable.
In `@include/util/SdFatFileWrapper.h`:
- Around line 34-107: Add override to every virtual method in SdFatFSImpl and
the related SdFat32FSImpl/SdFsExFatImpl classes so the compiler verifies they
still match fs::FileImpl/fs::FSImpl. Update the method declarations like write,
read, flush, seek, position, size, close, name, isDirectory, openNextFile,
seekDir, getNextFileName, rewindDirectory, and operator bool to use override,
keeping the signatures aligned with the base interfaces.
- Around line 109-157: `SdFat32FSImpl` and `SdFsExFatImpl` duplicate the same
`open`, `exists`, `rename`, `remove`, `mkdir`, and `rmdir` logic, differing only
by the wrapped storage type. Refactor these classes in `SdFatFileWrapper.h` into
a shared template base (for example, a generic `SdFsImplBase` that stores the
`sd` reference and implements the common `fs::FSImpl` methods), then make the
two existing implementations thin aliases or specializations for `SdFat` and
`SdFs`. Keep `open` using the existing `_convert_access_mode_to_flag` and
`SdFatFSImpl` behavior unchanged.
In `@tests/test_InputPolicy.cpp`:
- Around line 69-94: The current InputPolicy tests only cover the
PressKind::Press path in DefaultInputPolicyFactory and CommandDispatchPolicy.
Add a Release case in test_InputPolicy.cpp to verify
CommandDispatchPolicy::evaluate returns no decision on PressKind::Release, and
add a factory.build scenario with a null dispatcher to verify
DefaultInputPolicyFactory omits CommandDispatchPolicy when no dispatcher is
provided.
- Around line 81-94: Clarify the key-code intent in the InputPolicy test by
replacing the magic values in the InputEvent setup and expected assertion with
named constants or an inline comment tied to FocusTraversalPolicy semantics. Use
the existing test case around pipeline.process and
output.front().resolvedKeyCode to make it obvious whether the values represent
navigation keys like LV_KEY_UP or LV_KEY_PREV, so the assertion reads as
intentional rather than arbitrary.
🪄 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: 0beafb7e-eabb-46db-9d5a-53a8c6a92e73
⛔ Files ignored due to path filters (102)
generated/ui_320x240/.eez-project-buildis excluded by!**/generated/**generated/ui_320x240/actions.his excluded by!**/generated/**generated/ui_320x240/fonts.his excluded by!**/generated/**generated/ui_320x240/images.cis excluded by!**/generated/**generated/ui_320x240/images.his excluded by!**/generated/**generated/ui_320x240/screens.cis excluded by!**/generated/**generated/ui_320x240/screens.his excluded by!**/generated/**generated/ui_320x240/styles.cis excluded by!**/generated/**generated/ui_320x240/styles.his excluded by!**/generated/**generated/ui_320x240/ui.cis excluded by!**/generated/**generated/ui_320x240/ui_image_battery_bolt_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_battery_empty_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_battery_empty_warn_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_battery_full_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_battery_low_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_battery_mid_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_battery_plug_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_bluetooth_on_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_circle_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_circle_question_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_debug_log_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_gps_location_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_groups_bell_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_groups_bell_slash_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_groups_button_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_groups_key_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_groups_lock_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_groups_unlock_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_bell_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_bell_slash_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_bluetooth_off_button_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_bluetooth_on_button_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_button_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_clock_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_ethernet_button_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_fair_signal_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_good_signal_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_location_button_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_lora_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_mail_button_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_mail_unread_button_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_memory_button.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_no_signal_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_nodes_button_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_qr_button_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_sd_card_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_signal_button_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_strong_signal_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_weak_signal_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_webdav_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_wlan_button_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_home_wlan_off_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_hourgalss_start_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_key_generate_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_keyboard_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_knob_logo_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_location_lock_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_location_pin_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_lock_channel_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_lock_secure_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_lock_slash_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_map_button_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_meshtastic_boot_logo_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_meshtastic_logo_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_messages_button_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_mqtt_logo2_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_msg_popup_button_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_nav_down_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_nav_home_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_nav_left_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_nav_right_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_nav_up_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_no_tile_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_node_client_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_node_location_pin24_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_node_router_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_node_sensor_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_nodes_button_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_radar_beam_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_reboot_bt_on_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_reboot_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_settings_button_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_settings_trash_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_shutdown_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_signal_full_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_signal_slider_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_top_chart_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_top_chats_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_top_group_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_top_loop_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_top_lora_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_top_map_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_top_message_node_images.cis excluded by!**/generated/**generated/ui_320x240/ui_image_top_neighbors_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_top_node_detector_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_top_nodes_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_top_settings_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_unmessagable_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_user_question_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_zoom_minus_image.cis excluded by!**/generated/**generated/ui_320x240/ui_image_zoom_plus_image.cis excluded by!**/generated/**generated/ui_320x240/vars.his excluded by!**/generated/**
📒 Files selected for processing (47)
docs/input-policy-strategy-design.mdinclude/comms/WebDAVServer.hinclude/graphics/driver/LGFXDriver.hinclude/graphics/view/TFT/TFTView_320x240.hinclude/graphics/view/TFT/TFTView_Debug.hinclude/input/I2CKeyboardInputDriver.hinclude/input/policy/CommandDispatchPolicy.hinclude/input/policy/DefaultBindingResolver.hinclude/input/policy/DefaultInputPolicyFactory.hinclude/input/policy/FocusTraversalPolicy.hinclude/input/policy/IActionBindingResolver.hinclude/input/policy/IInputContextProvider.hinclude/input/policy/IInputPolicy.hinclude/input/policy/IInputSource.hinclude/input/policy/IUICommandDispatcher.hinclude/input/policy/InputContextState.hinclude/input/policy/InputPipeline.hinclude/input/policy/InputPolicyFactory.hinclude/input/policy/InputSourceRegistry.hinclude/input/policy/InputTypes.hinclude/input/policy/PassthroughPolicy.hinclude/input/policy/PolicyChain.hinclude/input/policy/PolicyDecision.hinclude/input/policy/UICommandDispatcher.hinclude/util/SdFatFileWrapper.hsource/comms/WebDAVServer.cppsource/graphics/TFT/TFTView_320x240.cppsource/graphics/TFT/TFTView_Debug.cppsource/graphics/TFT/Themes.cppsource/graphics/TFT/object_name_entries_320x240.incsource/graphics/map/MapTile.cppsource/input/I2CKeyboardInputDriver.cppsource/input/I2CKeyboardScanner.cppsource/input/KeyMatrixInputDriver.cppsource/input/policy/CommandDispatchPolicy.cppsource/input/policy/DefaultBindingResolver.cppsource/input/policy/DefaultInputPolicyFactory.cppsource/input/policy/FocusTraversalPolicy.cppsource/input/policy/InputContextState.cppsource/input/policy/InputPipeline.cppsource/input/policy/InputSourceRegistry.cppsource/input/policy/PassthroughPolicy.cppsource/input/policy/PolicyChain.cppsource/input/policy/UICommandDispatcher.cppsource/util/LogRotate.cppstudio/320x240/TFT320x240.eez-projecttests/test_InputPolicy.cpp
| * @param filesystem Pointer to fs::FS (must be properly initialized) | ||
| * @return true if started successfully, false if WiFi not connected or already running | ||
| */ | ||
| bool start(fs::FS *filesystem); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether WebDAV auth is configured anywhere in this PR.
rg -n -C3 'ESPWebDAV|WebDAVServer|auth|Auth|credential|password|begin\s*\(' include sourceRepository: meshtastic/device-ui
Length of output: 37910
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the WebDAV server interface and its call sites.
sed -n '1,220p' include/comms/WebDAVServer.h
printf '\n====\n'
sed -n '1,380p' source/comms/WebDAVServer.cpp
printf '\n====\n'
rg -n -C2 'WebDAVServer::instance\(|start\(fs::FS \*|setTransferStatusCallback|ESPWebDAV|dav->begin\(' source include
# Look for any config or UI path that could supply WebDAV credentials or an explicit unsecured mode.
printf '\n====\n'
rg -n -C3 'webdav|WebDAV|dav://|unsecured|auth|authent|credential|password|basic auth|username' source include
# If the ESPWebDAV dependency is vendored or referenced in repo docs, inspect for auth support.
printf '\n====\n'
rg -n -C3 'class ESPWebDAV|set.*Auth|auth.*ESPWebDAV|username|password' .Repository: meshtastic/device-ui
Length of output: 50376
🌐 Web query:
ESPWebDAV authentication API begin username password docs
💡 Result:
The ESPWebDAV library by d-a-v does not provide a native API method for setting a username and password directly within the library class [1][2]. Because ESPWebDAV is designed to integrate with standard HTTP server libraries (such as ESP8266WebServer or WebServer for ESP32), authentication must be handled at the HTTP server level [1][2]. To implement authentication, you should use the built-in HTTP authentication methods provided by the web server framework you are using alongside ESPWebDAV. The general approach is: 1. Configure your web server instance (e.g., ESP8266WebServer) to handle authentication using its native request handling methods [3][4]. 2. When a request comes in, the web server checks for valid credentials before passing the request to the ESPWebDAV hook [1][2]. 3. If using hookWebDAVForWebserver to route WebDAV traffic, ensure your authentication logic is applied to the paths handled by the WebDAV server [2][3]. As noted in the library documentation, implementation of credentials is listed as a feature context for developers, and users are encouraged to handle security through the parent web server instance [1]. Please consult the documentation for your specific web server library (e.g., ESP8266WebServer::requestAuthentication) to configure the required username and password checks [3][4].
Citations:
- 1: https://github.com/d-a-v/ESPWebDAV
- 2: https://context7.com/d-a-v/espwebdav/llms.txt
- 3: https://github.com/d-a-v/ESPWebDAV/blob/v2/examples/Hooked/Hooked.ino
- 4: https://github.com/d-a-v/ESPWebDAV/blob/v2/examples/Hooked_esp32/Hooked_esp32.ino
Add an explicit auth or unsecured-only gate for WebDAV
start(fs::FS*) exposes the filesystem over WebDAV with no way to configure credentials or access control, and the ESPWebDAV integration here doesn’t provide a native auth path. Add an auth layer in front of this server or require an explicit unsecured sharing mode before exposing the SD card.
🤖 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/comms/WebDAVServer.h` at line 51, The WebDAV entrypoint in
WebDAVServer::start(fs::FS*) currently exposes the filesystem without any
access-control gating. Add an explicit security gate to this API by either
wiring in an auth layer before serving the filesystem or requiring a clearly
named unsecured-sharing flag/mode before start() can expose the SD card. Make
the restriction visible in the WebDAVServer interface and enforce it in the
start implementation so the server cannot be launched anonymously by default.
Contributor
There was a problem hiding this comment.
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 liftPreserve all emitted pipeline events.
InputPipeline::processcan return multipleoutEventsforEmitSequence, but this path forwards onlyoutput.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
⛔ Files ignored due to path filters (1)
generated/ui_320x240/screens.cis excluded by!**/generated/**
📒 Files selected for processing (11)
docs/input-policy-strategy-design.mdinclude/graphics/driver/LGFXDriver.hinclude/input/policy/InputTypes.hinclude/input/policy/UICommandDispatcher.hinclude/util/SdFatFileWrapper.hsource/graphics/TFT/TFTView_320x240.cppsource/input/I2CKeyboardInputDriver.cppsource/input/I2CKeyboardScanner.cppsource/input/KeyMatrixInputDriver.cppsource/input/policy/InputPipeline.cppstudio/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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
source/graphics/common/SdCard.cpp (1)
79-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLeftover debug conditional and misleading "SD_MMC" log strings in the SPI-based init path.
#if 1 //#ifdefSDCARD_INIT_SPIlooks like a debug scaffold left permanently enabled; if the intent was to gate this block, use#ifdef SDCARD_INIT_SPIdirectly. 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 viaSD.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
📒 Files selected for processing (8)
include/comms/WebDAVServer.hinclude/graphics/common/SdCard.hinclude/util/ILog.hsource/comms/WebDAVServer.cppsource/graphics/TFT/TFTView_320x240.cppsource/graphics/common/SdCard.cppsource/graphics/map/SdFatService.cppsource/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; |
Contributor
There was a problem hiding this comment.
🎯 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:
- 1: https://github.com/espressif/arduino-esp32/blob/master/libraries/SD/src/SD.h
- 2: https://github.com/espressif/arduino-esp32/blob/7ff99fc4/libraries/SD/src/SD.h
🏁 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 || trueRepository: 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 on lines
+22
to
+24
| uint32_t WebDAVServer::RSSI() const | ||
| { | ||
| return 0; |
Contributor
There was a problem hiding this comment.
🎯 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 sourceRepository: 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 -nRepository: 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"); |
Contributor
There was a problem hiding this comment.
🎯 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 sourceRepository: 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 -nRepository: 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 -nRepository: 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.hRepository: 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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