Skip to content

Add Access point mode #57

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

Merged
merged 20 commits into from
Aug 19, 2019
Merged

Add Access point mode #57

merged 20 commits into from
Aug 19, 2019

Conversation

mscosti
Copy link
Contributor

@mscosti mscosti commented Jul 7, 2019

  • Adds access point creation methods to esp32spi.py.
    • Puts the ESP32 into AP mode so devices can connect to its own wifi local network.
    • Can specify the SSID and an optional password for the network

Todo

  • More cleanup
  • Fix linting errors + get green build

mscosti and others added 5 commits June 29, 2019 00:37
- channel needed to be in hex byte format.
- Add begining of a server example example program.
- TODO: figure out why cannot create AP w/ passphrase
@ladyada
Copy link
Member

ladyada commented Jul 9, 2019

i think this is a solid start. for making little servers, a 'micro Flask' could be a good way to create the structure for responding to queries. maybe we want to split this into two PRs, one for AP and one for server?

@mscosti
Copy link
Contributor Author

mscosti commented Jul 11, 2019

I think having 2 PRs is fine, one with AP, one with server. The only hesitation I have is that one isn't very useful w/out the other. Does this repo get a new release with every commit/PR, or is release cut manually when we think think its time?

In that case, I'll make this branch and PR just for AP and take out the example code. I'll open a new branch and draft PR for server with what I have now and can continue working on fleshing that out more.

@mscosti mscosti marked this pull request as ready for review July 11, 2019 03:02
@mscosti mscosti changed the title Access point mode + Server methods/example Add Access point mode Jul 11, 2019
@ladyada
Copy link
Member

ladyada commented Jul 11, 2019

we can make releases whenever. server is useful without AP (but agree AP is not useful without server) so lets do server first :)

@mscosti
Copy link
Contributor Author

mscosti commented Jul 12, 2019

Good to know. Server PR still has a lot more work to go into it, but I think this PR should be ready to go whenever we want, just because it's a simpler and more understood.

We can sit on it until Server PR is merged.

Copy link
Member

@brentru brentru left a comment

Choose a reason for hiding this comment

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

Looks promising (and something I'd use for a few of the CircuitPython IOT libraries!). Requesting a few changes..

@@ -409,6 +411,29 @@ def wifi_set_entenable(self):
if resp[0][0] != 1:
raise RuntimeError("Failed to enable enterprise mode")

def wifi_set_ap_network(self, ssid, channel):
Copy link
Member

Choose a reason for hiding this comment

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

Change to wifi_create_ap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

decided to leave this the same to match the name of the command we send to the ESP32 FW.

@mscosti
Copy link
Contributor Author

mscosti commented Jul 16, 2019

Thanks for the feedback @brentru ! I'll try to address your suggestions tomorrow night, unfortunately the only time I have to work on project is in my free time (nights, weekends) and this week I have a bit going on on.

@brentru
Copy link
Member

brentru commented Jul 16, 2019

@mscosti No rush - great work so far!

@mscosti
Copy link
Contributor Author

mscosti commented Jul 21, 2019

@brentru Thanks for the review! I have updated to incorporate your feedback, and responded to some of them.

Copy link
Contributor

@siddacious siddacious left a comment

Choose a reason for hiding this comment

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

Just one little nit pick about disabling the line length lint checker, and a question about the delay time for the timeout. I'll test in a bit.

@mscosti
Copy link
Contributor Author

mscosti commented Jul 23, 2019

I think this PR also has all currently requested changes made, and is ready for potential re-review.

@brentru
Copy link
Member

brentru commented Jul 23, 2019

@mscosti Just tested this, I'd like to suggest some things before we merge:

  • WiFiManager: create_ap should create an access point based on kwargs provided to it, a SSID and Password. I think people will use it this way
    • Providing the AP setup information to create_ap instead of setting it in init.
    • i.e: wifi.create_ap(SSID, PASS)
  • Let's add an example of creating an access point so people don't need to dig through WiFiManager (Add two calls in the example and comment one out. Show creating an insecure access point and one for creating a password-protected access point)

@mscosti
Copy link
Contributor Author

mscosti commented Aug 11, 2019

I fixed a bug, and added AP creation examples to the wsgi server demo code as options to use instead of connecting to wifi.

I also modified the web application to no longer depend on externally hosted static assets, since if you're connected to the esp32 hotspot, you likely don't have a way for your device to download those assets from the internet unless your device has already cached them. Now it will work regardless.

@brentru I haven't updated the method signature for create_AP to take in kwargs for ssid and pass, and still use secrets for getting it.

The reason I think its better to use secrets is that the wifimanager class currently expects a secrets object to be passed to it, and every method in it uses that. If we took in kwargs, then there was no point in passing in secrets to the constructor. I thought about supporting some sort of dual mode where you could use kwargs or secrets, but it felt clunky to me.

Curious what your thoughts are.

@brentru
Copy link
Member

brentru commented Aug 19, 2019

@mscosti Had the chance to test this AM. Works great with each of the AP examples you provided.

@brentru I haven't updated the method signature for create_AP to take in kwargs for ssid and pass, and still use secrets for getting it.

I like how AP creation works in your example - especially since it's contained within the init. for wifimanager as it's near the top of code near hardware initialization.

I thought about supporting some sort of dual mode where you could use kwargs or secrets, but it felt clunky to me.

Agreed, the way you have it right now isn't clunky and we can always add a dual-mode in later.

I'm going to merge this in and release, as you've made siddacious' requested changes too. Thank you for your patience with this PR.

@brentru brentru merged commit ea860b2 into adafruit:master Aug 19, 2019
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Aug 19, 2019
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.

4 participants