Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement multiple WiFi #3709

Merged
merged 11 commits into from
Feb 1, 2024
Merged

Implement multiple WiFi #3709

merged 11 commits into from
Feb 1, 2024

Conversation

blazoncek
Copy link
Collaborator

@blazoncek blazoncek added this to the 0.15.0-alpha candidate milestone Jan 21, 2024
@blazoncek blazoncek added enhancement connectivity Issue regarding protocols, WiFi connection or availability of interfaces labels Jan 22, 2024
@blazoncek blazoncek self-assigned this Jan 22, 2024
@JPZV
Copy link
Contributor

JPZV commented Jan 22, 2024

@blazoncek If I wanted to make a change to your solution, how do you like I make it? By a patch file or by a PR to multiwifi branch?

@blazoncek
Copy link
Collaborator Author

@blazoncek If I wanted to make a change to your solution, how do you like I make it? By a patch file or by a PR to multiwifi branch?

Start off by reviewing my PR.
When we agree on a change just branch off of my branch and PR back to it.

@JPZV
Copy link
Contributor

JPZV commented Jan 22, 2024

@blazoncek If I wanted to make a change to your solution, how do you like I make it? By a patch file or by a PR to multiwifi branch?

Start off by reviewing my PR. When we agree on a change just branch off of my branch and PR back to it.

I just sent you a review btw. Mainly I don't like the way you try to connect to an available network, it has many flaws

@blazoncek
Copy link
Collaborator Author

I just sent you a review btw.

I do not see any comment or review on Github. Where did you send it to?

@JPZV
Copy link
Contributor

JPZV commented Jan 22, 2024

Where did you send it to?

Right here in this PR:

image

@blazoncek
Copy link
Collaborator Author

Right here in this PR:

You need to finish/submit it for me to see it. I cannot see pending review.

JPZV
JPZV suggested changes Jan 22, 2024
View reviewed changes
wled00/wled.cpp Outdated Show resolved Hide resolved
JPZV
JPZV suggested changes Jan 23, 2024
View reviewed changes
Copy link
Contributor

@JPZV JPZV left a comment

Choose a reason for hiding this comment

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

I added two more issues that I found while testing your code.

Due to the first one on set.cpp or cfg.cpp, I cannot continue testing it, so maybe there're more issues.

wled00/cfg.cpp Show resolved Hide resolved
wled00/data/settings_wifi.htm Outdated Show resolved Hide resolved
@JPZV JPZV mentioned this pull request Jan 23, 2024
Closed
@blazoncek
Optimise wifi scan
1bebf3d 
- prioritize strongest signal
- prune removed networks
- fill present networks
@blazoncek
Remove regression
3e2aebc 
Init wifi for scan
Always save WiFi name
JPZV
JPZV suggested changes Jan 24, 2024
View reviewed changes
Copy link
Contributor

@JPZV JPZV left a comment

Choose a reason for hiding this comment

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

I've added three more issues that I found.

One related to the webpage, and the others two to the wled.cpp

wled00/data/settings_wifi.htm Outdated Show resolved Hide resolved
wled00/wled.cpp Outdated Show resolved Hide resolved
wled00/wled.cpp Outdated Show resolved Hide resolved
@blazoncek
Copy link
Collaborator Author

@JPZV I think this is now complete.

@JPZV
Copy link
Contributor

JPZV commented Jan 27, 2024

@JPZV I think this is now complete.

Okey! I'll test it right now and I'll give you my feedback today :)

JPZV
JPZV reviewed Jan 27, 2024
View reviewed changes
Copy link
Contributor

@JPZV JPZV left a comment

Choose a reason for hiding this comment

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

It looks nice. I only found a minor bug. Not sure if it needs a fix, so I'll leave it as a comment

wled00/set.cpp

strlcpy(multiWiFi[n].clientSSID, request->arg(cs).c_str(), 33);
if (strlen(oldSSID) == 0 || !strncmp(multiWiFi[n].clientSSID, oldSSID, 32)) {
forceReconnect = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems that setting the SSID to empty while connected to the WiFi doesn't triggers this ForceReconnect.

It's a silly issue because after a restart it'll behavior as intended, but I wonder how deep would be this issue

@blazoncek blazoncek merged commit fe54fad into 0_15 Feb 1, 2024
24 checks passed
@blazoncek blazoncek deleted the multiwifi branch February 1, 2024 17:38
@blazoncek blazoncek mentioned this pull request Feb 1, 2024
Closed
@JPZV
Copy link
Contributor

JPZV commented Mar 31, 2024

@blazoncek I just checked that I wasn't mentioned in v0.15.0-b1 credits

@blazoncek
Copy link
Collaborator Author

How would you want to be credited?

@JPZV
Copy link
Contributor

JPZV commented Mar 31, 2024

As a Contributor related to the Multi-Wifi feature, as I worked along with you to make it possible.

@blazoncek
Copy link
Collaborator Author

Will be included in next changelog update.

@blazoncek blazoncek mentioned this pull request Jun 19, 2024
Open
1 task
@jasmohan-narula
Copy link

@JPZV I thank you for helping implementing this. Even without credit, I appreciate your efforts.

@mellbergsimon
Copy link

Is there a limitation on which platforms this works? Esp8266 supported? There is no info in wifi setup how to enable multi wifi. The documentation seems to be outdated in that regard also. How can we show new users how to use this feature?

@blazoncek
Copy link
Collaborator Author

Screenshot 2025-01-20 at 06 45 31
It seems obvious. Same approach as adding a LED output.

@mellbergsimon
Copy link

You are correct. I did not notice the + sign. Thank you for the clarification. I should have noticed!

@blazoncek
Copy link
Collaborator Author

You can, if you wish, and contribute by updating KB to clarify that. See WLED-Docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willmmiles willmmiles willmmiles left review comments

@JPZV JPZV JPZV requested changes

@Aircoookie Aircoookie Awaiting requested review from Aircoookie

@softhack007 softhack007 Awaiting requested review from softhack007

Assignees

@blazoncek blazoncek

Labels
connectivity Issue regarding protocols, WiFi connection or availability of interfaces enhancement
Projects
None yet
Milestone
0.15.0-alpha candidate
Development

Successfully merging this pull request may close these issues.
5 participants
@blazoncek @JPZV @jasmohan-narula @mellbergsimon @willmmiles