Skip to content

Resolve #3913: Missing esp32 status() implementation#3942

Closed
mitchins wants to merge 3 commits intomicropython:masterfrom
mitchins:feature/3913_implement_esp32_status
Closed

Resolve #3913: Missing esp32 status() implementation#3942
mitchins wants to merge 3 commits intomicropython:masterfrom
mitchins:feature/3913_implement_esp32_status

Conversation

@mitchins
Copy link
Copy Markdown

Resubmit with cleaner rebase and diff'ing. (GitHub
closed the old one automatically)

Implements the default method (no args) which provides the link status for the STA_IF on ESP32 devices.

Implementation is inspired and consistent with ESP8266, success codes are named similarly, as are most error names. Error codes and names for wifi disconnect are otherwise taken (returned directly) from what the ESP-IDF gives us but are defined in the const dictionary where appropriate.

Success codes are number from 1000 upwards so as not to clash with the 8-bit error codes from ESP-IDF.

Tested connection. failure and success on Wemos R32 device, it seems to return as expected.
wifi_log.txt
Log attached

Define success codes explicitly
Re-use the ESP-IDF codes for errors
@mitchins
Copy link
Copy Markdown
Author

@dpgeorge apologies but GitHub created a new PR as the old one was closed.

Comment thread ports/esp32/modnetwork.c Outdated
if (self->if_id == WIFI_IF_STA) {
// Case of no arg is only for the STA interface
if (wifi_sta_connected) {
return MP_ROM_QSTR(MP_QSTR_STAT_GOT_IP);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You want to return the number, like MP_OBJ_NEW_SMALL_INT(STAT_GOT_IP)

Comment thread ports/esp32/modnetwork.c Outdated
if (wifi_sta_connected) {
return MP_ROM_QSTR(MP_QSTR_STAT_GOT_IP);
} else if (wifi_sta_connect_requested) {
return MP_ROM_QSTR(MP_QSTR_STAT_CONNECTING);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same as above, return STAT_CONNECTING.

Comment thread ports/esp32/modnetwork.c Outdated
} else if (wifi_sta_connect_requested) {
return MP_ROM_QSTR(MP_QSTR_STAT_CONNECTING);
} else {
switch (wifi_sta_disconn_reason) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And here you can just return MP_OBJ_NEW_SMALL_INT(wifi_sta_disconn_reason). Then you don't need any switch statement at all. That was the reason to reuse the constants from the IDF.

@mitchins
Copy link
Copy Markdown
Author

@dpgeorge updated, it seemed to have vanished into a stash. Thanks.

Comment thread ports/esp32/modnetwork.c Outdated
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Atom keeps stripping the ' ' every time I sae the file.

@mitchins mitchins force-pushed the feature/3913_implement_esp32_status branch from c0e213e to 0ca94cd Compare July 13, 2018 09:33
Comment thread ports/esp32/modnetwork.c Outdated
{ MP_ROM_QSTR(MP_QSTR_STAT_WRONG_PASSWORD), MP_ROM_INT(WIFI_REASON_AUTH_FAIL)},
{ MP_ROM_QSTR(MP_QSTR_STAT_ERROR_BEACON_TIMEOUT), MP_ROM_INT(WIFI_REASON_BEACON_TIMEOUT)},
{ MP_ROM_QSTR(MP_QSTR_STAT_ERROR_ASSOC_FAIL), MP_ROM_INT(WIFI_REASON_ASSOC_FAIL)},
{ MP_ROM_QSTR(MP_QSTR_STAT_ERROR_HANDSHAKE_TIMEOUT), MP_ROM_INT(WIFI_REASON_HANDSHAKE_TIMEOUT)},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think for these 3 constants the "ERROR" part is not needed. Simply STAT_ASSOC_FAIL would be enough, etc.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed, it's a little verbose and it's universally inferred to be an error. Will change

@dpgeorge
Copy link
Copy Markdown
Member

Thanks for updating. I squashed the commits and merged them in 385fa51. I changed indentation to match the existing code.

@dpgeorge dpgeorge closed this Jul 14, 2018
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.

2 participants