Skip to content

Conversation

DedeHai
Copy link
Collaborator

@DedeHai DedeHai commented Sep 2, 2025

this line was added in aed03cd as a part of #4495 to fix a bug which no longer seems relevant.

@arneboe and @netmindz please test and make sure it does not bring back that bug.

fixes #4655 (tested and confirmed)

Summary by CodeRabbit

  • New Features
    • Additional services now initialize when the device runs in Access Point mode without prior Wi‑Fi setup, enabling broader functionality out of the box.
  • Bug Fixes
    • Resolved an issue where certain features did not start in AP mode when no Wi‑Fi was configured, improving reliability during first-time setup and standalone use.
  • Chores
    • Internal initialization flow streamlined to ensure consistent startup behavior across connection states.

Copy link
Contributor

coderabbitai bot commented Sep 2, 2025

Walkthrough

Removed an early return in WLED::initConnection() that previously exited after starting AP when no WiFi was configured, allowing the function to continue executing subsequent initialization steps in AP mode.

Changes

Cohort / File(s) Summary of Changes
Connection init control flow
wled00/wled.cpp
Eliminated early return after AP start when WiFi not configured, enabling downstream initialization (including features initialized later in initConnection()).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Assessment against linked issues

Objective Addressed Explanation
Ensure ESP-NOW initializes/works on ESP32 latest build (Wizmote detection) [#4655] Change affects only the “no WiFi configured” path; unclear if ESP-NOW failure occurs solely in that state or generally.

Possibly related PRs

Suggested reviewers

  • netmindz
  • blazoncek

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 666a59f and bc5d4fe.

📒 Files selected for processing (1)
  • wled00/wled.cpp (0 hunks)
💤 Files with no reviewable changes (1)
  • wled00/wled.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: wled_build / Build Environments (usermods)
  • GitHub Check: wled_build / Build Environments (esp32c3dev)
  • GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
  • GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
  • GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
  • GitHub Check: wled_build / Build Environments (lolin_s2_mini)
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

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

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@arneboe
Copy link
Contributor

arneboe commented Sep 2, 2025

The missing attributes where later fixed upstream in esp_dmx.
The hack was removed in 68e9d70
But apparently I did not remove the early return. Sorry about that :(

@netmindz netmindz self-assigned this Sep 3, 2025
@netmindz netmindz added connectivity Issue regarding protocols, WiFi connection or availability of interfaces Awaiting testing labels Sep 3, 2025
@DedeHai
Copy link
Collaborator Author

DedeHai commented Sep 22, 2025

@netmindz did you get a chance to test this?

@Arcitec
Copy link
Contributor

Arcitec commented Sep 26, 2025

@DedeHai @netmindz @blazoncek @willmmiles Hey, since it's on the same topic and touches the same code - I've been waiting to hear more about Blaz's network rewrite before porting my ESP-NOW and WiFi fixes. There's a lot of bugs and a complete spaghetti nest in the current networking, and it's very easy to make WLED get into a state where both WiFi and Ethernet are activated at the same time (a very old bug). The ESP-NOW code also doesn't get initialized when Ethernet-only is being used. I fixed that too. But was going to hold the porting work until the network rewrite was done.

But I'm wondering, should I just submit it anyway, and then Blaz can see how I fixed it, and implement the same fixes in his rewrite? What do you think? Seems like there's more and more interest in people asking for ESP-NOW fixes.

PS: My bug fixes are for the 0.15.x branch. Hopefully main networking is still very similar, then I can safely and easily port it to main too. But we should start with 0.15.x, which is where it's stable and verified, and then review those patches before porting it to main.

@DedeHai
Copy link
Collaborator Author

DedeHai commented Sep 26, 2025

You can do a draft PR any time to have a discussuion but please do so for main, 0.15 can be backported once tested.

@Arcitec
Copy link
Contributor

Arcitec commented Sep 26, 2025

@DedeHai Alright, I'll start looking at the main code. I hope it's not already too different from 0.15, because untangling the old mess was already heavy work. I've never seen crazier networking code (like 5 different files all modifying the same messy state without any care for synchronization). But I'll get started next week on the main code.

@netmindz
Copy link
Member

Tried to test today, but my test setup for the existing code wasn't working so I need to build another hardware setup to test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting testing connectivity Issue regarding protocols, WiFi connection or availability of interfaces
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ESP-NOW not functioning on latest build
4 participants