-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
remove early return from initconnection() #4903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughRemoved 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
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
The missing attributes where later fixed upstream in esp_dmx. |
@netmindz did you get a chance to test this? |
@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 |
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. |
@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 |
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 |
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