feat: support current CrossPoint host APIs - #26
Conversation
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (23)
📝 WalkthroughWalkthroughThe simulator adds host-compatible Arduino, networking, clock, HTTP, crypto, heap, randomness, and OTA shims. PlatformIO settings are updated, and a host self-test validates clock synchronization, HTTP handling, and SHA-256 behavior. ChangesHost compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant esp_http_client
participant SimHttpFetch
participant curl
esp_http_client->>SimHttpFetch: fetch URL with timeout
SimHttpFetch->>curl: execute request and capture body and headers
curl-->>SimHttpFetch: response body, headers, and status
SimHttpFetch-->>esp_http_client: parsed response
esp_http_client->>esp_http_client: emit header/data events and update completion state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
tests/host_compat_self_test.cpp (1)
5-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the self-test immune to
NDEBUG.Every check here is an
assert, so the whole test silently passes if it is ever compiled with-DNDEBUG(e.g. someone adds release flags to the runner or reuses this file in a PlatformIO env). Forcing asserts on keeps it self-contained.♻️ Proposed change
`#include` <array> +#undef NDEBUG `#include` <cassert>🤖 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/host_compat_self_test.cpp` around lines 5 - 11, Make tests/host_compat_self_test.cpp immune to NDEBUG by ensuring assertions remain enabled within this self-test translation unit, while preserving the existing assert-based checks and keeping the change self-contained.sample-platformio-macos.ini (1)
32-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding
-Wno-deprecated-declarationson macOS.The new
src/mbedtls/sha256.hcallsCC_SHA256_*, which Apple has deprecated, so simulator builds will emit warnings for every translation unit that includes it.tests/run_host_compat_self_test.shalready passes this flag.🔧 Proposed change
-Wno-c++11-narrowing + -Wno-deprecated-declarations -DSIMULATOR -DCROSSPOINT_EMULATED=1🤖 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 `@sample-platformio-macos.ini` around lines 32 - 34, Add -Wno-deprecated-declarations to the macOS simulator compiler flags alongside -Wno-c++11-narrowing in sample-platformio-macos.ini, matching the existing configuration used by tests/run_host_compat_self_test.sh.tests/run_host_compat_self_test.sh (1)
8-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse an array for link flags to keep shellcheck quiet and splitting explicit.
Unquoted
$link_flags(SC2086) relies on word splitting and also breaks ifTMPDIR/LDFLAGScontain spaces.♻️ Proposed change
-link_flags="${LDFLAGS:-}" -if [[ "$(uname -s)" == "Linux" ]]; then - link_flags="$link_flags -lcrypto" -fi +read -r -a link_flags <<<"${LDFLAGS:-}" +if [[ "$(uname -s)" == "Linux" ]]; then + link_flags+=(-lcrypto) +fi "${CXX:-c++}" -std=gnu++20 -Wno-deprecated-declarations \ -I"$repo_root/src" \ "$repo_root/tests/host_compat_self_test.cpp" \ "$repo_root/src/HalClock.cpp" \ - $link_flags \ + "${link_flags[@]}" \ -o "$test_binary"🤖 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/run_host_compat_self_test.sh` around lines 8 - 18, Update the link_flags handling in the host compatibility build script to use a shell array, appending the Linux crypto flag as an element and expanding the array explicitly in the compiler invocation. Preserve support for multiple flags and values containing spaces from LDFLAGS while eliminating the unquoted link_flags expansion.Source: Linters/SAST tools
🤖 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 `@sample-platformio-linux-wsl.ini`:
- Around line 33-34: Restore the Linux/WSL linker flags in the sample
configuration so the OpenSSL linkage includes -lssl, -lcrypto, and
-Wno-deprecated-declarations in the required order.
In `@src/Arduino.h`:
- Around line 54-57: Update both random overloads in Arduino.h to use an
ordered, range-capable uniform distribution rather than std::rand() modulo
arithmetic, supporting ranges larger than RAND_MAX without bias. Preserve the
existing return behavior for non-positive max and ensure the two-argument
overload handles min >= max before computing max - min, consistently returning
min.
In `@src/esp_http_client.h`:
- Around line 172-177: Update the failed-transfer path in the handle perform
logic to also clear responseBody, reset bodyOffset, and reset contentLength
alongside performed, complete, and statusCode. Ensure a reused handle exposes no
prior response bytes or length after perform() fails.
In `@src/esp_random.h`:
- Around line 7-17: Replace the MT19937-backed implementations in
simulatorRandomEngine, esp_random, and esp_fill_random with an operating-system
cryptographically secure random source such as getrandom, arc4random_buf, or
OpenSSL RAND_bytes. Ensure both APIs use that source for all output and handle
failures appropriately; do not retain MT19937 or document these APIs as suitable
for secrets, nonces, or key material unless cryptographic security is actually
provided.
In `@src/HalClock.cpp`:
- Around line 149-154: Update formatDate’s timestamp validation to check the raw
nowUtc() result before applying offsetQuarterHours, matching the trust gating
used by getTime/getDateTime. Return false when the unadjusted clock value is
untrusted, and only compute the offset timestamp and call toUtc for trusted
values.
In `@src/IPAddress.h`:
- Around line 24-25: Update both const and non-const IPAddress::operator[]
overloads to reject indices outside the valid 0–3 range instead of applying
modulo wrapping. Use the firmware API’s established bounds-checking or
invalid-index behavior, while preserving direct access for valid indices.
- Around line 17-21: Update IPAddress::toString so each bytes_ octet passed to
snprintf is explicitly converted with static_cast<unsigned>, matching the %u
format specifier for all four arguments.
---
Nitpick comments:
In `@sample-platformio-macos.ini`:
- Around line 32-34: Add -Wno-deprecated-declarations to the macOS simulator
compiler flags alongside -Wno-c++11-narrowing in sample-platformio-macos.ini,
matching the existing configuration used by tests/run_host_compat_self_test.sh.
In `@tests/host_compat_self_test.cpp`:
- Around line 5-11: Make tests/host_compat_self_test.cpp immune to NDEBUG by
ensuring assertions remain enabled within this self-test translation unit, while
preserving the existing assert-based checks and keeping the change
self-contained.
In `@tests/run_host_compat_self_test.sh`:
- Around line 8-18: Update the link_flags handling in the host compatibility
build script to use a shell array, appending the Linux crypto flag as an element
and expanding the array explicitly in the compiler invocation. Preserve support
for multiple flags and values containing spaces from LDFLAGS while eliminating
the unquoted link_flags expansion.
🪄 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: 7bc76725-530f-474e-b1b0-141a00600294
📒 Files selected for processing (22)
README.mdrun_simulator.pysample-platformio-linux-wsl.inisample-platformio-macos.inisrc/Arduino.hsrc/Client.hsrc/HalClock.cppsrc/HalClock.hsrc/HalOtaSlot.cppsrc/HalOtaSlot.hsrc/IPAddress.hsrc/NetworkClient.cppsrc/NetworkClient.hsrc/SimHttpFetch.hsrc/WiFi.hsrc/esp_heap_caps.hsrc/esp_http_client.hsrc/esp_random.hsrc/esp_wifi.hsrc/mbedtls/sha256.htests/host_compat_self_test.cpptests/run_host_compat_self_test.sh
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
When adding an Arduino or ESP-IDF symbol, add the minimum matching stub to the corresponding header under
src/, match the upstream signature, and return a sensible default.
Files:
src/esp_wifi.hsrc/IPAddress.hsrc/esp_heap_caps.hsrc/esp_random.hsrc/NetworkClient.cppsrc/Client.hsrc/HalOtaSlot.hsrc/Arduino.hsrc/HalOtaSlot.cppsrc/NetworkClient.hsrc/HalClock.hsrc/HalClock.cppsrc/WiFi.hsrc/SimHttpFetch.hsrc/mbedtls/sha256.hsrc/esp_http_client.h
src/{WebServer.cpp,WebSocketsServer.cpp,NetworkClient.cpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Map firmware HTTP port 80 to
http://127.0.0.1:8080/and WebSocket port 81 tows://127.0.0.1:8081/;CROSSPOINT_SIM_HTTP_PORTmust move both ports together.
Files:
src/NetworkClient.cpp
src/Hal*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Each simulator
Hal*.cpp/.hmust preserve the corresponding firmware HAL class's public surface. When firmware adds a HAL method, add a matching stub—usually a no-op—with the exact signature; do not invent public HAL methods.
Files:
src/HalOtaSlot.hsrc/HalOtaSlot.cppsrc/HalClock.hsrc/HalClock.cpp
src/Arduino.h
📄 CodeRabbit inference engine (CLAUDE.md)
Implement
millis()andmicros()usingsteady_clock, notsystem_clock, so wall-clock changes do not affect timing.
Files:
src/Arduino.h
sample-platformio-{macos,linux-wsl}.ini
📄 CodeRabbit inference engine (CLAUDE.md)
Keep macOS and Linux/WSL PlatformIO build flags synchronized when changing flags; retain architecture-correct SDL flags, and Linux/WSL OpenSSL linkage with
-lssl -lcrypto -Wno-deprecated-declarations. Native Windows is unsupported.
Files:
sample-platformio-macos.inisample-platformio-linux-wsl.ini
🪛 ast-grep (0.45.0)
src/Arduino.h
[warning] 55-55: rand(), random(), and the *rand48 family are non-cryptographic pseudo-random number generators. Their output is predictable and must not be used for security tokens, keys, nonces, salts, or session identifiers. Use a cryptographically secure source such as getrandom(2), arc4random() / arc4random_uniform(), or RAND_bytes() from OpenSSL.
Context: random(max - min)
Note: [CWE-338] Use of Cryptographically Weak Pseudo-Random Number Generator (PRNG).
(insecure-random-rand-c)
src/SimHttpFetch.h
[warning] 295-295: Avoid atoi/atol/atoll/atof: they perform no error detection, returning 0 on non-numeric input and invoking undefined behavior on out-of-range values. This rule flags every use of these functions regardless of data provenance. Convert strings with strtol/strtoul/strtod and check errno (and the endptr) so malformed or overflowing input is rejected.
Context: atoi(statusText.c_str())
Note: [CWE-20] Improper Input Validation.
(atoi-no-error-detection-c)
🪛 Ruff (0.16.0)
run_simulator.py
[error] 3-3: Undefined name Import
(F821)
🪛 Shellcheck (0.11.0)
tests/run_host_compat_self_test.sh
[info] 17-17: Double quote to prevent globbing and word splitting.
(SC2086)
🔇 Additional comments (23)
src/HalClock.h (1)
7-37: LGTM!src/HalClock.cpp (1)
8-84: LGTM!Also applies to: 95-98, 173-173
src/SimHttpFetch.h (1)
3-11: LGTM!Also applies to: 24-25, 103-143, 233-257, 272-273, 286-309
src/esp_http_client.h (1)
3-29: LGTM!Also applies to: 52-52, 74-85, 95-152, 178-200, 203-266, 288-315, 317-357, 367-373, 386-387
src/mbedtls/sha256.h (2)
16-23: 📐 Maintainability & Code QualityConfirm
<cstring>is included in this header.
std::memsetis used here and inmbedtls_sha256_free; the prior version's<cstring>need may have come via another include. Lines 1-5 aren't in the review context.#!/bin/bash fd -p 'src/mbedtls/sha256.h' --exec sed -n '1,20p'
6-14: LGTM!Also applies to: 25-62
sample-platformio-linux-wsl.ini (1)
37-37: LGTM!Also applies to: 53-53
sample-platformio-macos.ini (1)
50-50: LGTM!run_simulator.py (1)
1-5: LGTM!tests/host_compat_self_test.cpp (1)
13-25: LGTM!Also applies to: 27-117
tests/run_host_compat_self_test.sh (1)
1-7: LGTM!Also applies to: 19-20
README.md (2)
119-121: 📐 Maintainability & Code QualityVerify the script's executable bit.
The documented invocation relies on the file mode; otherwise the snippet should be
bash tests/run_host_compat_self_test.sh.#!/bin/bash git ls-files -s tests/run_host_compat_self_test.sh
15-15: LGTM!Also applies to: 38-39, 115-118, 122-122
src/Arduino.h (1)
10-23: LGTM!src/Client.h (1)
1-5: LGTM!src/IPAddress.h (1)
9-15: LGTM!Also applies to: 26-30
src/NetworkClient.h (1)
10-18: LGTM!Also applies to: 41-41
src/NetworkClient.cpp (1)
11-11: LGTM!Also applies to: 61-63
src/WiFi.h (1)
2-2: LGTM!Also applies to: 42-42, 182-186
src/esp_heap_caps.h (1)
1-22: LGTM!src/esp_wifi.h (1)
13-13: LGTM!src/HalOtaSlot.h (1)
1-27: LGTM!src/HalOtaSlot.cpp (1)
1-13: LGTM!
4462f0c to
464d724
Compare
Summary
esp_http_clienthandles, request bodies, redirect/final response headers, timeouts, chunked reads, and connection stateWhy
CrossMux currently carries a second simulator implementation because the shared
simulator does not expose several APIs used by current firmware. Supplying those
host APIs here lets consumers use this repository directly as their PlatformIO
native dependency.
Redirect headers are still delivered to the firmware callback, while chunked
and persistent-connection state is derived only from the final response.
Validation
tests/run_host_compat_self_test.shLDFLAGS=-DNDEBUG tests/run_host_compat_self_test.shpio run -e simulator -e simulator_x3in CrossMux using this branch as a local dependency