8000 esp32/modnetwork.c: Fix for isconnected() for static IP config. by glenn20 · Pull Request #3838 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content

esp32/modnetwork.c: Fix for isconnected() for static IP config. #3838

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
Closed
wants to merge 2 commits into from
Closed

esp32/modnetwork.c: Fix for isconnected() for static IP config. #3838

wants to merge 2 commits into from

Conversation

glenn20
Copy link
Contributor
@glenn20 glenn20 commented Jun 4, 2018

Currently <WLAN>.isconnected() always returns True if a static
IP is set, regardless of the state of the connection. This breaks
the commonly documented wifi connection workflow:

    import network
    sta_if = network.WLAN(network.STA_IF)
    sta_if.active(True)
    sta_if.ifconfig(
        ('192.168.1.101', '255.255.255.0', '192.168.1.1', '8.8.8.8'))
    sta_if.connect('ssid', 'password')
    while not sta_if.isconnected():
        pass

This patch introduces a new flag wifi_sta_connected which is set in
event_handler() when GOT_IP event is received and reset when
DISCONNECTED event is received (unless re-connect is successful).
<WLAN>.isconnected() now simply returns the status of this flag (for STA_IF).

The pre-existing flag misleadingly named wifi_sta_connected is also
renamed to wifi_sta_connect_requested.

Fixes issue #3837

@glenn20
esp32/modnetwork.c: Fix for isconnected() for static IP config.
84a76d7 
Currently <WLAN>.isconnected() always returns True if a static
IP is set, regardless of the state of the connection. This breaks
the commonly documented wifi connection workflow:

    import network
    sta_if = network.WLAN(network.STA_IF)
    sta_if.active(True)
    sta_if.ifconfig(
        ('192.168.1.101', '255.255.255.0', '192.168.1.1', '8.8.8.8'))
    sta_if.connect('ssid', 'password')
    while not sta_if.isconnected():
        pass

This patch introduces a new flag 'wifi_sta_connected' which is set in
event_handler() when GOT_IP event is received and reset when
DISCONNECTED event is received (unless re-connect is successful).
isconnected() now simply returns the status of this flag (for STA_IF).

The pre-existing flag misleadingly named 'wifi_sta_connected" is also
renamed to 'wifi_sta_connect_requested'.

Fixes issue #3837
@dpgeorge
Copy link
Member
dpgeorge commented Jun 5, 2018

Thanks for the patch. Is the GOT_IP event fired even if a static IP address is set?

@dpgeorge dpgeorge mentioned this pull request Jun 5, 2018
Closed
@dpgeorge
Copy link
Member
dpgeorge commented Jun 5, 2018

FYI, there is also this older patch: micropython/micropython-esp32#217 which uses the SYSTEM_EVENT_STA_CONNECTED event.

@glenn20
Copy link
Contributor Author
glenn20 commented Jun 5, 2018

Thanks for the quick response.

Yes, the GOT_IP event is fired even for static IP - after connected to AP.

For dhcp I think it is better to wait for GOT_IP, rather than just STA_CONNECTED as the interface is then fully configured. This also means the behaviour is unchanged for dhcp connections.
However, I will check the patch you referenced tomorrow when I am back at my laptop.

@glenn20
Copy link
Contributor Author
glenn20 commented Jun 6, 2018

Thanks Damien. I checked out micropython/micropython-esp32#217. I included the code from that patch which prints the log message for STA_CONNECTED and confirmed that the GOT_IP event may lag the GOT_CONNECTED event by up to 1 second (for completion of DHCP). It is only about 10ms for static IP config.

Accordingly I think it is more robust to test for GOT_IP to ensure the interface is fully configured.

NOTE: I also picked up a logic error in my patch caused by the DISCONNECT event issued as we attempt to connect. I will follow up with another commit to fix that. I also find the log message for GOT_CONNECTED in #217 useful and will include that in my follow up commit.

I am trying to save that 1 second on wake from deepsleep - hence my interest in static IPs

I (526) wifi: STA_START
I (656) wifi: n:1 1, o:1 0, ap:255 255, sta:1 1, prof:1
I (1216) wifi: state: init -> auth (b0)
I (2216) wifi: state: auth -> init (2)
I (2216) wifi: n:1 0, o:1 1, ap:255 255, sta:1 1, prof:1
I (2216) wifi: STA_DISCONNECTED, reason:2
I (2336) wifi: n:1 1, o:1 0, ap:255 255, sta:1 1, prof:1
I (2336) wifi: state: init -> auth (b0)
I (2336) wifi: state: auth -> assoc (0)
I (2346) wifi: state: assoc -> run (10)
I (2376) wifi: connected with Number13b, channel 1
I (2376) wifi: pm start, type: 1
I (2376) network: CONNECTED
I (3386) event: sta ip: 192.168.1.53, mask: 255.255.255.0, gw: 192.168.1.1
I (3386) network: GOT_IP
Connected to: Number13b : config: ('192.168.1.53', '255.255.255.0', '192.168.1.1', '8.8.8.8') i= 629431
MicroPython v1.9.4-116-gdf13ecde-dirty on 2018-06-06; ESP32 module with ESP32

@glenn20
esp32/modenetwork.c: Fix for logic error in pull request #3838.
7058d6b 
Fix for isconnected() logic error triggered by DISCONNECTED event
which is generated before the CONNECTED event on first connect to
wifi.

Also included code from #217 to log CONNECTED events.
@dpgeorge
Copy link
Member
dpgeorge commented Jun 6, 2018

Accordingly I think it is more robust to test for GOT_IP to ensure the interface is fully configured.

Yes I agree. Then this patch will have the same behaviour as before for DHCP (because before it was waiting for a non-zero IP address).

And thanks for the further testing. I think this patch is now good to merge.

I am trying to save that 1 second on wake from deepsleep - hence my interest in static IPs

Yeah, waking and reconnecting can be slow. There is an interesting discussion about that here: espressif/esp-idf#799

@glenn20
Copy link
Contributor Author
glenn20 commented Jun 6, 2018

Thanks for the pointer to the discussion on esp-idf and thanks for accepting for merge. I note that the CI check failed - though I can't work out why. Any tips?

@dpgeorge
Copy link
Member
dpgeorge commented Jun 6, 2018

I note that the CI check failed - though I can't work out why. Any tips?

I'm trying to fix that right now: #3846

@glenn20
Copy link
Contributor Author
glenn20 commented Jun 6, 2018

Excellent - thanks. Busy I see :-).

@dpgeorge
Copy link
Member
dpgeorge commented Jun 8, 2018

Ok, rebased and merged in 039f196

@dpgeorge dpgeorge closed this Jun 8, 2018
@glenn20 glenn20 deleted the esp32-static-ip-isconnected-fix branch June 9, 2018 01:35
dhalbert added a commit to tannewt/circuitpython that referenced this pull request Dec 24, 2020
@dhalbert
Merge pull request micropython#3838 from StackRduino/patch-1
79658ad 
Update devices.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@glenn20 @dpgeorge
0