Add SerialImprov support for WiFi provisioning#3
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntegrates Serial Improv provisioning: adds ImprovWiFi dependency, Improv integration and new APIs (startImprovSerial, loop, httpSetup change), a SerialImprov example and test tool, README rewrites for serial provisioning, .env ignored, and removal of the ArduinoOTA example. Changes
Sequence Diagram(s)sequenceDiagram
participant Operator as Operator / test_improv.py
participant UART as Device UART
participant HWS as HeadlessWiFiSettings
participant Improv as ImprovWiFi
rect rgba(230,245,255,0.6)
Note over Operator,UART: Serial Improv provisioning session
Operator->>UART: framed RPC (IDENTIFY / GET_DEVICE_INFO / WIFI_SETTINGS)
UART->>Improv: serial frames received
Improv->>HWS: RPC callbacks / state events
HWS->>HWS: ensureMainEndpoint(), persist credentials (SPIFFS)
HWS->>Improv: startImprovSerial(...) / responds via improv
HWS->>UART: RPC results / state updates / restart trigger
Improv-->>Operator: STATE updates and RPC results over serial
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)examples/SerialImprov/SerialImprov.ino(1 hunks)library.json(1 hunks)library.properties(1 hunks)src/HeadlessWiFiSettings.cpp(1 hunks)src/HeadlessWiFiSettings.h(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/HeadlessWiFiSettings.h (1)
src/HeadlessWiFiSettings.cpp (4)
beginSerialImprov(341-358)beginSerialImprov(341-341)serialImprovLoop(360-362)serialImprovLoop(360-360)
🔇 Additional comments (12)
library.properties (1)
10-11: LGTM!The ImprovWiFi dependency declaration is correctly formatted for Arduino library properties.
README.md (1)
51-68: LGTM!The Serial Improv provisioning documentation is clear and provides a helpful example that aligns with the new API.
examples/SerialImprov/SerialImprov.ino (1)
1-14: LGTM!The example clearly demonstrates Serial Improv usage. The hardcoded firmware version "1.0" on line 7 is acceptable for an example, though in production code this should typically match your actual firmware version.
src/HeadlessWiFiSettings.h (2)
8-8: LGTM!The ImprovWiFi header inclusion is necessary for the new functionality.
23-24: LGTM!The method signatures are clear and follow good API design conventions.
src/HeadlessWiFiSettings.cpp (6)
341-342: LGTM!Calling
begin()ensures proper initialization before setting up Serial Improv.
343-346: LGTM!Proper cleanup of existing improv instance prevents memory leaks when
beginSerialImprovis called multiple times.
348-349: LGTM!The fallback from deviceName to hostname is a sensible default.
350-351: LGTM!Setting info and debug callbacks to Serial.println provides useful diagnostics.
352-357: LGTM with note on blocking behavior.The WiFi callback correctly stores credentials and triggers reconnection. Note that
connect(false)on line 356 is a blocking call that could take up to 60 seconds (default wait_seconds). This is acceptable for a provisioning callback but users should be aware the callback won't return immediately.
360-362: LGTM!The null-check prevents crashes when
serialImprovLoop()is called without prior initialization.library.json (1)
23-24: The dependencyjudge2005/ImprovWiFidoes not appear to exist on GitHub.Based on verification, the GitHub user judge2005 has repositories but no ImprovWiFi repository, and there is no repository named "ImprovWiFi" under the GitHub user judge2005. Alternative ImprovWiFi implementations exist from other sources (e.g., jnthas), but not from judge2005.
The review comment assumes this is a valid dependency and suggests adding a version constraint. However, the underlying issue is that the dependency identifier itself appears to be incorrect or non-existent. Adding a version constraint would not resolve this problem. The actual dependency reference should be corrected to point to the correct ImprovWiFi library implementation.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/HeadlessWiFiSettings.h (1)
56-58: Add destructor to clean up the improv pointer.The
improvpointer is dynamically allocated but the class lacks a destructor. WhileHeadlessWiFiSettingsis a global singleton that typically lives for the program's lifetime, best practice dictates providing a destructor to prevent potential leaks in testing or future refactoring scenarios.Add a destructor declaration in the header:
public: typedef std::function<void(void)> TCallback; typedef std::function<int(void)> TCallbackReturnsInt; typedef std::function<void(String &)> TCallbackString; HeadlessWiFiSettingsClass(); + ~HeadlessWiFiSettingsClass();And implement it in the .cpp file:
+HeadlessWiFiSettingsClass::~HeadlessWiFiSettingsClass() { + if (improv) { + delete improv; + improv = nullptr; + } +} + HeadlessWiFiSettingsClass::HeadlessWiFiSettingsClass() : http(80) { hostname = F("esp32-"); }src/HeadlessWiFiSettings.cpp (1)
373-378: Report provisioning outcome to ImprovWiFi instance.The WiFi callback saves credentials and calls
connect(false)but never updates the ImprovWiFi instance with the outcome. The Improv serial protocol requires explicit state transitions (e.g.,setState(STATE_PROVISIONED)on success orsetError()on failure) so the client knows when provisioning completes. Without these calls, client tools timeout waiting for confirmation.Consider modifying the callback to report outcomes:
improv->setWiFiCallback([this](const char* ssid, const char* password) { spurt("/wifi-ssid", ssid); spurt("/wifi-password", password); if (onConfigSaved) onConfigSaved(); - connect(false); + if (connect(false)) { + if (improv) improv->setState(ImprovTypes::STATE_PROVISIONED); + } else { + if (improv) improv->setError(ImprovTypes::ERROR_UNABLE_TO_CONNECT); + } });Note: Verify the exact ImprovWiFi API for state/error reporting and adjust accordingly.
🧹 Nitpick comments (1)
examples/SerialImprov/test_improv.py (1)
124-136: Remove unused variable.The
total_lenvariable at line 128 is assigned but never used. While this doesn't affect correctness, removing it improves code clarity.Apply this diff:
def decode_rpc_result(payload: bytes) -> Tuple[int, list[str]] | None: if len(payload) < 2: return None command = payload[0] - total_len = payload[1] idx = 2 values: list[str] = [] while idx < len(payload):Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)examples/SerialImprov/SerialImprov.ino(1 hunks)examples/SerialImprov/platformio.ini(1 hunks)examples/SerialImprov/test_improv.py(1 hunks)src/HeadlessWiFiSettings.cpp(5 hunks)src/HeadlessWiFiSettings.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 Additional context used
🧬 Code graph analysis (2)
src/HeadlessWiFiSettings.cpp (1)
examples/SerialImprov/SerialImprov.ino (2)
loop(11-13)loop(11-11)
src/HeadlessWiFiSettings.h (1)
src/HeadlessWiFiSettings.cpp (23)
HeadlessWiFiSettingsClass(721-723)name(65-65)name(67-67)begin(715-719)begin(715-715)connect(657-713)connect(657-657)portal(615-655)portal(615-615)httpSetup(385-613)httpSetup(385-385)startImprovSerial(361-379)startImprovSerial(361-361)loop(381-383)loop(381-381)string(247-257)string(247-247)string(259-263)string(259-259)string(265-270)string(265-265)dropdown(272-283)dropdown(272-272)
🪛 Ruff (0.14.1)
examples/SerialImprov/test_improv.py
128-128: Local variable total_len is assigned to but never used
Remove assignment to unused variable total_len
(F841)
🔇 Additional comments (11)
examples/SerialImprov/test_improv.py (3)
54-97: LGTM!The protocol framing functions (checksum, build_frame, build_rpc, wifi_payload, and packet builders) correctly implement the SerialImprov protocol structure with proper length-prefixed encoding.
99-121: LGTM!The packet parser correctly handles framing, validates version and checksum, manages partial packets, and implements proper buffer cleanup.
164-250: LGTM!The serial communication functions (read_for, wait_for_provisioned) and main test orchestration correctly implement the SerialImprov test flow with appropriate timeouts and error handling.
examples/SerialImprov/SerialImprov.ino (1)
1-13: LGTM!This example sketch correctly demonstrates the SerialImprov provisioning flow by initializing the required subsystems and delegating to the HeadlessWiFiSettings loop for processing.
src/HeadlessWiFiSettings.h (2)
8-8: LGTM!Adding the ImprovWiFi header is necessary for the new SerialImprov integration.
23-24: LGTM!The new public API methods
startImprovSerialandloopprovide a clean interface for enabling SerialImprov provisioning.src/HeadlessWiFiSettings.cpp (5)
13-16: LGTM!The conditional include of
sdkconfig.hfor ESP32 and the addition of<cstring>are appropriate for the platform detection logic added in this PR.
345-359: LGTM!The chip family detection logic comprehensively covers ESP32 variants (C3, S2, S3, base) and ESP8266, with a sensible fallback. This resolves the previous concern about hardcoded chip identifiers.
381-383: LGTM!The
loop()method correctly delegates to the ImprovWiFi instance when present, enabling serial protocol processing.
647-647: LGTM!Calling
loop()within the portal's main loop ensures SerialImprov messages are processed while the configuration portal is active.
697-697: LGTM!Calling
loop()within the connection retry loop ensures SerialImprov messages are processed during WiFi connection attempts.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/HeadlessWiFiSettings.cpp (1)
361-380: Improv provisioning never reports success/failure; host tools will hang.Wi‑Fi callback persists SSID/password and immediately restarts without updating the Improv state or returning an RPC result. Serial Improv clients expect a state transition to PROVISIONED or an explicit error; otherwise they time out.
Action (one of):
- Attempt connection inside the callback (or dispatch a task), then:
- On success: report PROVISIONED via the Improv API, send RPC result, then optionally restart.
- On failure: report UNABLE_TO_CONNECT (or appropriate error) and keep running so the client can retry.
- If you prefer to reboot first: persist a “pending provision” marker and, on boot, perform the connection attempt and immediately emit the current Improv state to complete the transaction before doing anything else.
I can sketch the concrete calls once you confirm the exact ImprovWiFi API surface used here (state/error setters).
examples/SerialImprov/test_improv.py (1)
28-30: Fix Serial IDENTIFY opcode; it collides with GET_STATE.Both
COMMAND_IDENTIFYandCOMMAND_GET_STATEare set to0x02. For the Serial protocol, IDENTIFY uses a different code (GET_STATE is0x02). Update IDENTIFY to the Serial value to avoid ambiguity.Apply:
-COMMAND_IDENTIFY = 0x02 +COMMAND_IDENTIFY = 0x04
🧹 Nitpick comments (3)
src/HeadlessWiFiSettings.cpp (1)
13-16: ESP32‑guarded sdkconfig include is safe;<cstring>may be unnecessary.
No issues with the guard. If<cstring>isn’t used, consider removing to keep includes lean.examples/SerialImprov/test_improv.py (2)
58-66: Guard against oversized payloads.
Length is a single byte; frames >255 bytes will wrap. Consider rejecting payloads ≥256 to avoid ambiguous framing.-def build_frame(packet_type: int, payload: bytes) -> bytes: +def build_frame(packet_type: int, payload: bytes) -> bytes: + if len(payload) > 0xFF: + raise ValueError("payload too large for Serial Improv frame") frame = bytearray(IMPROV_HEADER)
183-197: Provisioning wait relies solely on STATE_PROVISIONED.
This is fine, but many devices also return an RPC result for WIFI_SETTINGS. Optionally treat a successful RPC result as early success and surface ERROR_STATE details (reason codes) for better operator feedback.Happy to add this small enhancement if you want it bundled in this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/SerialImprov/test_improv.py(1 hunks)src/HeadlessWiFiSettings.cpp(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/HeadlessWiFiSettings.cpp (1)
examples/SerialImprov/SerialImprov.ino (2)
loop(11-13)loop(11-11)
🪛 Ruff (0.14.1)
examples/SerialImprov/test_improv.py
128-128: Local variable total_len is assigned to but never used
Remove assignment to unused variable total_len
(F841)
🔇 Additional comments (4)
src/HeadlessWiFiSettings.cpp (4)
369-371: Potential dangling pointer for chipFamily argument.When a non‑empty
chipFamilyis passed,chippoints tochipFamily.c_str()whose storage dies at function return unlessImprovWiFicopies it.improvFirmware,improvVersion, andhostnameare memberStrings and remain valid;chipFamilyis not.
- Please confirm
ImprovWiFicopies string arguments. If not, storechipFamilyinto a memberString(e.g.,improvChipFamily) and pass itsc_str().
382-384: Improv loop hook added — good.
Callingimprov->loop()behind a null check is correct and low overhead.
646-649: Portal loop integration — good placement.
Runningloop()once per portal iteration keeps the Improv state machine responsive without starving WDT.Please confirm this loop also runs while captive portal HTTP handlers may block (AsyncWebServer should be fine).
688-701: Connect loop integration — good, but consider timing.
Invokingloop()during connection retries is appropriate. Ensure youronWaitLoop()delays (default 100 ms) are sufficient for timely Improv updates.If hosts rely on quick state transitions, consider a shorter delay while provisioning is active.
DTTerastar
force-pushed
the
codex/add-serialimprov-support-for-wifi-settings
branch
from
63f4138 to
56328e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
examples/SerialImprov/test_improv.py (1)
140-153: Validate payload bounds and remove unused variable in decode_rpc_result.
- Add declared-length bounds checks and consume exactly that many bytes; fixes malformed frames and Ruff F841.
@@ def decode_rpc_result(payload: bytes) -> Tuple[int, list[str]] | None: if len(payload) < 2: return None command = payload[0] - total_len = payload[1] + total_len = payload[1] idx = 2 values: list[str] = [] - while idx < len(payload): - length = payload[idx] - idx += 1 - values.append(payload[idx : idx + length].decode("utf-8", errors="replace")) - idx += length + if len(payload) - 2 < total_len: + return None + end = 2 + total_len + while idx < end: + if idx >= len(payload): + return None + length = payload[idx] + idx += 1 + if idx + length > len(payload): + return None + values.append(payload[idx : idx + length].decode("utf-8", errors="replace")) + idx += length return command, values
🧹 Nitpick comments (4)
examples/SerialImprov/test_improv.py (4)
84-88: Guard payload lengths and clarify checksum coverage (spec ambiguity).
- Add <=255 checks for RPC and frame payloads to avoid ValueError/overflow on .append().
- Serial spec lists a “simple sum checksum” but doesn’t state whether the header is included; BLE sums preceding bytes. Recommend summing version..data (exclude header) and verify with device; keep if already tested. (improv-wifi.com)
@@ def build_frame(packet_type: int, payload: bytes) -> bytes: - frame = bytearray(IMPROV_HEADER) + if len(payload) > 255: + raise ValueError("Frame payload too large (>255 bytes).") + frame = bytearray(IMPROV_HEADER) frame.append(IMPROV_SERIAL_VERSION) frame.append(packet_type) frame.append(len(payload)) frame.extend(payload) - frame.append(checksum(frame)) + # If device rejects frames, try checksum(frame[6:]) to exclude header per BLE wording. + frame.append(checksum(frame[6:])) return bytes(frame) @@ def build_rpc(command: int, payload: bytes) -> bytes: - inner = bytearray([command, len(payload)]) + if len(payload) > 255: + raise ValueError("RPC data too large (>255 bytes).") + inner = bytearray([command, len(payload)]) inner.extend(payload) return build_frame(TYPE_RPC_COMMAND, inner)To confirm checksum coverage, test against a known ESPHome device implementing improv_serial. (api-docs.esphome.io)
Also applies to: 74-82
34-38: State/error enums include BLE‑only values; harmless, but annotate for clarity.
- 0x01 state (“Authorization Required”) and error 0x04 (“Not Authorized”) are specified for BLE, not Serial; keeping them is fine but consider a comment to avoid confusion. (improv-wifi.com)
Also applies to: 46-53
220-231: Unify password/env handling (avoid unintended env override).
- With default "", env var can override an intentionally empty CLI password. Mirror SSID handling by using None default.
@@ - parser.add_argument("password", nargs="?", default="", help="Wi-Fi password (optional if SERIAL_IMPROV_PASSWORD env set)") + parser.add_argument("password", nargs="?", default=None, help="Wi-Fi password (optional if SERIAL_IMPROV_PASSWORD env set)") @@ - password = args.password if args.password else os.environ.get("SERIAL_IMPROV_PASSWORD", "") + password = args.password if args.password is not None else os.environ.get("SERIAL_IMPROV_PASSWORD", "")
233-245: Ensure serial port is always closed (use context manager).
- Use a with‑block to guarantee closure across early returns.
@@ - try: - ser = serial.Serial(port, 115200, timeout=0.1) - except serial.SerialException as err: + try: + ser = serial.Serial(port, 115200, timeout=0.1) + except serial.SerialException as err: print(f"Could not open {port}: {err}") return 1 @@ - ser.close() - return 0 + ser.close() + return 0Or:
with serial.Serial(port, 115200, timeout=0.1) as ser: ...Also applies to: 270-271
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)examples/SerialImprov/test_improv.py(1 hunks)src/HeadlessWiFiSettings.cpp(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- src/HeadlessWiFiSettings.cpp
🧰 Additional context used
🪛 Ruff (0.14.1)
examples/SerialImprov/test_improv.py
144-144: Local variable total_len is assigned to but never used
Remove assignment to unused variable total_len
(F841)
DTTerastar
force-pushed
the
codex/add-serialimprov-support-for-wifi-settings
branch
from
56328e4 to
46ab9c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/HeadlessWiFiSettings.cpp (1)
382-382: Plaintext Wi‑Fi credentials written to SPIFFS.This issue was flagged in a previous review. Credentials are stored as cleartext files via
spurt("/wifi-ssid", ssid)andspurt("/wifi-password", password). Anyone with flash read access can recover them.Prefer using ESP32 NVS via
WiFi.persistent(true)to let the SDK store credentials securely, or encrypt/obfuscate at rest with a device-unique key.examples/SerialImprov/test_improv.py (3)
29-32: Duplicate command values break Serial Improv protocol.Lines 30-31 assign the same value
0x02to bothCOMMAND_IDENTIFYandCOMMAND_GET_STATE. As noted in previous reviews, the Serial Improv specification doesn't include an IDENTIFY command (that's BLE-only). Command 0x04 is "Request scanned Wi‑Fi networks" in the Serial spec.The constants should be updated to match the Serial protocol specification, replacing IDENTIFY with SCAN_NETWORKS (0x04).
90-97: UTF-8 encoding bug breaks non-ASCII SSIDs/passwords.The
wifi_payloadfunction useslen(ssid)andlen(password)(character count) for the length bytes, but the Improv protocol expects byte lengths. This breaks when SSIDs or passwords contain non-ASCII characters (e.g., "Café-WiFi" encodes to 10 bytes butlen()returns 9).Encode to UTF-8 first, then use
len(encoded_bytes)for the length bytes, and validate that byte length ≤ 255.
140-153: Unused variable and missing payload validation.The
total_lenvariable on Line 144 is assigned but never used (Ruff F841 warning). Additionally, the function doesn't validate that the payload contains the declared number of bytes before parsing, which could lead to partial/garbled decoding.Add validation to ensure
len(payload) - 2 >= total_lenbefore parsing, and add bounds checks when reading each length-prefixed string.
🧹 Nitpick comments (2)
README.md (2)
51-77: Documentation references problematic IDENTIFY command.The Serial Improv provisioning section documentation is clear and helpful. However, Line 76-77 mentions "IDENTIFY/GET STATE RPCs" which may be misleading. As noted in the review of
test_improv.py, the Serial Improv specification doesn't include an IDENTIFY command (0x02 is GET_STATE in Serial, while IDENTIFY is BLE-only). The Serial spec uses command 0x04 for scanning networks instead.Consider updating the documentation to reflect the correct Serial protocol commands: GET_STATE (0x02), GET_DEVICE_INFO (0x03), and SCAN_NETWORKS (0x04), or clarify which protocol variant the helper script implements.
193-193: Minor markdown lint: specify language for fenced code block.The fenced code block on Line 193 is missing a language identifier. Consider adding it for consistency:
-``` +```bash POST /wifiThis addresses the markdownlint warning MD040.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)README.md(3 hunks)examples/ArduinoOTA/ArduinoOTA.ino(0 hunks)examples/SerialImprov/SerialImprov.ino(1 hunks)examples/SerialImprov/test_improv.py(1 hunks)src/HeadlessWiFiSettings.cpp(8 hunks)
💤 Files with no reviewable changes (1)
- examples/ArduinoOTA/ArduinoOTA.ino
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/SerialImprov/SerialImprov.ino
🧰 Additional context used
🧬 Code graph analysis (1)
src/HeadlessWiFiSettings.cpp (1)
examples/SerialImprov/SerialImprov.ino (2)
loop(94-96)loop(94-94)
🪛 LanguageTool
README.md
[grammar] ~575-~575: Ensure spelling is correct
Context: ...liseconds until the next call (default: 100ms). ```C++ HeadlessWiFiSettings.onWaitLo...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
README.md
193-193: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.1)
examples/SerialImprov/test_improv.py
144-144: Local variable total_len is assigned to but never used
Remove assignment to unused variable total_len
(F841)
🔇 Additional comments (14)
.gitignore (1)
10-10: LGTM!Adding
.envto the gitignore is a best practice that prevents accidental commits of environment variable files containing sensitive credentials or configuration. This aligns with the PR's addition of provisioning features that may require configuration during development and testing.src/HeadlessWiFiSettings.cpp (8)
13-16: LGTM!The conditional include of
sdkconfig.hfor ESP32 and the addition of<cstring>are appropriate for the platform detection logic introduced later in the file.
24-26: LGTM!The static
ERROR_FLASHconstant and theensureMainEndpoint()helper provide consistent error messaging and defensive initialization of the default endpoint.
215-225: LGTM!The
ensureMainEndpoint()function and its integration intoparams()prevent potential out-of-bounds access when the endpoint vectors are empty, ensuring safe initialization.
353-367: LGTM!The platform-aware
defaultChipFamilycovers ESP32 variants (C3, S2, S3) and ESP8266 comprehensively, with a sensible fallback to "ESP32" for unknown platforms.
395-397: LGTM!The
loop()function correctly delegates to the Improv instance when present, with appropriate null-check.
615-616: LGTM!Using the
ERROR_FLASHconstant for consistent error messaging is good practice.
663-663: LGTM!Calling
loop()in the portal's main loop enables Improv message processing during the configuration portal phase.
713-713: LGTM!Calling
loop()in the connection retry loop allows Improv messages to be processed while waiting for WiFi connection.README.md (5)
79-118: LGTM!The WiFi configuration documentation accurately describes the portal mode, HTTP endpoints, and provides correct curl examples that match the implementation.
139-226: LGTM!The HTTP API reference accurately documents the endpoints, request/response formats, and behavior including password masking and signal strength semantics. The JSON examples match the implementation.
227-346: LGTM!The complete usage example is comprehensive and demonstrates proper usage of callbacks, parameters, endpoints, and Serial Improv integration. All function signatures and patterns match the implementation.
394-489: LGTM!The function reference accurately documents all public APIs including the new
startImprovSerial()andloop()functions. Function signatures, parameters, and behavior match the implementation.
526-631: LGTM!The callbacks section comprehensively documents all available callbacks with correct signatures and helpful examples. The
onRestartcallback documentation correctly notes it's called before restart (e.g., after Serial Improv credentials).
DTTerastar
force-pushed
the
codex/add-serialimprov-support-for-wifi-settings
branch
from
46ab9c4 to
a0ba30c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
examples/SerialImprov/test_improv.py (3)
29-32: Critical: Duplicate command value and Serial protocol mismatch.Both
COMMAND_IDENTIFYandCOMMAND_GET_STATEare assigned0x02. According to the Improv Serial protocol specification, there is no IDENTIFY command in the Serial protocol (that's BLE-only). The Serial protocol uses:
- 0x01: WIFI_SETTINGS
- 0x02: GET_STATE (Request current state)
- 0x03: GET_DEVICE_INFO
- 0x04: SCAN_NETWORKS (Request scanned Wi-Fi networks)
Apply this diff to fix the protocol compliance:
COMMAND_WIFI_SETTINGS = 0x01 -COMMAND_IDENTIFY = 0x02 COMMAND_GET_STATE = 0x02 COMMAND_GET_DEVICE_INFO = 0x03 +COMMAND_SCAN_NETWORKS = 0x04Then update the helper function (lines 99-100), the response handler (lines 170-171), and the call site (lines 251-253) to use
COMMAND_SCAN_NETWORKSinstead ofCOMMAND_IDENTIFY.
90-97: Critical: UTF-8 length calculation bug in wifi_payload.Using
len(ssid)andlen(password)returns character counts, not byte counts. For non-ASCII characters (e.g., "Café"), this will send incorrect length bytes and cause protocol errors.Apply this diff:
def wifi_payload(ssid: str, password: str) -> bytes: + ssid_b = ssid.encode("utf-8") + pwd_b = password.encode("utf-8") + if len(ssid_b) > 255 or len(pwd_b) > 255: + raise ValueError("SSID/password too long for Improv (max 255 bytes)") body = bytearray() - body.append(len(ssid)) - body.extend(ssid.encode()) - body.append(len(password)) - body.extend(password.encode()) + body.append(len(ssid_b)) + body.extend(ssid_b) + body.append(len(pwd_b)) + body.extend(pwd_b) return bytes(body)
140-153: Minor: Unused variable and missing payload length validation.The
total_lenvariable is assigned but never used, and there's no validation that the payload contains the declared number of bytes, which could lead to partial/garbled decoding.Apply this diff:
def decode_rpc_result(payload: bytes) -> Tuple[int, list[str]] | None: if len(payload) < 2: return None command = payload[0] total_len = payload[1] idx = 2 values: list[str] = [] + # Validate payload has declared bytes + if len(payload) - 2 < total_len: + return None + end = 2 + total_len - while idx < len(payload): + while idx < end: + if idx >= len(payload): + return None length = payload[idx] idx += 1 + if idx + length > len(payload): + return None values.append(payload[idx : idx + length].decode("utf-8", errors="replace")) idx += length return command, valuessrc/HeadlessWiFiSettings.cpp (2)
353-376: Critical: Improv provisioning outcome never reported.The WiFi callback (line 364) saves credentials and immediately restarts without:
- Attempting to connect to the network
- Reporting success/failure to the Improv client via
improv->setState(STATE_PROVISIONED)or error callbacksThis violates the Improv protocol specification, which requires the device to explicitly signal provisioning outcome. The host tool will timeout waiting for
STATE_PROVISIONEDeven if credentials are valid.Consider this flow instead:
improv->setWiFiCallback([this](const char* ssid, const char* password) { bool ok = spurt("/wifi-ssid", ssid) && spurt("/wifi-password", password); if (!ok) { Serial.println(ERROR_FLASH); if (improv) improv->handleConnectionResult(false); return; } if (onConfigSaved) onConfigSaved(); // Attempt connection WiFi.disconnect(true, true); WiFi.begin(ssid, password); unsigned long start = millis(); while (WiFi.status() != WL_CONNECTED && millis() - start < 30000) { delay(100); } bool connected = WiFi.status() == WL_CONNECTED; if (improv) improv->handleConnectionResult(connected); if (connected) { if (onSuccess) onSuccess(); } else { if (onFailure) onFailure(); } if (onRestart) onRestart(); delay(500); ESP.restart(); });
365-365: Major security concern: Plaintext credentials in SPIFFS.WiFi credentials are stored as plaintext files in SPIFFS (
/wifi-ssid,/wifi-password). Anyone with physical access or flash dump capability can recover them.Recommended mitigations:
- Preferred: Use ESP32's NVS with
WiFi.persistent(true)beforeWiFi.begin()to let the SDK store credentials securely- Alternative: Encrypt credentials at rest using device-unique keys (eFuse MAC-derived or hardware key storage) with AES-GCM
Note: HTTP endpoints already mask passwords correctly (
***###***).
🧹 Nitpick comments (1)
README.md (1)
193-198: Add language identifier to fenced code block.The code block showing the POST request format should specify a language (e.g.,
httportext) for proper syntax highlighting.Apply this diff:
-``` +```http POST /wifi Content-Type: application/x-www-form-urlencoded
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)README.md(3 hunks)examples/ArduinoOTA/ArduinoOTA.ino(0 hunks)examples/SerialImprov/SerialImprov.ino(1 hunks)examples/SerialImprov/test_improv.py(1 hunks)src/HeadlessWiFiSettings.cpp(8 hunks)
💤 Files with no reviewable changes (1)
- examples/ArduinoOTA/ArduinoOTA.ino
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (1)
src/HeadlessWiFiSettings.cpp (1)
examples/SerialImprov/SerialImprov.ino (2)
loop(94-96)loop(94-94)
🪛 LanguageTool
README.md
[grammar] ~575-~575: Ensure spelling is correct
Context: ...liseconds until the next call (default: 100ms). ```C++ HeadlessWiFiSettings.onWaitLo...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
README.md
193-193: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.1)
examples/SerialImprov/test_improv.py
144-144: Local variable total_len is assigned to but never used
Remove assignment to unused variable total_len
(F841)
🔇 Additional comments (12)
README.md (2)
51-78: LGTM! Clear Serial Improv documentation.The Serial Improv provisioning section clearly documents initialization, usage, and provides a practical Python testing workflow. The examples align well with the new API surface.
227-314: LGTM! Comprehensive usage example.The complete usage example effectively demonstrates most features including callbacks, endpoint configuration, and Serial Improv integration. This provides excellent guidance for users.
examples/SerialImprov/test_improv.py (3)
56-68: LGTM! Clean environment file loader.The
.envfile parser properly handles comments, empty lines, and strips quotes from values. The use ofsetdefaultensures environment variables aren't overwritten.
115-137: LGTM! Robust packet parser with checksum validation.The parser correctly:
- Handles incomplete packets (waits for more data)
- Validates protocol version
- Verifies checksums
- Removes invalid data and continues parsing
216-272: LGTM! Well-structured CLI with environment variable support.The main function properly handles:
- Environment variable fallback for configuration
- Serial port initialization with optional reset
- Command sequencing (boot, identify, device info, state, provisioning)
- Timeout handling with appropriate exit codes
examples/SerialImprov/SerialImprov.ino (3)
5-70: LGTM! Clean provisioning UI.The HTML page provides a simple, accessible interface with:
- Clear form structure
- Responsive feedback
- Automatic restart after successful save
- Good error handling for failed requests
72-92: Verify HTTP response is sent before restart.Line 86 has a 100ms delay before
ESP.restart(), which may be too short to ensure the HTTP response is fully transmitted to the client, especially over slower connections or when buffering is involved.Consider increasing to at least 500ms to ensure reliable response delivery:
- delay(100); + delay(500);Alternatively, use
request->onDisconnect()to restart only after the client receives the response, though this adds complexity.
94-96: LGTM! Proper integration with HeadlessWiFiSettings.The loop correctly delegates to
HeadlessWiFiSettings.loop()to process Serial Improv messages.src/HeadlessWiFiSettings.cpp (4)
24-24: LGTM! Consistent error messaging.Introducing
ERROR_FLASHconstant provides consistent error messaging across HTTP and provisioning paths.
215-220: LGTM! Ensures endpoint stability.The
ensureMainEndpoint()function guarantees a main endpoint exists before operations, preventing crashes when accessing endpoint collections before parameter registration.
378-380: LGTM! Clean loop integration.The public
loop()method correctly delegates to the Improv instance when active, enabling message processing in user's main loop.
646-646: LGTM! Improv integration in blocking loops.Adding
loop()calls in bothportal()(line 646) andconnect()(line 696) ensures Serial Improv messages are processed even during blocking connection/portal operations.Also applies to: 696-696
DTTerastar
force-pushed
the
codex/add-serialimprov-support-for-wifi-settings
branch
from
a0ba30c to
cc49f06
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
examples/SerialImprov/platformio.ini (1)
10-12: Missing library dependencies for SerialImprov example (unresolved from prior review).The
lib_depsare missing the required dependencies. The example code depends onHeadlessWiFiSettingsandImprovWiFi, but neither is declared. Whilelib_extra_dirs = ../../points to the parent library directory, PlatformIO requires explicitlib_depsentries for proper dependency resolution and transitive dependency management.Add the missing dependencies to
lib_deps:lib_deps = ESP32Async/AsyncTCP ESP32Async/ESPAsyncWebServer + player-one/HeadlessWiFiSettings + judge2005/ImprovWiFiAlternatively, if
HeadlessWiFiSettingsshould not be added as a library dependency (since this is an example within the library itself), verify thatlib_extra_dirscorrectly resolves the parent library and addjudge2005/ImprovWiFias the explicit dependency.src/HeadlessWiFiSettings.cpp (1)
353-376: Immediate restart prevents Improv outcome reporting and stores plaintext credentials.Two critical issues remain from past reviews:
Line 374:
ESP.restart()is called immediately after saving credentials without attempting connection or callingimprov->handleConnectionResult(). The Improv protocol expects explicit success/failure signals; the host tool will timeout waiting forSTATE_PROVISIONED.Line 365: WiFi credentials are stored as plaintext in SPIFFS. Anyone with flash access can recover them.
Consider:
- Attempt connection before restart and report outcome via
improv->handleConnectionResult(success)- Use ESP32 NVS (
WiFi.persistent(true)) for secure credential storage, or encrypt at rest with device-unique keysexamples/SerialImprov/test_improv.py (3)
29-33: Duplicate command value and protocol mismatch.Lines 30-31 assign the same value
0x02to bothCOMMAND_IDENTIFYandCOMMAND_GET_STATE. According to the Improv Wi-Fi Serial protocol specification,IDENTIFYis a BLE-only command (not part of Serial), and0x04should beSCAN_NETWORKSfor Serial implementations.This issue was already documented in past review comments.
90-97: UTF-8 length bug breaks non-ASCII credentials.Lines 92 and 94 use
len(ssid)andlen(password)which return character counts, not byte lengths. For non-ASCII SSIDs or passwords, this causes incorrect length prefixes and breaks protocol framing.This issue was already documented in past review comments.
140-153: Unused variable and missing payload length validation.Line 144 assigns
total_lenbut never uses it. The function should either validate thatlen(payload) - 2 >= total_lenbefore parsing, or remove the unused variable.This issue was already documented in past review comments.
🧹 Nitpick comments (1)
README.md (1)
193-198: Consider adding language identifier to fenced code block.The fenced code block on Line 195 is missing a language identifier. While not critical, adding
bashorshellwould improve syntax highlighting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignore(1 hunks)README.md(3 hunks)examples/ArduinoOTA/ArduinoOTA.ino(0 hunks)examples/SerialImprov/SerialImprov.ino(1 hunks)examples/SerialImprov/platformio.ini(1 hunks)examples/SerialImprov/test_improv.py(1 hunks)src/HeadlessWiFiSettings.cpp(8 hunks)
💤 Files with no reviewable changes (1)
- examples/ArduinoOTA/ArduinoOTA.ino
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/SerialImprov/SerialImprov.ino
🧰 Additional context used
🧬 Code graph analysis (1)
src/HeadlessWiFiSettings.cpp (1)
examples/SerialImprov/SerialImprov.ino (2)
loop(94-96)loop(94-94)
🪛 LanguageTool
README.md
[grammar] ~575-~575: Ensure spelling is correct
Context: ...liseconds until the next call (default: 100ms). ```C++ HeadlessWiFiSettings.onWaitLo...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
README.md
193-193: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.1)
examples/SerialImprov/test_improv.py
144-144: Local variable total_len is assigned to but never used
Remove assignment to unused variable total_len
(F841)
🔇 Additional comments (11)
.gitignore (1)
10-10: LGTM!Adding
.envto.gitignoreis standard practice and prevents accidental commits of environment variables used by the test script.README.md (2)
51-78: LGTM!The Serial Improv provisioning documentation clearly explains the feature with practical examples that match the implementation.
139-346: LGTM!The HTTP API reference and complete usage example provide comprehensive documentation that accurately reflects the implementation. The curl examples and code samples are practical and helpful.
src/HeadlessWiFiSettings.cpp (8)
24-27: LGTM!The
ERROR_FLASHconstant promotes code reuse, and the forward declaration forensureMainEndpoint()follows good practices.
215-220: LGTM!The
ensureMainEndpoint()function is a defensive programming pattern that guarantees a default "main" endpoint exists, preventing potential null access bugs throughout the codebase.
378-380: LGTM!The
loop()implementation correctly delegates toimprov->loop()with appropriate null-pointer protection.
222-229: LGTM!Strategic placement of
ensureMainEndpoint()calls inparams()andfindOrCreateEndpoint()prevents accessing empty endpoint vectors.
512-512: LGTM!The call to
ensureMainEndpoint()before endpoint lookup prevents potential null access issues.
598-600: LGTM!Consistent use of the
ERROR_FLASHconstant with appropriate logging to Serial improves maintainability and debugging.
646-646: LGTM!Integrating
loop()into the portal's wait loop enables Serial Improv provisioning during captive portal mode.
696-696: LGTM!Calling
loop()during WiFi connection attempts ensures Serial Improv messages are processed while waiting for connection.
DTTerastar
force-pushed
the
codex/add-serialimprov-support-for-wifi-settings
branch
from
cc49f06 to
efea95b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
examples/SerialImprov/test_improv.py (3)
29-32: Fix duplicate command value: IDENTIFY conflicts with GET_STATE.Both
COMMAND_IDENTIFYandCOMMAND_GET_STATEare assigned0x02. According to the Improv Wi-Fi serial protocol specification,IDENTIFY(0x02) is a BLE command, not a Serial command. For Serial protocol, 0x02 isGET_STATEand 0x04 isSCAN_NETWORKS.This issue was already identified in previous review comments. Please apply the suggested fix to replace
COMMAND_IDENTIFYwithCOMMAND_SCAN_NETWORKS = 0x04and update all references (lines 99-101, 167-173, 251-254).
90-97: Fix UTF-8 length bug: using character count instead of byte length.The function uses
len(ssid)andlen(password)which count Unicode characters, not UTF-8 bytes. For non-ASCII SSIDs or passwords, the length prefix will be incorrect, breaking the protocol framing.This issue was already identified in previous review comments. Encode to UTF-8 first, use byte lengths, and validate they fit in a single byte (≤255):
def wifi_payload(ssid: str, password: str) -> bytes: + ssid_b = ssid.encode("utf-8") + pwd_b = password.encode("utf-8") + if len(ssid_b) > 255 or len(pwd_b) > 255: + raise ValueError("SSID/password too long for Improv (max 255 bytes).") body = bytearray() - body.append(len(ssid)) - body.extend(ssid.encode()) - body.append(len(password)) - body.extend(password.encode()) + body.append(len(ssid_b)) + body.extend(ssid_b) + body.append(len(pwd_b)) + body.extend(pwd_b) return bytes(body)
140-152: Remove unused variable and add payload bounds validation.The
total_lenvariable is assigned but never used, and the function doesn't validate that enough bytes remain before reading each length-prefixed string. This can cause incorrect parsing or exceptions on malformed payloads.This issue was already identified in previous review comments. Add proper bounds checking:
def decode_rpc_result(payload: bytes) -> Tuple[int, list[str]] | None: if len(payload) < 2: return None command = payload[0] total_len = payload[1] idx = 2 values: list[str] = [] + # Validate that the payload has at least the declared number of bytes + if len(payload) - 2 < total_len: + return None + end = 2 + total_len - while idx < len(payload): + while idx < end: + if idx >= len(payload): + return None length = payload[idx] idx += 1 + if idx + length > len(payload): + return None values.append(payload[idx : idx + length].decode("utf-8", errors="replace")) idx += length return command, valuessrc/HeadlessWiFiSettings.cpp (2)
353-376: Provisioning outcome not reported before restart.The success path (lines 372-374) saves credentials and restarts immediately without calling
improv->handleConnectionResult(true)or signalingSTATE_PROVISIONED. The Improv serial protocol expects explicit confirmation; without it, the host tool waits indefinitely even if credentials are saved successfully.
365-365: Plaintext Wi‑Fi credentials written to SPIFFS.Credentials are stored as cleartext files via
spurt(). Anyone with flash read access can recover them. Consider using ESP32 NVS withWiFi.persistent(true)or encrypting at rest with a device-unique key.
🧹 Nitpick comments (1)
README.md (1)
193-197: Add language identifier to code block.The code block is missing a language identifier. Add
bashafter the opening backticks for proper syntax highlighting.-``` +```bash POST /wifi Content-Type: application/x-www-form-urlencoded
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)README.md(3 hunks)examples/ArduinoOTA/ArduinoOTA.ino(0 hunks)examples/SerialImprov/SerialImprov.ino(1 hunks)examples/SerialImprov/test_improv.py(1 hunks)src/HeadlessWiFiSettings.cpp(8 hunks)
💤 Files with no reviewable changes (1)
- examples/ArduinoOTA/ArduinoOTA.ino
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- examples/SerialImprov/SerialImprov.ino
🧰 Additional context used
🧬 Code graph analysis (1)
src/HeadlessWiFiSettings.cpp (1)
examples/SerialImprov/SerialImprov.ino (2)
loop(94-96)loop(94-94)
🪛 LanguageTool
README.md
[grammar] ~575-~575: Ensure spelling is correct
Context: ...liseconds until the next call (default: 100ms). ```C++ HeadlessWiFiSettings.onWaitLo...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
README.md
193-193: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.1)
examples/SerialImprov/test_improv.py
144-144: Local variable total_len is assigned to but never used
Remove assignment to unused variable total_len
(F841)
🔇 Additional comments (11)
README.md (5)
51-77: LGTM! Clear documentation for Serial Improv provisioning.The Serial Improv section provides a clear example of the setup/loop pattern and includes helpful instructions for testing with the Python helper script.
79-118: LGTM! Comprehensive WiFi configuration documentation.The section clearly explains portal mode, HTTP endpoints, and Serial Improv provisioning methods.
227-314: LGTM! Excellent comprehensive usage example.This example demonstrates all major features including Serial Improv, multiple endpoints, callbacks, various parameter types, and custom HTTP routes. The code structure is clear and educational.
394-488: LGTM! Thorough API reference.The parameter configuration functions are well-documented with clear signatures and usage notes. The note about calling before
.connect()or.portal()is particularly important.
525-631: LGTM! Complete callback documentation.All callbacks are documented with signatures and usage examples. The callbacks provide good hooks for customizing behavior at various stages.
examples/SerialImprov/test_improv.py (2)
180-213: LGTM! Clean I/O and state waiting logic.The
read_forandwait_for_provisionedfunctions properly handle non-blocking reads, timeouts, and state transitions. The error handling for provisioning failures is appropriate.
216-275: Main function structure is solid, but depends on fixing COMMAND_IDENTIFY.The CLI interface, argument parsing, environment variable fallbacks, and protocol flow are well-designed. However, the
identify_packet()call on line 252 uses the incorrectCOMMAND_IDENTIFYconstant that needs to be fixed as noted in earlier comments.Once the
COMMAND_IDENTIFY/COMMAND_SCAN_NETWORKSconstant is corrected, verify that the protocol flow still works as intended.src/HeadlessWiFiSettings.cpp (4)
24-24: Good refactoring for DRY compliance.Centralizing the error message in the
ERROR_FLASHconstant improves maintainability and consistency.Also applies to: 598-600
215-220: Defensive initialization ensures safe parameter access.The
ensureMainEndpoint()helper and its usage throughout parameter and endpoint lookup paths prevent potential out-of-bounds access when the endpoint vector is empty.Also applies to: 222-225, 228-240, 512-512
378-380: LGTM: Clean forwarding implementation.The
loop()method correctly delegates toimprov->loop()when the Improv instance is active.
646-646: Improv integration in wait loops enables serial provisioning.Calling
loop()during portal and connect wait loops allows the Improv protocol to process serial commands while the device is waiting, enabling serial-based provisioning workflows.Also applies to: 696-696
DTTerastar
force-pushed
the
codex/add-serialimprov-support-for-wifi-settings
branch
2 times, most recently
from
7829db3 to
512e8bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/HeadlessWiFiSettings.cpp (1)
308-331: Restart without connection attempt prevents Improv outcome reporting.The WiFi callback saves credentials and restarts immediately without attempting to connect. This prevents reporting the actual connection outcome (success/wrong password/network not found) to the Improv host, which expects explicit
STATE_PROVISIONEDor error signals.examples/SerialImprov/test_improv.py (3)
29-33: Duplicate command codes and incorrect Serial protocol command.Lines 30-31 both use
0x02, and the Serial Improv protocol doesn't define an IDENTIFY command (that's BLE-only). Command0x04is "Request scanned Wi-Fi networks" in the Serial spec.
90-97: Character count vs byte count breaks non-ASCII credentials.Lines 92 and 94 use
len(ssid)andlen(password)which count characters, not UTF-8 bytes. Non-ASCII SSIDs/passwords will have incorrect length prefixes, causing protocol parsing failures.
140-153: Unused variable and missing payload validation.Line 144's
total_lenis assigned but never used (Ruff F841). The function should validate thatlen(payload) - 2 >= total_lento prevent partial/garbled decoding.
🧹 Nitpick comments (2)
README.md (2)
193-198: Optional: Specify language for code block.The fenced code block lacks a language specifier. Adding
httportextwould improve rendering and satisfy markdown linters.-``` +```http POST /wifi Content-Type: application/x-www-form-urlencoded
474-489: Minor: Parameter name mismatch with implementation.Line 477 shows the parameter as
chip, but the implementation in HeadlessWiFiSettings.cpp line 308 names itchipFamily. Consider updating the documentation to match:-void startImprovSerial(String firmware, String version, String chip = ""); +void startImprovSerial(String firmware, String version, String chipFamily = "");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)README.md(3 hunks)examples/ArduinoOTA/ArduinoOTA.ino(0 hunks)examples/SerialImprov/SerialImprov.ino(1 hunks)examples/SerialImprov/test_improv.py(1 hunks)src/HeadlessWiFiSettings.cpp(13 hunks)
💤 Files with no reviewable changes (1)
- examples/ArduinoOTA/ArduinoOTA.ino
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/SerialImprov/SerialImprov.ino
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (1)
src/HeadlessWiFiSettings.cpp (2)
src/json_utils.h (1)
json_encode(8-31)examples/SerialImprov/SerialImprov.ino (2)
loop(94-96)loop(94-94)
🪛 LanguageTool
README.md
[grammar] ~575-~575: Ensure spelling is correct
Context: ...liseconds until the next call (default: 100ms). ```C++ HeadlessWiFiSettings.onWaitLo...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
README.md
193-193: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.1)
examples/SerialImprov/test_improv.py
144-144: Local variable total_len is assigned to but never used
Remove assignment to unused variable total_len
(F841)
🔇 Additional comments (11)
src/HeadlessWiFiSettings.cpp (8)
19-27: LGTM: Centralized constants improve maintainability.The centralized error messages and JSON template strings follow good practices. The
ensureMainEndpoint()forward declaration is appropriate for the later definition.
45-62: Good refactor: Consolidated JSON formatting helpers.The new
jsonStringandjsonNumerichelpers eliminate code duplication across parameter types and ensure consistent JSON formatting with proper escaping.
159-195: LGTM: Endpoint management helpers are well-designed.The
ensureMainEndpoint()guard prevents issues with empty endpoint lists, andfindEndpoint()provides a clean search API. Linear search is appropriate given the expected small number of endpoints.
333-335: LGTM: Loop delegation is correct.The
loop()method properly forwards toimprov->loop()with appropriate null-checking.
515-516: LGTM: Consistent error handling.The centralized
ERROR_FLASHconstant ensures consistent error messages across HTTP responses and serial logs.
563-563: LGTM: Improv integration in portal loop.Calling
loop()during the portal's main loop ensures Serial Improv messages are processed while the captive portal is active.
613-613: LGTM: Improv integration in connect retry loop.Calling
loop()during WiFi connection attempts ensures Serial Improv messages are processed during the potentially long connection timeout period.
454-460: Fix path parsing: leading slash breaks endpoint lookup.Line 454's
path.substring(6)extracts "/mqtt" from "/wifi/mqtt", butfindEndpoint()searches for "mqtt" (no leading slash). This causes all named endpoints to return 404.Apply this diff to strip the leading slash:
- String endpointName = (path.length() <= 6) ? "main" : path.substring(6); + String endpointName = (path.length() <= 6) ? "main" : path.substring(6).startsWith("/") ? path.substring(7) : path.substring(6);Or more cleanly:
- String endpointName = (path.length() <= 6) ? "main" : path.substring(6); + String endpointName; + if (path.length() <= 6) { + endpointName = "main"; + } else { + endpointName = path.substring(path.startsWith(F(WIFI_PATH)) ? 6 : 6); + if (endpointName.startsWith("/")) endpointName = endpointName.substring(1); + } int endpointIndex = findEndpoint(endpointName);Likely an incorrect or invalid review comment.
README.md (3)
51-78: LGTM: Clear Serial Improv documentation.The Serial Improv provisioning section clearly explains initialization, loop integration, and provides practical examples including the Python test script usage.
79-118: LGTM: Comprehensive WiFi configuration documentation.The section clearly explains portal mode, HTTP endpoint usage, and Serial Improv provisioning with practical curl examples.
227-314: LGTM: Excellent comprehensive example.This complete example effectively demonstrates the library's key features including callbacks, multiple endpoints, custom HTTP routes, and Serial Improv integration.
DTTerastar
force-pushed
the
codex/add-serialimprov-support-for-wifi-settings
branch
from
512e8bf to
bf9a296
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
src/HeadlessWiFiSettings.cpp (4)
311-334: Critical: Improv provisioning never reports connection outcome.The WiFi callback saves credentials to SPIFFS and immediately restarts (line 332) without attempting to connect or reporting success/failure back to the Improv host tool. According to the Improv serial protocol specification, the device must signal
STATE_PROVISIONEDor an error after the connection attempt completes.Current flow:
- Receives credentials via Improv
- Saves to SPIFFS
- Restarts immediately ← Problem: no connection attempt, no result reported
Expected flow:
- Receives credentials
- Saves to SPIFFS
- Attempts connection (
connect(false))- Reports outcome via
improv->handleConnectionResult(success)- Then restart (or stay running)
This causes the host provisioning tool to time out waiting for confirmation, even if the connection will succeed after restart.
Consider attempting the connection before restart and reporting the outcome:
improv->setWiFiCallback([this](const char* ssid, const char* password) { bool ok = spurt("/wifi-ssid", ssid) && spurt("/wifi-password", password); if (!ok) { Serial.println(ERROR_FLASH); if (onFailure) onFailure(); if (improv) improv->handleConnectionResult(false); return; } if (onConfigSaved) onConfigSaved(); - if (onRestart) onRestart(); - ESP.restart(); + + // Attempt connection and report outcome + WiFi.begin(ssid, password); + unsigned long start = millis(); + while (WiFi.status() != WL_CONNECTED && millis() - start < 30000) { + delay(100); + } + bool connected = (WiFi.status() == WL_CONNECTED); + if (improv) improv->handleConnectionResult(connected); + + if (connected && onSuccess) onSuccess(); + else if (!connected && onFailure) onFailure(); + + if (onRestart) onRestart(); + ESP.restart(); });
457-463: Critical: Path parsing bug breaks named endpoint GET requests.Line 457's
substring(6)extracts the endpoint name but fails to strip the leading slash for paths like/wifi/mqtt, producingendpointName = "/mqtt"instead of"mqtt". This causesfindEndpointto return -1 and the request fails with 404.Example:
- Request:
GET /wifi/mqtt- Current:
path.substring(6)→"/mqtt"→findEndpoint("/mqtt")→ -1 → 404 ❌- Expected:
"mqtt"→findEndpoint("mqtt")→ valid index → 200 ✓Apply this diff:
- String endpointName = (path.length() <= 6) ? "main" : path.substring(6); + String endpointName; + if (path.length() <= 6) { + endpointName = "main"; + } else { + endpointName = path.substring(6); + if (endpointName.startsWith("/")) endpointName = endpointName.substring(1); + } int endpointIndex = findEndpoint(endpointName);
500-506: Critical: Identical path parsing bug in POST handler.Line 500 has the same
substring(6)issue as the GET handler (line 457), causing POST requests to named endpoints like/wifi/mqttto fail with 404.Apply the same fix:
- String endpointName = (path.length() <= 6) ? "main" : path.substring(6); + String endpointName; + if (path.length() <= 6) { + endpointName = "main"; + } else { + endpointName = path.substring(6); + if (endpointName.startsWith("/")) endpointName = endpointName.substring(1); + } int endpointIndex = findEndpoint(endpointName);
587-588: Major security issue: WiFi credentials stored as plaintext in SPIFFS.Lines 323, 587-588 write SSID and password as cleartext files to SPIFFS (
/wifi-ssid,/wifi-password). Anyone with physical access to the device or flash read capability can extract the credentials. This is a security posture gap.Consider these alternatives:
- Preferred: Use ESP32 NVS by calling
WiFi.persistent(true)beforeWiFi.begin(), letting the SDK store credentials securely- Encrypt credentials at rest using a device-unique key (derived from eFuse/MAC) with authenticated encryption (AES-GCM)
The HTTP endpoints already mask password values (
***###***) correctly, which is good. Ensure plaintext is never exposed via any interface.examples/SerialImprov/test_improv.py (3)
29-32: Critical: Invalid duplicate command codes (0x02) and wrong protocol.Lines 30-31 assign the same value
0x02to bothCOMMAND_IDENTIFYandCOMMAND_GET_STATE. Additionally, according to the Improv Wi-Fi serial protocol specification,IDENTIFY(0x02) is a BLE RPC command, not a Serial command. The Serial protocol defines command0x04as "Request scanned Wi-Fi networks" (SCAN_NETWORKS), not IDENTIFY.Current (incorrect):
COMMAND_IDENTIFY = 0x02← BLE protocol, wrong for SerialCOMMAND_GET_STATE = 0x02← Correct for Serial, but duplicateBased on learnings from past reviews, apply these fixes:
COMMAND_WIFI_SETTINGS = 0x01 -COMMAND_IDENTIFY = 0x02 COMMAND_GET_STATE = 0x02 COMMAND_GET_DEVICE_INFO = 0x03 +COMMAND_SCAN_NETWORKS = 0x04Also update the helper function (lines 99-100), print handling (lines 170-171), and call site (lines 251-253):
-def identify_packet() -> bytes: - return build_rpc(COMMAND_IDENTIFY, b"") +def scan_networks_packet() -> bytes: + return build_rpc(COMMAND_SCAN_NETWORKS, b"")- elif command == COMMAND_IDENTIFY: - print(f"Identify RPC response: {values}") + elif command == COMMAND_SCAN_NETWORKS: + print(f"Scan networks result: {values}")- print("Sending IDENTIFY") - ser.write(identify_packet()) + print("Requesting Wi‑Fi scan") + ser.write(scan_networks_packet()) read_for(ser, 1.0, buffer)
90-96: Critical: UTF-8 length bug breaks non-ASCII WiFi credentials.The
wifi_payloadfunction useslen(ssid)andlen(password)which return character counts, not byte lengths. The Improv protocol requires byte-length prefixes for UTF-8 encoded strings. This breaks any SSID or password containing non-ASCII characters (e.g., "Café" would be incorrectly prefixed).Example failure:
- SSID:
"Café"(4 characters, 5 UTF-8 bytes)- Current: sends length byte
0x04, then 5 bytes → decoder reads 4 bytes, leaves 1 orphaned- Correct: send length byte
0x05, then 5 bytesApply this fix:
def wifi_payload(ssid: str, password: str) -> bytes: + ssid_b = ssid.encode("utf-8") + pwd_b = password.encode("utf-8") + if len(ssid_b) > 255 or len(pwd_b) > 255: + raise ValueError("SSID/password too long for Improv (max 255 bytes).") body = bytearray() - body.append(len(ssid)) - body.extend(ssid.encode()) - body.append(len(password)) - body.extend(password.encode()) + body.append(len(ssid_b)) + body.extend(ssid_b) + body.append(len(pwd_b)) + body.extend(pwd_b) return bytes(body)
140-152: Minor: Unused variable and missing payload validation.Line 144 assigns
total_lenbut never uses it (flagged by Ruff F841). Additionally, the function doesn't validate that the payload actually contains the declared number of bytes, which could lead to partial/garbled decodes.Apply this fix to validate payload length and remove the unused variable:
def decode_rpc_result(payload: bytes) -> Tuple[int, list[str]] | None: if len(payload) < 2: return None command = payload[0] total_len = payload[1] + # Validate that payload has the declared bytes + if len(payload) - 2 < total_len: + return None idx = 2 + end = 2 + total_len values: list[str] = [] - while idx < len(payload): + while idx < end: + if idx >= len(payload): + return None length = payload[idx] idx += 1 + if idx + length > len(payload): + return None values.append(payload[idx : idx + length].decode("utf-8", errors="replace")) idx += length return command, values
🧹 Nitpick comments (1)
README.md (1)
193-198: Add language identifier to fenced code block.The fenced code block is missing a language identifier, which helps with syntax highlighting and accessibility.
Apply this diff:
-``` +```bash POST /wifi Content-Type: application/x-www-form-urlencoded
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)README.md(3 hunks)examples/ArduinoOTA/ArduinoOTA.ino(0 hunks)examples/SerialImprov/SerialImprov.ino(1 hunks)examples/SerialImprov/test_improv.py(1 hunks)src/HeadlessWiFiSettings.cpp(18 hunks)
💤 Files with no reviewable changes (1)
- examples/ArduinoOTA/ArduinoOTA.ino
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- examples/SerialImprov/SerialImprov.ino
🧰 Additional context used
🧬 Code graph analysis (1)
src/HeadlessWiFiSettings.cpp (2)
src/json_utils.h (1)
json_encode(8-31)examples/SerialImprov/SerialImprov.ino (2)
loop(94-96)loop(94-94)
🪛 LanguageTool
README.md
[grammar] ~575-~575: Ensure spelling is correct
Context: ...liseconds until the next call (default: 100ms). ```C++ HeadlessWiFiSettings.onWaitLo...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
README.md
193-193: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.1)
examples/SerialImprov/test_improv.py
144-144: Local variable total_len is assigned to but never used
Remove assignment to unused variable total_len
(F841)
🔇 Additional comments (4)
README.md (2)
51-78: LGTM! Clear Serial Improv documentation.The new Serial Improv provisioning section provides clear examples and usage instructions that align well with the implementation in
src/HeadlessWiFiSettings.cppand the example sketch.
227-346: Comprehensive API documentation.The complete usage example and API testing section provide excellent coverage of the library's capabilities, including callbacks, multiple endpoints, and Serial Improv integration.
src/HeadlessWiFiSettings.cpp (1)
336-338: LGTM: Clean loop delegation.The
loop()implementation correctly delegates toimprov->loop()when the Improv instance is active, integrating Serial Improv processing into the application's main loop.examples/SerialImprov/test_improv.py (1)
1-10: Good documentation in module docstring.The module docstring clearly explains the script's purpose and usage, including the optional SSID/PASSWORD arguments.
DTTerastar
force-pushed
the
codex/add-serialimprov-support-for-wifi-settings
branch
from
bf9a296 to
15fc0ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/HeadlessWiFiSettings.cpp (2)
624-638: Also signal failure on reconnect path when pending credentials exist.
If the reconnect after reboot fails, notify Improv and clear the flag.if (status != WL_CONNECTED) { Serial.printf(" failed (status=%d).\n", status); + if (ESPFS.exists(PENDING_WIFI_FILE)) { + ESPFS.remove(PENDING_WIFI_FILE); + if (improv) improv->handleConnectionResult(false); + } if (onFailure) onFailure(); if (portal) this->portal(); return false; } @@ - if (ESPFS.exists(PENDING_WIFI_FILE)) { - ESPFS.remove(PENDING_WIFI_FILE); - if (improv) improv->handleConnectionResult(true); - } + if (ESPFS.exists(PENDING_WIFI_FILE)) { + ESPFS.remove(PENDING_WIFI_FILE); + if (improv) improv->handleConnectionResult(true); + }
646-650: Implement destructor to free ImprovWiFi.
Completes the header change and prevents leaks.HeadlessWiFiSettingsClass::HeadlessWiFiSettingsClass() : http(80) { hostname = F("esp32-"); } +HeadlessWiFiSettingsClass::~HeadlessWiFiSettingsClass() { + if (improv) { + delete improv; + improv = nullptr; + } +} + HeadlessWiFiSettingsClass HeadlessWiFiSettings;
♻️ Duplicate comments (7)
examples/SerialImprov/test_improv.py (5)
29-33: Serial spec mismatch: use SCAN_NETWORKS (0x04), remove IDENTIFY, fix duplicate 0x02.IDENTIFY isn’t a Serial RPC; 0x02 is GET_STATE. Replace IDENTIFY with SCAN_NETWORKS=0x04 and update usages.
Apply:
-COMMAND_WIFI_SETTINGS = 0x01 -COMMAND_IDENTIFY = 0x02 -COMMAND_GET_STATE = 0x02 -COMMAND_GET_DEVICE_INFO = 0x03 +COMMAND_WIFI_SETTINGS = 0x01 +COMMAND_GET_STATE = 0x02 +COMMAND_GET_DEVICE_INFO = 0x03 +COMMAND_SCAN_NETWORKS = 0x04
90-96: Fix UTF‑8 length bug in wifi_payload (use byte lengths).
Using len() on Python strings breaks for non‑ASCII SSIDs/passwords.-def wifi_payload(ssid: str, password: str) -> bytes: - body = bytearray() - body.append(len(ssid)) - body.extend(ssid.encode()) - body.append(len(password)) - body.extend(password.encode()) - return bytes(body) +def wifi_payload(ssid: str, password: str) -> bytes: + ssid_b = ssid.encode("utf-8") + pwd_b = password.encode("utf-8") + if len(ssid_b) > 255 or len(pwd_b) > 255: + raise ValueError("SSID/password too long for Improv (max 255 bytes).") + body = bytearray() + body.append(len(ssid_b)) + body.extend(ssid_b) + body.append(len(pwd_b)) + body.extend(pwd_b) + return bytes(body)
99-101: Rename IDENTIFY to SCAN_NETWORKS across helpers, prints, and CLI text.
Align with Serial RPCs and avoid conflicting 0x02.-def identify_packet() -> bytes: - return build_rpc(COMMAND_IDENTIFY, b"") +def scan_networks_packet() -> bytes: + return build_rpc(COMMAND_SCAN_NETWORKS, b"") @@ - elif command == COMMAND_IDENTIFY: - print(f"Identify RPC response: {values}") + elif command == COMMAND_SCAN_NETWORKS: + print(f"Scan networks result: {values}") @@ - print("Sending IDENTIFY") - ser.write(identify_packet()) + print("Requesting Wi‑Fi scan") + ser.write(scan_networks_packet()) @@ -If SSID/PASSWORD are omitted the script only issues IDENTIFY/GET_STATE and +If SSID/PASSWORD are omitted the script only issues SCAN_NETWORKS/GET_STATE and prints any responses.Also applies to: 167-173, 251-254, 7-9
140-152: Validate length-prefixed strings in decode_rpc_result and use total_len.
Avoid overreads and silence Ruff F841.def decode_rpc_result(payload: bytes) -> Tuple[int, list[str]] | None: if len(payload) < 2: return None command = payload[0] - total_len = payload[1] + total_len = payload[1] idx = 2 values: list[str] = [] - while idx < len(payload): - length = payload[idx] - idx += 1 - values.append(payload[idx : idx + length].decode("utf-8", errors="replace")) - idx += length + if len(payload) - 2 < total_len: + return None + end = 2 + total_len + while idx < end: + if idx >= len(payload): + return None + length = payload[idx] + idx += 1 + if idx + length > len(payload): + return None + values.append(payload[idx: idx + length].decode("utf-8", errors="replace")) + idx += length return command, values
74-82: Guard against payloads >255 bytes in build_frame.
Length is a single byte; validate before append.def build_frame(packet_type: int, payload: bytes) -> bytes: frame = bytearray(IMPROV_HEADER) frame.append(IMPROV_SERIAL_VERSION) frame.append(packet_type) - frame.append(len(payload)) + if len(payload) > 255: + raise ValueError("Payload too large for SerialImprov frame (max 255 bytes).") + frame.append(len(payload)) frame.extend(payload) frame.append(checksum(frame)) return bytes(frame)src/HeadlessWiFiSettings.h (1)
11-25: Add destructor to free ImprovWiFi.
Avoid leaking the dynamically-allocated improv instance.class HeadlessWiFiSettingsClass { public: @@ - void loop(); + void loop(); + ~HeadlessWiFiSettingsClass(); @@ private: AsyncWebServer http; bool begun = false; bool httpBegun = false; ImprovWiFi *improv = nullptr;Also applies to: 52-60
src/HeadlessWiFiSettings.cpp (1)
312-337: Report provisioning success/failure to Improv and avoid immediate blind restart.
Currently you restart without signaling outcome; hosts time out. Attempt connect, call handleConnectionResult, then restart on success.improv->setWiFiCallback([this](const char* ssid, const char* password) { - bool ok = spurt("/wifi-ssid", ssid) && spurt("/wifi-password", password); - if (!ok) { + bool ok = spurt("/wifi-ssid", ssid) && spurt("/wifi-password", password); + if (!ok) { Serial.println(ERROR_FLASH); if (onFailure) onFailure(); if (improv) improv->handleConnectionResult(false); return; } - spurt(PENDING_WIFI_FILE, "1"); - if (onConfigSaved) onConfigSaved(); - if (onRestart) onRestart(); - ESP.restart(); + spurt(PENDING_WIFI_FILE, "1"); + if (onConfigSaved) onConfigSaved(); + // Attempt a bounded connect without entering portal + bool success = this->connect(false, 30); + if (improv) improv->handleConnectionResult(success); + if (success) { + ESPFS.remove(PENDING_WIFI_FILE); + if (onRestart) onRestart(); + ESP.restart(); + } else { + if (onFailure) onFailure(); + // Stay running to allow another attempt via Improv + } });
🧹 Nitpick comments (1)
README.md (1)
193-199: Add a language to the fenced block (markdownlint MD040).
Use http for request examples.-``` +```http POST /wifi Content-Type: application/x-www-form-urlencoded wifi-ssid=MyNetwork&wifi-password=MyPassword&server_host=api.example.com&server_port=8080</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between bf9a296cdf3fc076bfae647330bbb5f4a410ce29 and 15fc0ea63f68f31d9672d6ed554bac672f2a5a73. </details> <details> <summary>📒 Files selected for processing (7)</summary> * `.gitignore` (1 hunks) * `README.md` (3 hunks) * `examples/ArduinoOTA/ArduinoOTA.ino` (0 hunks) * `examples/SerialImprov/SerialImprov.ino` (1 hunks) * `examples/SerialImprov/test_improv.py` (1 hunks) * `src/HeadlessWiFiSettings.cpp` (19 hunks) * `src/HeadlessWiFiSettings.h` (1 hunks) </details> <details> <summary>💤 Files with no reviewable changes (1)</summary> * examples/ArduinoOTA/ArduinoOTA.ino </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary> * .gitignore * examples/SerialImprov/SerialImprov.ino </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (3)</summary> <details> <summary>src/HeadlessWiFiSettings.cpp (2)</summary><blockquote> <details> <summary>src/json_utils.h (1)</summary> * `json_encode` (8-31) </details> <details> <summary>examples/SerialImprov/SerialImprov.ino (2)</summary> * `loop` (94-96) * `loop` (94-94) </details> </blockquote></details> <details> <summary>src/HeadlessWiFiSettings.h (2)</summary><blockquote> <details> <summary>src/HeadlessWiFiSettings.cpp (23)</summary> * `HeadlessWiFiSettingsClass` (646-648) * `name` (91-91) * `name` (93-93) * `begin` (640-644) * `begin` (640-640) * `connect` (578-638) * `connect` (578-578) * `portal` (537-576) * `portal` (537-537) * `httpSetup` (343-535) * `httpSetup` (343-343) * `startImprovSerial` (312-337) * `startImprovSerial` (312-312) * `loop` (339-341) * `loop` (339-339) * `string` (214-224) * `string` (214-214) * `string` (226-230) * `string` (226-226) * `string` (232-237) * `string` (232-232) * `dropdown` (239-250) * `dropdown` (239-239) </details> <details> <summary>examples/SerialImprov/SerialImprov.ino (2)</summary> * `loop` (94-96) * `loop` (94-94) </details> </blockquote></details> <details> <summary>examples/SerialImprov/test_improv.py (1)</summary><blockquote> <details> <summary>src/HeadlessWiFiSettings.cpp (3)</summary> * `value` (130-130) * `name` (91-91) * `name` (93-93) </details> </blockquote></details> </details><details> <summary>🪛 LanguageTool</summary> <details> <summary>README.md</summary> [grammar] ~575-~575: Ensure spelling is correct Context: ...liseconds until the next call (default: 100ms). ```C++ HeadlessWiFiSettings.onWaitLo... (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>README.md</summary> 193-193: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> <details> <summary>🪛 Ruff (0.14.1)</summary> <details> <summary>examples/SerialImprov/test_improv.py</summary> 144-144: Local variable `total_len` is assigned to but never used Remove assignment to unused variable `total_len` (F841) </details> </details> </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| String endpointName = (path.length() <= 6) ? "main" : path.substring(6); | ||
| int endpointIndex = findEndpoint(endpointName); | ||
|
|
||
| if (!found) { | ||
| request->send(404, "text/plain", "Endpoint not found"); | ||
| if (endpointIndex < 0) { | ||
| request->send(404, CONTENT_TEXT, ENDPOINT_NOT_FOUND); | ||
| return; |
There was a problem hiding this comment.
Normalize endpoint path parsing to handle double slashes.
substring(6) yields a leading “/” for paths like “/wifi//mqtt”; strip it.
- String endpointName = (path.length() <= 6) ? "main" : path.substring(6);
+ String endpointName = (path.length() <= 6) ? "main" : path.substring(6);
+ if (endpointName.startsWith("/")) endpointName = endpointName.substring(1);
@@
- String endpointName = (path.length() <= 6) ? "main" : path.substring(6);
+ String endpointName = (path.length() <= 6) ? "main" : path.substring(6);
+ if (endpointName.startsWith("/")) endpointName = endpointName.substring(1);Also applies to: 503-508
🤖 Prompt for AI Agents
In src/HeadlessWiFiSettings.cpp around lines 460-465 and 503-508, endpointName
is derived with path.substring(6) which can start with a leading '/' for inputs
like "/wifi//mqtt"; normalize by trimming a leading slash after substring (e.g.,
if endpointName.length() && endpointName.charAt(0)=='/' then endpointName =
endpointName.substring(1)) so endpoint lookups no longer fail due to an extra
slash.
15fc0ea to
9728951
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
README.md (1)
76-77: Update wording: Serial Improv uses SCAN_NETWORKS, not IDENTIFY.The Serial protocol specification doesn't include an IDENTIFY RPC. The command at 0x04 is "Request scanned Wi-Fi networks" (SCAN_NETWORKS), not IDENTIFY.
Apply this diff:
-Leave the SSID/password arguments off if you only want to send IDENTIFY/GET -STATE RPCs. +Leave the SSID/password arguments off to send SCAN_NETWORKS and GET_STATE RPCs.src/HeadlessWiFiSettings.cpp (1)
325-325: Plaintext WiFi credentials stored in SPIFFS.Credentials are written as cleartext files to SPIFFS. Anyone with physical access or flash dump can recover SSID and password. This is a major security posture gap that has been flagged in multiple previous reviews.
Consider:
- Use ESP32 NVS with
WiFi.persistent(true)to let the SDK store credentials securely, or- Encrypt at rest using a device-unique key (eFuse MAC-derived) with AES-GCM
Note: HTTP endpoints already mask password values (good), but storage remains vulnerable.
Also applies to: 590-591
examples/SerialImprov/test_improv.py (4)
29-32: Fix duplicate command code: use SCAN_NETWORKS (0x04) instead of IDENTIFY.Both
COMMAND_IDENTIFYandCOMMAND_GET_STATEare set to0x02, which is incorrect. According to the Improv Wi-Fi Serial protocol specification:
- 0x02 = "Request current state" (GET_STATE) ✓
- 0x03 = "Request device information" (GET_DEVICE_INFO) ✓
- 0x04 = "Request scanned Wi-Fi networks" (SCAN_NETWORKS)
The Serial protocol has no IDENTIFY command; that's a BLE-only RPC. Replace
COMMAND_IDENTIFYwithCOMMAND_SCAN_NETWORKS = 0x04.Apply this diff:
COMMAND_WIFI_SETTINGS = 0x01 -COMMAND_IDENTIFY = 0x02 COMMAND_GET_STATE = 0x02 COMMAND_GET_DEVICE_INFO = 0x03 +COMMAND_SCAN_NETWORKS = 0x04Also update lines 99-101, 170-171, and 259-260 to use
COMMAND_SCAN_NETWORKS.
90-96: Fix UTF-8 length bug: use byte length, not character count.The
len(ssid)andlen(password)calls count characters, not bytes. This breaks for non-ASCII SSIDs or passwords. For example, "Café" is 4 characters but 5 UTF-8 bytes. The Improv protocol requires byte lengths.Apply this diff:
def wifi_payload(ssid: str, password: str) -> bytes: + ssid_b = ssid.encode("utf-8") + pwd_b = password.encode("utf-8") + if len(ssid_b) > 255 or len(pwd_b) > 255: + raise ValueError("SSID/password too long (max 255 bytes each)") body = bytearray() - body.append(len(ssid)) - body.extend(ssid.encode()) - body.append(len(password)) - body.extend(password.encode()) + body.append(len(ssid_b)) + body.extend(ssid_b) + body.append(len(pwd_b)) + body.extend(pwd_b) return bytes(body)
140-152: Remove unused variable and validate payload length.
total_len(line 144) is assigned but never used, triggering Ruff F841. Additionally, the function doesn't validate that the payload actually contains the declared number of bytes, which can cause partial/garbled decodes.Apply this diff:
def decode_rpc_result(payload: bytes) -> Tuple[int, list[str]] | None: if len(payload) < 2: return None command = payload[0] - total_len = payload[1] + declared_len = payload[1] idx = 2 values: list[str] = [] + end = 2 + declared_len + if len(payload) < end: + return None # Payload shorter than declared - while idx < len(payload): + while idx < end: + if idx >= len(payload): + return None length = payload[idx] idx += 1 + if idx + length > len(payload): + return None values.append(payload[idx : idx + length].decode("utf-8", errors="replace")) idx += length return command, values
99-101: Rename function to match SCAN_NETWORKS command.Once you replace
COMMAND_IDENTIFYwithCOMMAND_SCAN_NETWORKS(see earlier comment), rename this helper for consistency.Apply this diff:
-def identify_packet() -> bytes: - return build_rpc(COMMAND_IDENTIFY, b"") +def scan_networks_packet() -> bytes: + return build_rpc(COMMAND_SCAN_NETWORKS, b"")Also update the call site at line 260 and the response handler at lines 170-171.
🧹 Nitpick comments (1)
README.md (1)
193-193: Add language specifier to fenced code block.The fenced code block is missing a language identifier, which helps with syntax highlighting and tooling.
Apply this diff:
-``` +```bash POST /wifi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignore(1 hunks)README.md(3 hunks)examples/ArduinoOTA/ArduinoOTA.ino(0 hunks)examples/SerialImprov/SerialImprov.ino(1 hunks)examples/SerialImprov/test_improv.py(1 hunks)src/HeadlessWiFiSettings.cpp(19 hunks)src/HeadlessWiFiSettings.h(1 hunks)
💤 Files with no reviewable changes (1)
- examples/ArduinoOTA/ArduinoOTA.ino
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/SerialImprov/SerialImprov.ino
- src/HeadlessWiFiSettings.h
🧰 Additional context used
🧬 Code graph analysis (1)
src/HeadlessWiFiSettings.cpp (2)
src/json_utils.h (1)
json_encode(8-31)examples/SerialImprov/SerialImprov.ino (2)
loop(94-96)loop(94-94)
🪛 LanguageTool
README.md
[grammar] ~575-~575: Ensure spelling is correct
Context: ...liseconds until the next call (default: 100ms). ```C++ HeadlessWiFiSettings.onWaitLo...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
README.md
193-193: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.1)
examples/SerialImprov/test_improv.py
144-144: Local variable total_len is assigned to but never used
Remove assignment to unused variable total_len
(F841)
195-195: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (9)
.gitignore (1)
10-10: LGTM! Good security practice.Adding
.envto.gitignoreprevents environment files containing credentials from being committed to version control.src/HeadlessWiFiSettings.cpp (7)
20-28: LGTM! Consistent error messaging.The centralized string constants improve maintainability and ensure consistent error messages across the codebase.
49-65: LGTM! DRY JSON formatting.The
jsonStringandjsonNumerichelpers eliminate code duplication and provide consistent JSON output formatting across all parameter types.
163-199: LGTM! Robust endpoint management.The
ensureMainEndpointpattern guarantees a "main" endpoint exists before any operations, andfindEndpointcorrectly returns -1 for not-found cases, enabling proper 404 responses.
312-337: Verify error handling and restart flow.The implementation stores credentials, sets a pending flag, and restarts immediately. The connection result is reported after reboot (lines 632-635), which means:
- The Improv client must wait through a device restart
- If the device fails to connect post-restart, the error won't be reported via Improv
Ensure this deferred-confirmation flow is acceptable for your use case and that clients handle the restart gracefully.
Additionally, verify that the ImprovWiFi library properly handles the state transitions during the restart sequence.
460-465: Verify path parsing handles malformed URLs.The
substring(6)extracts everything after "/wifi/", which works for "/wifi/mqtt" → "mqtt". However, verify that malformed paths like "/wifi//mqtt" or "/wifi/" are handled correctly. Previous reviews suggested normalizing by stripping any leading slash from the extracted endpoint name.If needed, apply this defensive check:
String endpointName = (path.length() <= 6) ? "main" : path.substring(6); +if (endpointName.startsWith("/")) endpointName = endpointName.substring(1); int endpointIndex = findEndpoint(endpointName);Also applies to: 503-508
568-568: LGTM! Improv protocol processed during portal.Calling
loop()within the portal ensures Serial Improv messages are handled even when the device is in AP mode.
632-635: LGTM! Deferred Improv confirmation after successful connect.Removing the pending-wifi marker and notifying Improv on successful connection completes the provisioning flow. This deferred-confirmation approach works around the immediate restart in
startImprovSerial.examples/SerialImprov/test_improv.py (1)
195-195: Acceptable use of broad exception handler for display fallback.Catching
Exceptionat line 195 is necessary here because the code attempts to display arbitrary serial data and needs a robust fallback when decoding fails. This is an acceptable use case for a broad exception handler.
1 participant
Summary
Testing
pio test -e nativehttps://chatgpt.com/codex/tasks/task_e_68ab5d99a0888324b2bf4aafd24dbca5
Summary by CodeRabbit
New Features
Documentation
Examples / Tests
Dependency
Chores
Removed