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

Attempt at pi cow ap #7101

Merged
merged 15 commits into from
Dec 2, 2022
Merged

Attempt at pi cow ap #7101

merged 15 commits into from
Dec 2, 2022

Conversation

bill88t
Copy link

@bill88t bill88t commented Oct 22, 2022

This is an attempt to implement access point functionallity for the pi cow.
The access point itself works.

However:

  • stop_ap does not work
  • You can connect to an access point successfully even when the ap is still running..
    (This is due to CYW43_LINK_UP)

@anecdata
Copy link
Member

anecdata commented Oct 22, 2022

You can connect to an access point successfully even when the ap is still running.

This is normal in espressif where you can have independent control of whether STA and / or AP are up (if I'm interpreting the statement correctly).

Also, I'm not really clear on how the cyw43 api works, but in connect, why stop_station before connecting? This breaks our working model of connect being a NOP if already connected (again, if I'm interpreting the code correctly).

@bill88t
Copy link
Author

bill88t commented Oct 22, 2022

Oh sorry I didn't phrase it very well.
With the ap open, you can connect to a network and it will connect
But any pings / requests etc. will fail. cyw43 will start crying debug messages.
I just have to fix the stop_ap and call it before a connection.

@anecdata
Copy link
Member

anecdata commented Oct 22, 2022

OK, sorry, missed that this was in draft. I'm excited about the feature :-)

@paul-1
Copy link

paul-1 commented Nov 3, 2022

Outside of running both AP and STA modes at the same time. The AP is working really nice. I have made a few tweaks outside of what @bill88t has done here.

  1. Added wifi channel selection when starting the AP. The interesting thing with this is that the channel can only be set on the first run after cold boot. If you do a warm reset, the radio will not change to a different channel.
  2. Enabled dhcpserver for the AP. This is a compile time option.

I have the changes that I've made here: https://github.com/paul-1/circuitpython/tree/picow-AP-mode-with-DHCP-server

@bill88t
Copy link
Author

bill88t commented Nov 10, 2022

Updated my local branch, cherry-picked the commits from paul-1's fork, thanks @paul-1!
And I am now giving it another try.
As soon as I get the disconnection working, we should be able to merge this.

@paul-1
Copy link

paul-1 commented Nov 10, 2022

Not sure if you or anyone has some advice about the country code commit. I know we are initializing the wifi chip early, so that the digital pins are available for general use. That appears to be the only time we can set the country code. The default wifi country is a global wifi policy.

Perhaps before wifi init we should use getenv to look at see if the user has a valid country code defined in the .env file?

@tannewt
Copy link
Member

tannewt commented Nov 15, 2022

Perhaps before wifi init we should use getenv to look at see if the user has a valid country code defined in the .env file?

That sounds like a good use for .env! 👍

@bill88t
Copy link
Author

bill88t commented Nov 17, 2022

I cannot get the ap to stop no matter what I do..
This is due to cyw43_wifi_leave being borked and doing nothing.

Looking at the respective micropython code we can see, they haven't implemented it either.

I will continue implementing the AP, without stop_ap.
stop_ap will simply raise an appropriate error. Feel free to suggest error messages.
What I propose is NotImplementedError: AP cannot be disconnected.

@anecdata
Copy link
Member

anecdata commented Nov 17, 2022

What happens if start_ap() is called a second time, with different parameters such as ssid or password?

Do we have any API to access cyw43_deinit() to shut it all down and start over (short of a CircuitPython reload)?

@bill88t
Copy link
Author

bill88t commented Nov 17, 2022

What happens if start_ap() is called a second time, with different parameters such as ssid or password?

Stays on original ssid and accepts no connections.
This applies for same ssid, different passwd
different ssid, same passwd
different ssid, different passwd

Do we have any API to access cyw43_deinit() to shut it all down and start over (short of a CircuitPython reload)?

Aren't some pins controlled from the wifi chip?
It would be pretty problematic resetting.
Pretty sure this is the reason it hasn't been implemented so far.

Note: I have not touched any of the early-boot cyw43 code.
I absolutely do not know a thing.

@bill88t
Copy link
Author

bill88t commented Nov 17, 2022

Now, wifi.radio.start_ap, wifi.radio.stop_ap, wifi.radio.connect and wifi.radio.stop_station are ready.
I only now need to do the getters for the ap.

The error messages I added are very much suboptimal. I would very much appreciate some suggestions.

@paul-1
Copy link

paul-1 commented Nov 17, 2022

TBH, I'm not sure there is significant value top stop the AP during a running program. Changing the AP SSID/password, then restarting the code worked fine when I previously tested. I think there are things that just requires the radio to be hard power cycled. For example, once the AP is started the channel assignment will not change without a hard power cycle.

I looked at the error messages. "cannot be stopped" has the connotation that something is preventing it from stopping. IMO, "Stopping AP is not supported" is a better choice.

@anecdata
Copy link
Member

I have an application (on ESP32-S*) that sets up different SSIDs at different times during a run.

We are (hopefully) trying to have a single consistent wifi API for both espressif and raspberrypi, but unfortunately the raspberrypi underpinnings are less mature / less granular, so we'll have some limitations (for now?)

@bill88t
Copy link
Author

bill88t commented Nov 17, 2022

Hopefully as cyw43 is updated, this stuff will be fixed.
However now we are stuck with what we got.
microcontroller.reset() is still an option.

@paul-1
Copy link

paul-1 commented Nov 18, 2022

"Significant Value" was a poor choice of words, on my part. I agree with common api within CP. Since we have to wait for SDK/firmware improvements, I propose we just stick with the current state of the AP development. Cleanup and merge for 8.0.0, and add an open issue or milestone to re-evaluate for future releases.

@bill88t
Copy link
Author

bill88t commented Nov 20, 2022

Actually ap_info is for "the access point we are connected to". So there is nothing more to implement.
Ready for review.

@bill88t bill88t marked this pull request as ready for review November 20, 2022 11:16
Copy link
Member

@anecdata anecdata left a comment

Choose a reason for hiding this comment

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

Differences from espressif, from above and from testing:

  • Station and AP can’t operate simultaneously
    -- if device AP is started, device Station can't connect to an external AP (RuntimeError: Already in access point mode. RuntimeError: Wifi is in access point mode.)
    -- If device Station is connected to an external AP, device AP can't be started (RuntimeError: Already connected to station. RuntimeError: Wifi is in station mode.)
  • once an AP is started, it can't be stopped without a power cycle or microcontroller.reset()
  • AP channel (edit: and other AP settings) can only be set once after a power cycle or microcontroller.reset()
  • wifi.radio.enabled is not implemented (AttributeError: 'Radio' object has no attribute 'enabled'), so to disconnect from an external AP, use wifi.radio.stop_station() --> EDIT: this has now been implemented --> EDIT 2: .enabled isn't fully implemented, but is checked when trying to start a station or AP..

Thanks!

Addendum:

  • wifi country is set to global wifi policy

ports/raspberrypi/common-hal/wifi/Radio.c Outdated Show resolved Hide resolved
@bill88t
Copy link
Author

bill88t commented Nov 21, 2022

  • once an AP is started, it can't be stopped

Or even have its settings changed.
You get one shot. 😅

anecdata
anecdata previously approved these changes Nov 22, 2022
Copy link
Member

@anecdata anecdata left a comment

Choose a reason for hiding this comment

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

Everything I've tried is working as expected. Just ran an adafruit_httpserver on the wifi AP, and connected with a CircuitPython client and got served. I'm going to approve this and defer to one of the core devs to decide to merge. It would be nice to get this out there for more folks to poke at it :-)

@bill88t
Copy link
Author

bill88t commented Nov 23, 2022

1211248 bytes used, 357520 bytes free in flash firmware space out of 1568768 bytes (1532.0kB).
66592 bytes used, 195552 bytes free in ram for stack and heap out of 262144 bytes (256.0kB).

Just in case anyone cares about free board storage.

@dhalbert dhalbert requested a review from jepler November 30, 2022 16:17
@dhalbert
Copy link
Collaborator

@jepler Could you look this over before we merge? Thanks.

@bill88t There are now merge conflicts that need to be resolved.

@bill88t
Copy link
Author

bill88t commented Nov 30, 2022

Built and tested locally.

1259888 bytes used, 308880 bytes free in flash firmware space out of 1568768 bytes (1532.0kB).
69044 bytes used, 193100 bytes free in ram for stack and heap out of 262144 bytes (256.0kB).

One thing I noticed however, these messages pop up whenever a device connects.
DHCPS: client connected: MAC=8a:ab:19:62:52:ad IP=192.168.4.16
The build is not a debug build. I'm pretty sure they should be disabled on regular builds.

@bill88t
Copy link
Author

bill88t commented Nov 30, 2022

shared/netutils/dhcpserver.c:268
Why isn't there 'some' condition? This needs to be silenced in some way..

@jepler
Copy link
Member

jepler commented Nov 30, 2022

perhaps

diff --git a/shared/netutils/dhcpserver.c b/shared/netutils/dhcpserver.c
index a61501c93c..d396a2ba56 100644
--- a/shared/netutils/dhcpserver.c
+++ b/shared/netutils/dhcpserver.c
@@ -265,9 +265,9 @@ static void dhcp_server_process(void *arg, struct udp_pcb *upcb, struct pbuf *p,
             d->lease[yi].expiry = (mp_hal_ticks_ms() + DEFAULT_LEASE_TIME_S * 1000) >> 16;
             dhcp_msg.yiaddr[3] = DHCPS_BASE_IP + yi;
             opt_write_u8(&opt, DHCP_OPT_MSG_TYPE, DHCPACK);
-            printf("DHCPS: client connected: MAC=%02x:%02x:%02x:%02x:%02x:%02x IP=%u.%u.%u.%u\n",
+            LWIP_DEBUGF(DHCP_DEBUG, ("DHCPS: client connected: MAC=%02x:%02x:%02x:%02x:%02x:%02x IP=%u.%u.%u.%u\n",
                 dhcp_msg.chaddr[0], dhcp_msg.chaddr[1], dhcp_msg.chaddr[2], dhcp_msg.chaddr[3], dhcp_msg.chaddr[4], dhcp_msg.chaddr[5],
-                dhcp_msg.yiaddr[0], dhcp_msg.yiaddr[1], dhcp_msg.yiaddr[2], dhcp_msg.yiaddr[3]);
+                dhcp_msg.yiaddr[0], dhcp_msg.yiaddr[1], dhcp_msg.yiaddr[2], dhcp_msg.yiaddr[3]));
             break;
         }
 

@bill88t
Copy link
Author

bill88t commented Nov 30, 2022

Oh I thought it was a submodule. Thankfully it is not.
Imma test it just in case (w & wo/ debug), and we are good to go.

jepler
jepler previously approved these changes Nov 30, 2022
Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! No testing performed.

ports/raspberrypi/Makefile Show resolved Hide resolved
ports/raspberrypi/common-hal/wifi/Radio.c Outdated Show resolved Hide resolved
ports/raspberrypi/common-hal/wifi/Radio.c Show resolved Hide resolved
ports/raspberrypi/supervisor/port.c Show resolved Hide resolved
jepler
jepler previously approved these changes Nov 30, 2022
Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Thank you again!

@bill88t
Copy link
Author

bill88t commented Nov 30, 2022

Now when DEBUG=1, dhcps should announce connections.
This debugging info is useful and shouldn't be barriered behind the header file.
Testing, and I am done with this pr.

@@ -31,6 +31,10 @@
#define DHCPS_BASE_IP (16)
#define DHCPS_MAX_IP (8)

#ifndef DHCP_DEBUG
#define DHCP_DEBUG LWIP_DBG_ON
Copy link
Member

Choose a reason for hiding this comment

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

won't this turn the debug messages back on? is that what you intended?

Copy link
Author

Choose a reason for hiding this comment

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

When DEBUG=0 the user shouldn't see them.
When DEBUG=1 you probably want them.

Copy link
Author

Choose a reason for hiding this comment

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

It builds (w & wo/ debug) but doesn't work though, and I am not sure why.

@bill88t
Copy link
Author

bill88t commented Nov 30, 2022

Idk how to fix it, and it really doesn't matter.
I will continue testing locally, and follow up with more pr's on picow ap.
As much as this one is concerned, done.

Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Thanks! I do not have a strong opinion about that dhcp debug message, it can be fixed in the future with a fresh PR.

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

6 participants