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.

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

esp8266: Changed esp_scan to keep the current WiFi operating mode but throw an exception if WiFi is in AP only mode #1315

Closed
wants to merge 1 commit into from

Conversation

owens-bill
Copy link
Contributor

While reading modesp.c I discovered that esp.scan() calls wifi_set_opmode to change the operating mode of the radio to 'station mode', disabling the Soft AP function. That appears to be taken from an example in the ESP SDK docs. However, this has two side effects; it changes the operational mode and makes 'station mode' the new bootup default. I changed the function to have it get the current mode (with wifi_get_opmode), then change just the current setting (wifi_set_opmode_current), and change it back afterwards. I also switched from STATION_MODE to the hex literal because I couldn't figure out where STATION_MODE is defined; it seems to be in the SDK somewhere rather than in the MicroPython code, so I wasn't sure whether it was robust against changes to the SDK.

I put this into Issue #1313 and said that I didn't feel comfortable submitting a pull request, but after reading the developer workflow and some docs on rebase, I decided to give it a try ;) Please correct anything I've done wrong, either code or process missteps...

@owens-bill
Copy link
Contributor Author

Went back to using STATION_MODE rather than 0x1, since I found where it's defined in the SDK and see that lots of other, similar constants are being used everywhere ;)

@owens-bill
Copy link
Contributor Author

@pfalcon If this change is worthwhile, should I move it to the new modnetwork.c?

@pfalcon
Copy link
Contributor

pfalcon commented Jun 12, 2015

Sorry, it's in my queue to provide detailed feedback to this PR and associated ticket. In the meantime, quick suggestion I can give: please test how .scan() behaves in AP and STA+AP modes, so you and everyone else knew which exact issues is being fixed here.

@pfalcon
Copy link
Contributor

pfalcon commented Jun 12, 2015

(I mean, try not to set mode to STA before .scan(), in 2 modes above)

@danicampora
Copy link
Member

I think the .scan()function should NOT change to Station mode under the hood. When the WiFi radio is configured in AP mode, it cannot scan for networks, that's correct behaviour, and so, .scan() should either return an empty list (as it does now), or throw an exception, perhaps the latter is better because it makes clear that something is not right. It's the responsibility of the user to select the correct mode before scanning for networks.

@owens-bill
Copy link
Contributor Author

@pfalcon @danicampora I've changed to the approach you suggested; the mode is not changed, but it is checked to see whether the chip is in SOFTAP_MODE, and if so throws an exception.

I'm still wrapping my head around Git so this came out as two commits rather than one; if you'd rather have a single one to keep things neat I'll see if I can figure out how to fix that...

@owens-bill owens-bill changed the title esp8266: Changed esp_scan to preserve WiFi operating mode esp8266: Changed esp_scan to keep the current WiFi operating mode but throw an exception if WiFi is in AP only mode Jun 13, 2015
@dhylands
Copy link
Contributor

Use git rebase -i to squash commits. Here's some more background: https://github.com/ginatrapani/todo.txt-android/wiki/Squash-All-Commits-Related-to-a-Single-Issue-into-a-Single-Commit

… throw an exception if WiFi is in AP only mode
@owens-bill
Copy link
Contributor Author

@dhylands Thanks for that link - I was missing the part about changing 'pick' to 'squash'. I think everything is in order now...

@pfalcon
Copy link
Contributor

pfalcon commented Jun 14, 2015

Merged, thanks!

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.

None yet

4 participants