Wifi improvments#29
Merged
Merged
Conversation
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdd configurable scan limits and a scan-driven “best AP first” STA connection workflow with BSSID/channel locking, exponential reconnect backoff, and centralized STA config. Update HTTP scan endpoint to use the limit. Replace DialogService with an Angular modal for Wi‑Fi scanning and show deduplicated strongest results. ChangesWi‑Fi Scanning & Network Selection
Sequence DiagramsequenceDiagram
participant User as User
participant UI as Angular UI
participant Server as HTTP Server
participant Firmware as ESP32 Firmware
User->>UI: Click "Scan Networks"
activate UI
UI->>Server: GET /api/wifi/scan
deactivate UI
activate Server
Server->>Firmware: esp_wifi_scan_start()
deactivate Server
activate Firmware
Firmware->>Firmware: Active scan with timeout/retry
Firmware->>Firmware: WIFI_EVENT_SCAN_DONE (cap to LIMIT, sort by RSSI)
deactivate Firmware
activate Server
Firmware->>Server: Scan complete / results
Server->>UI: Return deduplicated networks (strongest per SSID)
deactivate Server
activate UI
UI->>UI: Populate scannedNetworks and show modal
deactivate UI
User->>UI: Select network
activate UI
UI->>UI: selectWifiNetwork(ssid) -> patch form and hide modal
deactivate UI
Note over Firmware: On STA_START or STA_DISCONNECTED: attempt strongest AP first, then fallback to unspecific connect
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
main/http_server/forge-os/src/app/components/network-edit/network.edit.component.ts (1)
99-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the component base URI for Wi‑Fi scans too.
This request is hardcoded to
/api/system/wifi/scan, while the rest of the component goes throughthis.uri. If this screen is served from a prefixed path, scan is the odd one out and will hit the wrong endpoint.Suggested fix
public scanWifi() { this.scanning = true; - this.http.get<{networks: WifiNetwork[]}>('/api/system/wifi/scan') + const baseUri = this.uri.replace(/\/$/, ''); + this.http.get<{networks: WifiNetwork[]}>(`${baseUri}/api/system/wifi/scan`) .pipe( finalize(() => this.scanning = false) )🤖 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 `@main/http_server/forge-os/src/app/components/network-edit/network.edit.component.ts` around lines 99 - 123, The scanWifi() method uses a hardcoded endpoint '/api/system/wifi/scan' instead of the component base URI; update the HTTP call in scanWifi (the this.http.get(...) invocation) to build the request using this.uri (same pattern used elsewhere in the component) so the request respects any prefixed path, then keep the rest of the logic (filter/sort/reduce, this.scannedNetworks assignment and this.wifiScanModal.isVisible toggle) unchanged.components/connect/connect.c (1)
315-345:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip STA reconnect logic when no target SSID is configured.
The new strongest-AP flow runs on
WIFI_EVENT_STA_STARTand disconnect events unconditionally. In the no-SSID boot path, that means AP-only startup can still enter scan/connect retries with empty credentials.Suggested fix
if (event_id == WIFI_EVENT_STA_START) { + if (s_target_ssid[0] == '\0') { + ESP_LOGI(TAG, "No STA SSID configured, skipping station connect"); + return; + } esp_err_t err = wifi_connect_best_ap(); if (err != ESP_OK) { ESP_LOGW(TAG, "Strongest-AP connect unavailable, using fallback connect"); @@ } else if (event_id == WIFI_EVENT_STA_DISCONNECTED) { + if (s_target_ssid[0] == '\0') { + return; + } wifi_event_sta_disconnected_t* event = (wifi_event_sta_disconnected_t*) event_data; if (event->reason == WIFI_REASON_ROAMING) { ESP_LOGI(TAG, "We are roaming, nothing to do");🤖 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 `@components/connect/connect.c` around lines 315 - 345, The STA start/disconnect handler unconditionally calls wifi_connect_best_ap() / wifi_connect_with_fallback(), which triggers scan/retry even when no target SSID is configured; add a guard that checks whether a configured target SSID is present (non-empty) before attempting any connect/reconnect logic in both the WIFI_EVENT_STA_START and WIFI_EVENT_STA_DISCONNECTED branches, and if no SSID is configured simply skip the wifi_connect_best_ap()/wifi_connect_with_fallback() calls (and return or set AP-only status) so the device stays in AP-only mode; look for the handler containing the WIFI_EVENT_STA_START and WIFI_EVENT_STA_DISCONNECTED cases and wrap calls to wifi_connect_best_ap and wifi_connect_with_fallback with the SSID-present 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 `@components/connect/connect.c`:
- Around line 293-305: The code calls
esp_wifi_scan_get_ap_records(&fetched_ap_count, ap_info) but only logs errors
and then proceeds to reuse potentially stale ap_info via ap_number and
wifi_connect_best_ap_from_scan_results; fix by, when
esp_wifi_scan_get_ap_records() returns non-ESP_OK, reset ap_number to 0 (and
optionally clear ap_info), ensure s_scan_mode and is_scanning remain consistent,
and skip invoking wifi_connect_best_ap_from_scan_results() when the fetch failed
(use completed_scan_mode only when records were successfully fetched); reference
esp_wifi_scan_get_ap_records, ap_info, ap_number, s_scan_mode, is_scanning,
completed_scan_mode, and wifi_connect_best_ap_from_scan_results.
In
`@main/http_server/forge-os/src/app/components/network-edit/network.edit.component.scss`:
- Around line 256-292: Long SSIDs inside .wifi-network-button can push the RSSI
off-screen because the .wifi-network-label keeps the default flex min width;
update .wifi-network-label to allow shrinking by adding a flex rule (e.g., flex:
1 1 auto or flex: 1 1 0) and set min-width: 0 so the text-overflow: ellipsis can
take effect; keep pointer-events and font-weight as-is and ensure the RSSI
(.wifi-network-rssi) remains nowrap.
---
Outside diff comments:
In `@components/connect/connect.c`:
- Around line 315-345: The STA start/disconnect handler unconditionally calls
wifi_connect_best_ap() / wifi_connect_with_fallback(), which triggers scan/retry
even when no target SSID is configured; add a guard that checks whether a
configured target SSID is present (non-empty) before attempting any
connect/reconnect logic in both the WIFI_EVENT_STA_START and
WIFI_EVENT_STA_DISCONNECTED branches, and if no SSID is configured simply skip
the wifi_connect_best_ap()/wifi_connect_with_fallback() calls (and return or set
AP-only status) so the device stays in AP-only mode; look for the handler
containing the WIFI_EVENT_STA_START and WIFI_EVENT_STA_DISCONNECTED cases and
wrap calls to wifi_connect_best_ap and wifi_connect_with_fallback with the
SSID-present check.
In
`@main/http_server/forge-os/src/app/components/network-edit/network.edit.component.ts`:
- Around line 99-123: The scanWifi() method uses a hardcoded endpoint
'/api/system/wifi/scan' instead of the component base URI; update the HTTP call
in scanWifi (the this.http.get(...) invocation) to build the request using
this.uri (same pattern used elsewhere in the component) so the request respects
any prefixed path, then keep the rest of the logic (filter/sort/reduce,
this.scannedNetworks assignment and this.wifiScanModal.isVisible toggle)
unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5cbbed17-703c-4fd3-a826-e549f7422ce9
📒 Files selected for processing (6)
components/connect/connect.ccomponents/connect/include/connect.hmain/http_server/forge-os/src/app/components/network-edit/network.edit.component.htmlmain/http_server/forge-os/src/app/components/network-edit/network.edit.component.scssmain/http_server/forge-os/src/app/components/network-edit/network.edit.component.tsmain/http_server/http_server.c
Comment on lines
+293
to
+305
| if (esp_wifi_scan_get_ap_records(&fetched_ap_count, ap_info) != ESP_OK) { | ||
| ESP_LOGI(TAG, "Failed esp_wifi_scan_get_ap_records"); | ||
| } | ||
|
|
||
| ap_number = fetched_ap_count; | ||
| s_scan_mode = WIFI_SCAN_MODE_NONE; | ||
| is_scanning = false; | ||
|
|
||
| if (completed_scan_mode == WIFI_SCAN_MODE_CONNECT) { | ||
| esp_err_t err = wifi_connect_best_ap_from_scan_results(); | ||
| if (err != ESP_OK) { | ||
| ESP_LOGE(TAG, "Failed to connect using strongest AP selection: %s", esp_err_to_name(err)); | ||
| } |
Contributor
There was a problem hiding this comment.
Don't reuse stale scan results after esp_wifi_scan_get_ap_records() fails.
This path only logs the fetch failure, but it still leaves ap_number populated and can immediately call wifi_connect_best_ap_from_scan_results() with whatever was already in ap_info. That can steer reconnects to an old BSSID after a failed scan.
Suggested fix
- if (esp_wifi_scan_get_ap_records(&fetched_ap_count, ap_info) != ESP_OK) {
- ESP_LOGI(TAG, "Failed esp_wifi_scan_get_ap_records");
- }
-
- ap_number = fetched_ap_count;
+ if (esp_wifi_scan_get_ap_records(&fetched_ap_count, ap_info) != ESP_OK) {
+ ESP_LOGI(TAG, "Failed esp_wifi_scan_get_ap_records");
+ ap_number = 0;
+ s_scan_mode = WIFI_SCAN_MODE_NONE;
+ is_scanning = false;
+
+ if (completed_scan_mode == WIFI_SCAN_MODE_CONNECT) {
+ wifi_connect_with_fallback();
+ }
+ return;
+ }
+
+ ap_number = fetched_ap_count;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/connect/connect.c` around lines 293 - 305, The code calls
esp_wifi_scan_get_ap_records(&fetched_ap_count, ap_info) but only logs errors
and then proceeds to reuse potentially stale ap_info via ap_number and
wifi_connect_best_ap_from_scan_results; fix by, when
esp_wifi_scan_get_ap_records() returns non-ESP_OK, reset ap_number to 0 (and
optionally clear ap_info), ensure s_scan_mode and is_scanning remain consistent,
and skip invoking wifi_connect_best_ap_from_scan_results() when the fetch failed
(use completed_scan_mode only when records were successfully fetched); reference
esp_wifi_scan_get_ap_records, ap_info, ap_number, s_scan_mode, is_scanning,
completed_scan_mode, and wifi_connect_best_ap_from_scan_results.
Comment on lines
+256
to
+292
| .wifi-network-button { | ||
| width: 100%; | ||
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| text-align: left; | ||
| gap: 1rem; | ||
| padding: 1rem 1.25rem; | ||
| border-radius: 0.75rem; | ||
| background: rgba(15, 23, 42, 0.9) !important; | ||
| border: 1px solid rgba(51, 65, 85, 0.6) !important; | ||
| color: var(--text-primary) !important; | ||
| box-shadow: inset 0 1px 0 rgba(148, 163, 184, 0.08); | ||
|
|
||
| &:hover:not(:disabled) { | ||
| transform: translateY(-1px); | ||
| border-color: rgba(6, 182, 212, 0.65) !important; | ||
| box-shadow: 0 10px 24px rgba(2, 132, 199, 0.18); | ||
| } | ||
| } | ||
|
|
||
| .wifi-network-label, | ||
| .wifi-network-rssi { | ||
| pointer-events: none; | ||
| } | ||
|
|
||
| .wifi-network-label { | ||
| font-weight: 600; | ||
| overflow: hidden; | ||
| text-overflow: ellipsis; | ||
| } | ||
|
|
||
| .wifi-network-rssi { | ||
| color: var(--text-secondary); | ||
| white-space: nowrap; | ||
| font-size: 0.875rem; | ||
| } |
Contributor
There was a problem hiding this comment.
Let long SSIDs shrink inside the flex row.
Right now a long network name can still force the RSSI label off the edge because the SSID span keeps its default flex minimum width. Add a shrinking rule so the ellipsis actually kicks in.
Suggested fix
.wifi-network-label {
+ flex: 1 1 auto;
+ min-width: 0;
+ white-space: nowrap;
font-weight: 600;
overflow: hidden;
text-overflow: ellipsis;
}🤖 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
`@main/http_server/forge-os/src/app/components/network-edit/network.edit.component.scss`
around lines 256 - 292, Long SSIDs inside .wifi-network-button can push the RSSI
off-screen because the .wifi-network-label keeps the default flex min width;
update .wifi-network-label to allow shrinking by adding a flex rule (e.g., flex:
1 1 auto or flex: 1 1 0) and set min-width: 0 so the text-overflow: ellipsis can
take effect; keep pointer-events and font-weight as-is and ensure the RSSI
(.wifi-network-rssi) remains nowrap.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixed wifi scan screen
added longer scan duration
added logic to connect to best BSSID
Summary by CodeRabbit
New Features
Improvements