Skip to content

Created wifi-module - #663

Merged
KenVanHoeylandt merged 4 commits into
mainfrom
wifi-improvements
Sep 26, 2026
Merged

KenVanHoeylandt merged 4 commits into
mainfrom
wifi-improvements

Conversation

@KenVanHoeylandt

@KenVanHoeylandt KenVanHoeylandt commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor
  • Moved WiFi logic from TactilityKernel/ to wifi-module
  • Created commandline app to control Wi-Fi
  • Update tactility.py to v6.1.0 so it can be executed directly

+ created wifi commandline app
+ update tactility.py
@coderabbitai

coderabbitai Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

📝 Walkthrough

Walkthrough

The Wi-Fi service and settings implementation move into a new wifi-module, which provides saved-network persistence, provisioning import, an auto-connect service, and a command-line app. Applications and platform services now use Wi-Fi device and driver APIs. The former Wi-Fi service interfaces and implementation are removed. Build and SDK configuration includes the new module.

Priority: ➖ Normal

Merge Risk: 🟡 Moderate · up to e51fa

Failed connection attempts can disable automatic reconnection, and a transient Wi-Fi startup failure can prevent the module from being retried until restart. These issues should be fixed before merging.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to e51fa

Manual connection and automatic reconnection no longer consistently share the same coordination path. Under particular event timing, a saved network could be selected while a user is connecting to a different network. The observed scope is one device; broader exposure is not established.

Retained concerns

  • Medium · security · inferred: Migrated UI connection paths bypass the pause used to coordinate manual attempts with auto-connect. If a scan-finished event is handled before the driver reports the manual attempt as active, auto-connect can initiate a connection to a saved AP instead.
Security review details

Security Blast Radius

  • inferred — The identified connection-ordering race can affect which network a device joins. Evidence establishes neither cross-device reach nor a new tenant or credential boundary.

Security Findings and Attack Paths

  • inferred — A scan-finished event during a direct UI connection attempt can select a saved, auto-connect-enabled AP before the manual attempt is reflected as active. The worker's active-station check limits this to an ordering window; no successful unwanted connection was demonstrated.

Trust Boundaries and Controls

  • observed — Auto-connect checks pause, radio, and station state before connecting. The new command's manual-connect path sets the pause, whereas the inspected UI connect paths do not.

Resilience and Maintainability Implications

  • observed — When no device is found at startup, the auto-connect service returns success without subscribing to boot or Wi-Fi events. Platform device-start ordering and whether this differs materially from the former service remain unverified.

Hardening Proposals

  • proposed — Route every manual connection through one operation that establishes auto-connect exclusion before radio or station changes, retains it across radio-on events, and resolves it on success, failure, cancellation, or timeout.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 111 functions across 26 files. (2 skipped… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the primary change: creation of the new wifi-module. It is concise and related to the pull request objectives.
Full details: Docstring Coverage

Explanation

Docstring coverage is 11.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 111 functions across 26 files. (2 skipped: 2 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: cbbcfa5e-576c-4578-86c1-e3293931139f

📥 Commits

Reviewing files that changed from the base of the PR and between 7c508bb and fc2cafc.

📒 Files selected for processing (43)
  • Buildscripts/release-sdk-modules.txt
  • CMakeLists.txt
  • Documentation/ideas.md
  • Modules/wifi-module/CMakeLists.txt
  • Modules/wifi-module/LICENSE-Apache-2.0.md
  • Modules/wifi-module/README.md
  • Modules/wifi-module/include/wifi/module.h
  • Modules/wifi-module/include/wifi/wifi_autoconnect.h
  • Modules/wifi-module/include/wifi/wifi_settings.h
  • Modules/wifi-module/private/wifi/private/wifi_service.h
  • Modules/wifi-module/source/module.cpp
  • Modules/wifi-module/source/wifi_autoconnect.cpp
  • Modules/wifi-module/source/wifi_command.cpp
  • Modules/wifi-module/source/wifi_provisioning.cpp
  • Modules/wifi-module/source/wifi_settings.cpp
  • Tactility/CMakeLists.txt
  • Tactility/Include/Tactility/service/wifi/Wifi.h
  • Tactility/Include/Tactility/service/wifi/WifiApSettings.h
  • Tactility/Include/Tactility/service/wifi/WifiGlobals.h
  • Tactility/Include/Tactility/service/wifi/WifiSettings.h
  • Tactility/Private/Tactility/app/wifimanage/State.h
  • Tactility/Private/Tactility/app/wifimanage/WifiManagePrivate.h
  • Tactility/Private/Tactility/service/wifi/WifiBootSplashInit.h
  • Tactility/Private/Tactility/service/wifi/WifiPrivate.h
  • Tactility/Source/Tactility.cpp
  • Tactility/Source/app/apphub/AppHubApp.cpp
  • Tactility/Source/app/development/Development.cpp
  • Tactility/Source/app/setup/Setup.cpp
  • Tactility/Source/app/webserversettings/WebServerSettings.cpp
  • Tactility/Source/app/wifiapsettings/WifiApSettings.cpp
  • Tactility/Source/app/wificonnect/WifiConnect.cpp
  • Tactility/Source/app/wifimanage/State.cpp
  • Tactility/Source/app/wifimanage/View.cpp
  • Tactility/Source/app/wifimanage/WifiManage.cpp
  • Tactility/Source/service/espnow/EspNowBackendNative.cpp
  • Tactility/Source/service/statusbar/Statusbar.cpp
  • Tactility/Source/service/webserver/WebServerService.cpp
  • Tactility/Source/service/wifi/Wifi.cpp
  • Tactility/Source/service/wifi/WifiApSettings.cpp
  • Tactility/Source/service/wifi/WifiBootSplashInit.cpp
  • Tactility/Source/service/wifi/WifiSettings.cpp
  • TactilityKernel/include/tactility/wifi_auto_scan.h
  • Tests/SdkIntegration/tactility.py
💤 Files with no reviewable changes (11)
  • Tactility/Private/Tactility/service/wifi/WifiPrivate.h
  • Tactility/Include/Tactility/service/wifi/WifiSettings.h
  • Tactility/Private/Tactility/service/wifi/WifiBootSplashInit.h
  • Tactility/Source/service/wifi/Wifi.cpp
  • Tactility/Include/Tactility/service/wifi/WifiApSettings.h
  • Tactility/Source/service/wifi/WifiBootSplashInit.cpp
  • Tactility/Private/Tactility/app/wifimanage/WifiManagePrivate.h
  • Tactility/Include/Tactility/service/wifi/Wifi.h
  • Tactility/Include/Tactility/service/wifi/WifiGlobals.h
  • Tactility/Source/service/wifi/WifiSettings.cpp
  • Tactility/Source/service/wifi/WifiApSettings.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

Comment thread Modules/wifi-module/README.md Outdated
Comment thread Modules/wifi-module/source/wifi_command.cpp

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to GitHub limitations.

⚠️ Outside diff range comments (1)

🟡 Minor · Remove the Wi-Fi service manifest when startup fails. · module.cpp:14-23

Modules/wifi-module/source/module.cpp:14-23
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Remove the Wi-Fi service manifest when startup fails.

service_manager_add registers wifi_service_manifest before starting it. A failed start removes only the service instance, not the manifest. This path removes the command app but leaves the manifest registered. A retry then fails with ERROR_INVALID_ARGUMENT, and unloading the failed module does not call stop because the module never reached the started state.

Suggested fix
    error = service_manager_add(&wifi_service_manifest, /*auto_start=*/true);
    if (error != ERROR_NONE) {
+       service_manager_remove(wifi_service_manifest.id);
        app_manager_remove(wifi_command_manifest.id);
    }

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: c8fc0644-fa5e-46da-b892-48fa11c0f812

📥 Commits

Reviewing files that changed from the base of the PR and between fc2cafc and e51fa62.

📒 Files selected for processing (3)
  • Modules/cpp-symbols-module/source/function_instantiations.cpp
  • Modules/wifi-module/README.md
  • Tactility/Tests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
  • Modules/wifi-module/README.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

@KenVanHoeylandt
KenVanHoeylandt merged commit 33898dd into main Sep 26, 2026
65 checks passed
@KenVanHoeylandt
KenVanHoeylandt deleted the wifi-improvements branch September 26, 2026 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant