feat(rp2350w): add AutoResearch HTTP peer - #3843
Conversation
📝 WalkthroughWalkthroughChangesRP2350W AutoResearch networking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Sketch as AutoResearch sketch
participant Network as AutoResearchNet
participant CYW43 as RP2350W CYW43
participant Peer as HTTP peer
Sketch->>Network: startNetServer()
Network->>CYW43: initialize WiFiServer
Sketch->>Network: pollNetServer()
Network->>CYW43: accept and read request
CYW43->>Peer: serve /ping, /status, or /leds
Peer-->>Network: HTTP response
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
🧹 Nitpick comments (3)
ci/tests/test_rp2350w_board_config.py (2)
22-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
AUTORESEARCH_INOinstead of adding a duplicate path constant.Line 11 already binds the same path to
AUTORESEARCH_INO.AUTORESEARCH_SKETCHat Line 23 duplicates it. Two names for one file can drift if the sketch is ever renamed.♻️ Proposed fix to drop the duplicate constant
AUTORESEARCH_NET = REPO_ROOT / "examples" / "AutoResearch" / "AutoResearchNet.cpp" -AUTORESEARCH_SKETCH = REPO_ROOT / "examples" / "AutoResearch" / "AutoResearch.ino"Then use
AUTORESEARCH_INOin the new test:- sketch_source = AUTORESEARCH_SKETCH.read_text(encoding="utf-8") + sketch_source = AUTORESEARCH_INO.read_text(encoding="utf-8")🤖 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 `@ci/tests/test_rp2350w_board_config.py` around lines 22 - 23, Remove the duplicate AUTORESEARCH_SKETCH constant and update the new test to reference the existing AUTORESEARCH_INO symbol for the sketch path, preserving the current path and behavior.
106-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLine 110 passes even if the RP2350 polling implementation is removed.
AutoResearchNet.cppdefinesvoid pollNetServer() {}as a no-op in the ESP32 branch (Line 489) and in the fallback branch (Line 770). The substring"void pollNetServer()"matches those no-ops. If the RP2350 implementation at Lines 671-726 is deleted, this assertion still passes, so the test does not guard what its name claims.Assert on a symbol that only the RP2350 implementation contains, for example the
rpPeerState()call inside the polling body.💚 Proposed fix to tighten the assertion
assert "void pollNetServer()" in net_source + assert "state.server->accept()" in net_source + assert "hasClient()" in net_source assert "pollNetServer();" in sketch_source🤖 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 `@ci/tests/test_rp2350w_board_config.py` around lines 106 - 111, Replace the broad “void pollNetServer()” assertion in the test with an assertion for a symbol unique to the RP2350 polling implementation, such as the rpPeerState() call within pollNetServer. Keep the existing pollNetServer invocation assertion unchanged.examples/AutoResearch/AutoResearchNet.cpp (1)
561-576: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRead the status line until newline, not until the socket drains.
The wait loop at Line 555 returns as soon as one byte is available. The read loop at Line 563 then stops when
available()reaches zero. If the peer's first TCP segment delivers fewer than 12 bytes,status_lineis truncated and the test reports "Expected HTTP 200" even though the peer answered correctly. Continue reading until a newline or the deadline.♻️ Proposed fix to keep reading until the newline or deadline
char status_line[64]; size_t length = 0; - while (client.available() && length + 1 < sizeof(status_line)) { - const int ch = client.read(); - if (ch < 0 || ch == '\n') { - break; - } - if (ch != '\r') { - status_line[length++] = static_cast<char>(ch); - } - } + bool status_line_complete = false; + while (!status_line_complete && length + 1 < sizeof(status_line) && + static_cast<int32_t>(millis() - deadline_ms) < 0) { + if (!client.available()) { + if (!client.connected()) { + break; + } + FastLED.watchdog().feed(); + delay(1); + continue; + } + const int ch = client.read(); + if (ch < 0) { + break; + } + if (ch == '\n') { + status_line_complete = true; + break; + } + if (ch != '\r') { + status_line[length++] = static_cast<char>(ch); + } + } status_line[length] = '\0';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/AutoResearch/AutoResearchNet.cpp` around lines 561 - 576, Update the status-line read logic around client.available() so it continues waiting for additional bytes until a newline is received or the existing request deadline expires, rather than ending when the socket is temporarily empty. Preserve the buffer bound, carriage-return filtering, null termination, and HTTP 200 validation in the passed check.
🤖 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 `@examples/AutoResearch/AutoResearchNet.cpp`:
- Around line 617-622: Split the combined guard in the request-handling flow
around host_ip and fl::net::wifi::isConnected() into separate checks. Return an
error identifying the invalid or missing host_ip when host_ip is null or empty,
and preserve the WiFi station error only for a disconnected station.
- Around line 671-708: Update pollNetServer and the associated RpPeerState to
track an accept/request idle deadline, initialize it when accepting a client,
and release/reset the client when the deadline expires or client->connected() is
false. Ensure stalled partial requests and peers that disconnect without sending
a newline free the single client slot so later connections can be accepted,
while preserving the existing request parsing and 414 handling.
---
Nitpick comments:
In `@ci/tests/test_rp2350w_board_config.py`:
- Around line 22-23: Remove the duplicate AUTORESEARCH_SKETCH constant and
update the new test to reference the existing AUTORESEARCH_INO symbol for the
sketch path, preserving the current path and behavior.
- Around line 106-111: Replace the broad “void pollNetServer()” assertion in the
test with an assertion for a symbol unique to the RP2350 polling implementation,
such as the rpPeerState() call within pollNetServer. Keep the existing
pollNetServer invocation assertion unchanged.
In `@examples/AutoResearch/AutoResearchNet.cpp`:
- Around line 561-576: Update the status-line read logic around
client.available() so it continues waiting for additional bytes until a newline
is received or the existing request deadline expires, rather than ending when
the socket is temporarily empty. Preserve the buffer bound, carriage-return
filtering, null termination, and HTTP 200 validation in the passed check.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b2965e96-0748-4d22-b199-09b22bd3fa70
📒 Files selected for processing (4)
ci/tests/test_rp2350w_board_config.pyexamples/AutoResearch/AutoResearch.inoexamples/AutoResearch/AutoResearchNet.cppexamples/AutoResearch/AutoResearchNet.h
| if (host_ip == nullptr || *host_ip == '\0' || | ||
| !fl::net::wifi::isConnected()) { | ||
| response.set("success", false); | ||
| response.set("error", "WiFi station is not connected"); | ||
| return response; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Report the actual failure cause for an invalid host_ip.
Lines 617-622 collapse two distinct failures into one message. If host_ip is null or empty while WiFi is connected, the response still says "WiFi station is not connected". That message misdirects fixture debugging. Separate the two checks.
🔧 Proposed fix to distinguish the two failures
- if (host_ip == nullptr || *host_ip == '\0' ||
- !fl::net::wifi::isConnected()) {
+ if (host_ip == nullptr || *host_ip == '\0') {
+ response.set("success", false);
+ response.set("error", "host_ip is missing or empty");
+ return response;
+ }
+ if (!fl::net::wifi::isConnected()) {
response.set("success", false);
response.set("error", "WiFi station is not connected");
return response;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (host_ip == nullptr || *host_ip == '\0' || | |
| !fl::net::wifi::isConnected()) { | |
| response.set("success", false); | |
| response.set("error", "WiFi station is not connected"); | |
| return response; | |
| } | |
| if (host_ip == nullptr || *host_ip == '\0') { | |
| response.set("success", false); | |
| response.set("error", "host_ip is missing or empty"); | |
| return response; | |
| } | |
| if (!fl::net::wifi::isConnected()) { | |
| response.set("success", false); | |
| response.set("error", "WiFi station is not connected"); | |
| return response; | |
| } |
🤖 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 `@examples/AutoResearch/AutoResearchNet.cpp` around lines 617 - 622, Split the
combined guard in the request-handling flow around host_ip and
fl::net::wifi::isConnected() into separate checks. Return an error identifying
the invalid or missing host_ip when host_ip is null or empty, and preserve the
WiFi station error only for a disconnected station.
| void pollNetServer() { | ||
| RpPeerState& state = rpPeerState(); | ||
| if (!state.server) { | ||
| return; | ||
| } | ||
| if (!state.client && state.server->hasClient()) { | ||
| state.client = fl::make_unique<WiFiClient>(state.server->accept()); | ||
| state.request_length = 0; | ||
| } | ||
| if (!state.client || !state.client->available()) { | ||
| return; | ||
| } | ||
|
|
||
| bool request_line_complete = false; | ||
| while (state.client->available() && | ||
| state.request_length + 1 < sizeof(state.request)) { | ||
| const int ch = state.client->read(); | ||
| if (ch < 0) { | ||
| break; | ||
| } | ||
| if (ch == '\n') { | ||
| request_line_complete = true; | ||
| break; | ||
| } | ||
| if (ch != '\r') { | ||
| state.request[state.request_length++] = static_cast<char>(ch); | ||
| } | ||
| } | ||
| if (!request_line_complete) { | ||
| if (state.request_length + 1 == sizeof(state.request)) { | ||
| state.client->print( | ||
| "HTTP/1.1 414 URI Too Long\r\nConnection: close\r\n\r\n"); | ||
| state.client->stop(); | ||
| state.client.reset(); | ||
| state.request_length = 0; | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A stalled peer permanently wedges the single client slot.
pollNetServer accepts at most one client at Line 676 and only when state.client is null. Nothing releases that client unless a newline arrives or the 127-byte buffer fills exactly.
Trace the stall path:
- A peer connects and sends a few bytes with no
\n, then stops. request_line_completestays false andstate.request_lengthstays below the bound, so the 414 branch at Line 700 never runs.state.clientremains set. Every later poll returns at Line 680.- Line 676 never accepts a new client, so
/ping,/status, and/ledsstop responding untilstopNet()runs.
A peer that opens the connection and disappears causes the same wedge, because Line 680 returns before any connected() check. A port scan is enough to trigger this. Add an accept deadline and drop a disconnected client.
🔒️ Proposed fix to add an idle deadline and disconnect handling
Add the deadline field to the state struct:
struct RpPeerState {
fl::unique_ptr<WiFiServer> server;
fl::unique_ptr<WiFiClient> client;
char request[128] = {};
size_t request_length = 0;
+ uint32_t client_deadline_ms = 0;
};Then release the client on timeout or disconnect:
if (!state.client && state.server->hasClient()) {
state.client = fl::make_unique<WiFiClient>(state.server->accept());
state.request_length = 0;
+ state.client_deadline_ms = millis() + 2000;
}
- if (!state.client || !state.client->available()) {
+ if (!state.client) {
+ return;
+ }
+ if (!state.client->available()) {
+ const bool timed_out =
+ static_cast<int32_t>(millis() - state.client_deadline_ms) >= 0;
+ if (timed_out || !state.client->connected()) {
+ state.client->stop();
+ state.client.reset();
+ state.request_length = 0;
+ }
return;
}🤖 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 `@examples/AutoResearch/AutoResearchNet.cpp` around lines 671 - 708, Update
pollNetServer and the associated RpPeerState to track an accept/request idle
deadline, initialize it when accepting a client, and release/reset the client
when the deadline expires or client->connected() is false. Ensure stalled
partial requests and peers that disconnect without sending a newline free the
single client slot so later connections can be accepted, while preserving the
existing request parsing and 414 handling.
|
Update: fbuild#1251 merged after the real RP2350W compile showed an Arduino-Pico lwIP_CYW43/lwIP_Ethernet private-header collision. The autonomous fbuild 2.5.9 release is queued; this PR will update its pin and rerun the RP2350W AutoResearch build once published. |
1 participant
Part of #3832.\n\nAdds the RP2350W CYW43 implementation of AutoResearch's existing peer HTTP surface: cooperative server polling plus outbound C6 fixture checks for /ping, /status, and /leds. The RP peer state is held in l::Singleton.\n\nThe follow-up fbuild 2.5.8 dependency bump is waiting for the autonomous release from FastLED/fbuild#1249; that release fixes Arduino-Pico framework-library selection so WiFi headers and sources are available to this build.
Summary by CodeRabbit
New Features
Tests