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.

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

optionally allow WPS #4889

Merged
merged 10 commits into from
Jul 6, 2018
Merged

optionally allow WPS #4889

merged 10 commits into from
Jul 6, 2018

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented Jul 4, 2018

  • Disable WPS at compile time
  • At the price of 4K in heap, allow WPS by using a boards.txt.py option
  • documentation in FAQ

ref: #4779

@d-a-v d-a-v requested review from earlephilhower, igrr and devyte July 4, 2018 11:21
devyte
devyte requested changes Jul 4, 2018
View reviewed changes
tools/sdk/include/user_interface.h Outdated
#define wifi_wps_enable(...) WPS_is_unavailable_in_this_configuration__Please_check_FAQ_or_board_generator_tool()
#define wifi_wps_disable() WPS_is_unavailable_in_this_configuration__Please_check_FAQ_or_board_generator_tool()
#define wifi_wps_start() WPS_is_unavailable_in_this_configuration__Please_check_FAQ_or_board_generator_tool()
#endif

typedef void (*wps_st_cb_t)(int status);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should these 2 lines be moved into the #ifdef ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

cores/esp8266/core_esp8266_main.cpp Outdated
@@ -156,11 +156,25 @@ void init_done() {
* Peripherals (except for SPI0 and UART0) are not initialized.
* This function does not return.
*/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a descriptive comment about what the #define means (explain it's either WPS or stack optimization, but not both), or at least reference the issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would this (above, below) be a clear enough explanation with its references ?

doc/faq/readme.rst Outdated
~~~~~~~~~~~~~~~~~~~~~

WPS is disabled by default, this offers an extra 4KB in ram/heap. To enable
WPS, use this boards generator option:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please state explicitly that enabling WPS removes the 4KB heap optimization.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

libraries/ESP8266WiFi/src/ESP8266WiFiSTA.cpp
@@ -571,6 +571,8 @@ int32_t ESP8266WiFiSTAClass::RSSI(void) {
// -------------------------------------------------- STA remote configure -----------------------------------------------
// -----------------------------------------------------------------------------------------------------------------------

#ifdef NO_EXTRA_4K_HEAP
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a descriptive comment about what the #define means, or at least reference the issue.

libraries/ESP8266WiFi/src/ESP8266WiFiSTA.h
@@ -92,7 +93,13 @@ class ESP8266WiFiSTAClass {

public:

#ifdef NO_EXTRA_4K_HEAP
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a descriptive comment about what the #define means, or at least reference the issue.

tools/sdk/include/user_interface.h Outdated
@@ -575,9 +575,16 @@ enum wps_cb_status {
WPS_CB_ST_UNK,
};

#ifdef NO_EXTRA_4K_HEAP
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a descriptive comment about what the #define means, or at least reference the issue.

@igrr In this case, should the change be moved into a patch that gets applied over the original user_interface.h file? I'm thinking of migration to future sdk versions, and what will happen with our own changes to this file.

@devyte
Copy link
Collaborator

devyte commented Jul 4, 2018

@d-a-v this is pretty good. My sole requests is to add comments to explain what it means. I'm thinking of the future when we come back and look at this, to avoid head scratching/WTFs and so on.

earlephilhower
earlephilhower approved these changes Jul 4, 2018
View reviewed changes
Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

LGTM, one minor suggestion.

While we three know what "no extra 4k heap" means, an end user coming to this from outside wouldn't and would never want less heap. They might want WPS, though. A better name for the #define and build.py parameter might be "enableWPS" (or "disableWPS" if you want to keep same logic polarity in the code now).

@devyte devyte added this to the 2.4.2 milestone Jul 4, 2018
devyte
devyte reviewed Jul 6, 2018
View reviewed changes
doc/faq/readme.rst Outdated
~~~~~~~~~~~~~~~~~~~~~

WPS is disabled by default, this offers an extra 4KB in ram/heap. To enable
WPS (and loose 4KB of useable ram), use this boards generator option:
Copy link
Collaborator

Choose a reason for hiding this comment

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

loose -> lose

devyte
devyte reviewed Jul 6, 2018
View reviewed changes
cores/esp8266/core_esp8266_main.cpp Outdated
/*
A bit of explanation for this entry point:

SYS is the RTOS task/context used by the upperlying system to run its
Copy link
Collaborator

Choose a reason for hiding this comment

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

RTOS task/context -> SDK task context

There is confusion about RTOS and NONOS, I'd prefer to not add to that.

devyte
devyte requested changes Jul 6, 2018
View reviewed changes
Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

Just a couple of typos

@d-a-v d-a-v merged commit e486887 into esp8266:master Jul 6, 2018
@d-a-v d-a-v mentioned this pull request Jul 6, 2018
Closed
liebman added a commit to liebman/WiFiManager that referenced this pull request Jul 7, 2018
@liebman
In esp8266/Arduino#4889 the core was modified to
5a06f29 
disable WPS by default.  Its only enabled if NO_EXTRA_4K_HEAP is defined.
To enable you must re-run the board manager with --noextra4kheap.
This addes #ifdef NO_EXTRA_4K_HEAP arround WPS usage so it won't generate
compiler errors.
@liebman liebman mentioned this pull request Jul 7, 2018
Merged
@arendst arendst mentioned this pull request Jul 14, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@earlephilhower earlephilhower earlephilhower approved these changes

@devyte devyte devyte approved these changes

@igrr igrr Awaiting requested review from igrr

Assignees

@d-a-v d-a-v

Labels
component: core type: bug
Projects
None yet
Milestone
2.4.2
Development

Successfully merging this pull request may close these issues.
3 participants
@d-a-v @devyte @earlephilhower