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

Automatic stack location selection (SYS or HEAP), enable per library AR-chive in arduino build system #5018

Merged
merged 20 commits into from
Aug 20, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions cores/esp8266/cont.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ typedef struct cont_ {
unsigned* struct_start;
} cont_t;

/* Not static, used in core_esp8266_postmortem.c.
* Placed into noinit section because we assign value to this variable
* before .bss is zero-filled, and need to preserve the value.
*/
extern cont_t* g_pcont __attribute__((section(".noinit")));
Copy link
Member

Choose a reason for hiding this comment

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

__attribute__ seems to be needed only on definition, not on declaration. Details about placement into noinit section can live there as well.


// Initialize the cont_t structure before calling cont_run
void cont_init(cont_t*);

Expand Down
38 changes: 38 additions & 0 deletions cores/esp8266/core_esp8266_app_entry_noextra4k.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
* This is the original app_entry() not providing extra 4K heap, but allowing
* the use of WPS.
*
* see comments in core_esp8266_main.cpp's app_entry()
*
*/

#include <c_types.h>
#include "cont.h"
#include "coredecls.h"

// calls to this function must *always* be inlined
Copy link
Member

Choose a reason for hiding this comment

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

must never be inlined?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was meaning: callers to this function must *always* be inlined

void disable_extra4k_at_link_time (void)
{
/*
* does nothing
* allows overriding the core_esp8266_main.cpp's app_entry()
* by this one below, at link time
*
*/
}

/* the following code is linked only if a call to the above function is made somewhere */

extern "C" void call_user_start();

/* this is the default NONOS-SDK user's heap location */
static cont_t g_cont __attribute__ ((aligned (16)));

extern "C" void ICACHE_RAM_ATTR app_entry_redefinable(void)
{
/* this is the default NONOS-SDK user's heap location */
g_pcont = &g_cont;

/* Call the entry point of the SDK code. */
call_user_start();
}
37 changes: 18 additions & 19 deletions cores/esp8266/core_esp8266_main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -175,10 +175,15 @@ void init_done() {

WPS beeing flawed by its poor security, or not beeing used by lots of
users, it has been decided that we are still going to use that memory for
user's stack and disable the use of WPS, with an option to revert that
back at the user's discretion. This selection can be done with the
global define NO_EXTRA_4K_HEAP. An option has been added to the board
generator script.
user's stack and disable the use of WPS.

app_entry() jumps to app_entry_custom() defined as "weakref" calling
itself a weak customizable function, allowing to use another one when
this is required (see core_esp8266_app_entry_noextra4k.cpp, used by WPS).

(note: setting app_entry() itself as "weak" is not sufficient and always
ends up with the other "noextra4k" one linked, maybe because it has a
default value in linker scripts).

References:
https://github.com/esp8266/Arduino/pull/4553
Expand All @@ -188,31 +193,25 @@ void init_done() {

*/

#ifdef NO_EXTRA_4K_HEAP
/* this is the default NONOS-SDK user's heap location */
cont_t g_cont __attribute__ ((aligned (16)));
#endif

extern "C" void ICACHE_RAM_ATTR app_entry(void)
extern "C" void ICACHE_RAM_ATTR app_entry_redefinable(void) __attribute__((weak));
extern "C" void ICACHE_RAM_ATTR app_entry_redefinable(void)
{
#ifdef NO_EXTRA_4K_HEAP

/* this is the default NONOS-SDK user's heap location */
g_pcont = &g_cont;

#else

/* Allocate continuation context on this SYS stack,
and save pointer to it. */
cont_t s_cont __attribute__((aligned(16)));
g_pcont = &s_cont;

#endif

/* Call the entry point of the SDK code. */
call_user_start();
}

static void ICACHE_RAM_ATTR app_entry_custom (void) __attribute__((weakref("app_entry_redefinable")));

extern "C" void ICACHE_RAM_ATTR app_entry (void)
Copy link
Member

Choose a reason for hiding this comment

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

Can we drop this extra call level?

In this file, you define:

extern "C" void ICACHE_RAM_ATTR app_entry_sysstack(void)
{
    cont_t s_cont __attribute__((aligned(16)));
    g_pcont = &s_cont;
    call_user_start();
}
extern "C" void app_entry(void) __attribute__((weak, alias("app_entry_sysstack")));

In the other file, you define app_entry as a strong symbol:

extern "C" void ICACHE_RAM_ATTR app_entry(void)
{
    g_pcont = &g_cont;
    call_user_start();
}

I think it should work...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried that, and just re-tried your way (weak+alias instead of weakref) and it still does not work, the strong one is always used. I don't know why for sure.

I had put that comment just above about that.

(note: setting app_entry() itself as "weak" is not sufficient and always ends up with the other "noextra4k" one linked, maybe because it has a default value in linker scripts).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you are right. That's a side effect of specifying something as ENTRY, it also makes the symbol undefined, and the linker picks it up from core_esp8266_app_entry_noextra4k.cpp before it sees core_esp8266_main.cpp (probably due to alphabetical ordering when creating the archive file)...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried again (after dot_a_linkage, just to be sure), still the same. The indirection is still needed.

Copy link
Member

Choose a reason for hiding this comment

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

No problem, that only cost us 16 bytes of the stack, which we can later remove by re-writing app_entry as a one-line assembly function which call0-s app_entry_redefinable.

{
return app_entry_custom();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we call app_entry_redefinable() directly here? or is that what is meant about the extra level of indirection in the comments above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See comment above: tried that 3 times with different context with no luck. It is because (maybe) app_entry is defined as an ENTRY in ldscripts.

tools/sdk/ld/eagle.app.v6.common.ld.h:ENTRY(app_entry)

}

extern "C" void user_init(void) {
struct rst_info *rtc_info_ptr = system_get_rst_info();
memcpy((void *) &resetInfo, (void *) rtc_info_ptr, sizeof(resetInfo));
Expand Down
5 changes: 5 additions & 0 deletions cores/esp8266/coredecls.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,14 @@ extern "C" {

extern bool timeshift64_is_set;

void esp_yield();
void esp_schedule();
void tune_timeshift64 (uint64_t now_us);
void settimeofday_cb (void (*cb)(void));

// calls to this function must *always* be inlined
void disable_extra4k_at_link_time (void) __attribute__((noinline));

#ifdef __cplusplus
}
#endif
Expand Down
2 changes: 0 additions & 2 deletions doc/faq/a05-board-generator.rst
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,6 @@ As of today you can:

* increase available flash space by disabling floats in ``*printf`` functions

* enable WPS which is now disabled by default (at the cost of a smaller heap by ~4KB)

* change led pin ``LED_BUILTIN`` for the two generic boards

* change the default lwIP version (1.4 or 2)
Expand Down
16 changes: 10 additions & 6 deletions doc/faq/readme.rst
Original file line number Diff line number Diff line change
Expand Up @@ -46,22 +46,26 @@ How can I get some extra KBs in flash ?
* Using ``*printf()`` with floats is enabled by default. Some KBs of flash can
be saved by using the option ``--nofloat`` with the boards generator:

``./tools/boards.txt.py --nofloat --allgen``
``./tools/boards.txt.py --nofloat --boardsgen``

* Use the debug level option ``NoAssert-NDEBUG`` (in the Tools menu)

`Read more <a05-board-generator.rst>`__.

Why can't I use WPS ?
~~~~~~~~~~~~~~~~~~~~~
About WPS
~~~~~~~~~

WPS is disabled by default, this offers an extra 4KB in ram/heap. To enable
WPS (and lose 4KB of useable ram), use this boards generator option:
In release 2.4.2 only, WPS is disabled by default. To enable WPS, use this
boards generator option:

``./tools/boards.txt.py --allowWPS --allgen``
``./tools/boards.txt.py --allowWPS --boardsgen``

`Read more <a05-board-generator.rst>`__.

From release 2.4.2 and ahead, using WPS will reduce available heap space to
user by around 4.5KB. In other words, from release 2.4.2 without using WPS,
Copy link
Member

Choose a reason for hiding this comment

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

Suggest rewording this to make it clear that this is not yet available in 2.4.2. E.g. "starting from 2.5.0, ...".

an extra ~4.5KB is available in user's heap space.

This Arduino library doesn't work on ESP. How do I make it work?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Expand Down
84 changes: 84 additions & 0 deletions libraries/ESP8266WiFi/src/ESP8266WiFiSTA-WPS.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@

Copy link
Member

Choose a reason for hiding this comment

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

LGPL license missing

#include "ESP8266WiFi.h"
#include "ESP8266WiFiGeneric.h"
#include "ESP8266WiFiSTA.h"

void wifi_wps_status_cb(wps_cb_status status);

/**
* WPS config
* so far only WPS_TYPE_PBC is supported (SDK 1.2.0)
* @return ok
*/
bool beginWPSConfig(void) {
Copy link
Member

@igrr igrr Aug 8, 2018

Choose a reason for hiding this comment

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

You can keep this a member function of ESP8266WiFiSTAClass, just move implementation to a separate file, as you did.


if(!WiFi.enableSTA(true)) {
// enable STA failed
return false;
}

WiFi.disconnect();

DEBUGV("wps begin\n");

if(!wifi_wps_disable()) {
DEBUGV("wps disable failed\n");
return false;
}

// so far only WPS_TYPE_PBC is supported (SDK 1.2.0)
if(!wifi_wps_enable(WPS_TYPE_PBC)) {
DEBUGV("wps enable failed\n");
return false;
}

if(!wifi_set_wps_cb((wps_st_cb_t) &wifi_wps_status_cb)) {
DEBUGV("wps cb failed\n");
return false;
}

if(!wifi_wps_start()) {
DEBUGV("wps start failed\n");
return false;
}

esp_yield();
// will return here when wifi_wps_status_cb fires

return true;
}

/**
* WPS callback
* @param status wps_cb_status
*/
void wifi_wps_status_cb(wps_cb_status status) {
DEBUGV("wps cb status: %d\r\n", status);
switch(status) {
case WPS_CB_ST_SUCCESS:
if(!wifi_wps_disable()) {
DEBUGV("wps disable failed\n");
}
wifi_station_connect();
break;
case WPS_CB_ST_FAILED:
DEBUGV("wps FAILED\n");
break;
case WPS_CB_ST_TIMEOUT:
DEBUGV("wps TIMEOUT\n");
break;
case WPS_CB_ST_WEP:
DEBUGV("wps WEP\n");
break;
case WPS_CB_ST_UNK:
DEBUGV("wps UNKNOWN\n");
if(!wifi_wps_disable()) {
DEBUGV("wps disable failed\n");
}
break;
}
// TODO user function to get status

esp_schedule(); // resume the beginWPSConfig function
}

84 changes: 0 additions & 84 deletions libraries/ESP8266WiFi/src/ESP8266WiFiSTA.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -571,90 +571,6 @@ int32_t ESP8266WiFiSTAClass::RSSI(void) {
// -------------------------------------------------- STA remote configure -----------------------------------------------
// -----------------------------------------------------------------------------------------------------------------------

#ifdef NO_EXTRA_4K_HEAP
/* NO_EXTRA_4K_HEAP's description in cores/esp8266/core_esp8266_main.cpp */

void wifi_wps_status_cb(wps_cb_status status);

/**
* WPS config
* so far only WPS_TYPE_PBC is supported (SDK 1.2.0)
* @return ok
*/
bool ESP8266WiFiSTAClass::beginWPSConfig(void) {

if(!WiFi.enableSTA(true)) {
// enable STA failed
return false;
}

disconnect();

DEBUGV("wps begin\n");

if(!wifi_wps_disable()) {
DEBUGV("wps disable failed\n");
return false;
}

// so far only WPS_TYPE_PBC is supported (SDK 1.2.0)
if(!wifi_wps_enable(WPS_TYPE_PBC)) {
DEBUGV("wps enable failed\n");
return false;
}

if(!wifi_set_wps_cb((wps_st_cb_t) &wifi_wps_status_cb)) {
DEBUGV("wps cb failed\n");
return false;
}

if(!wifi_wps_start()) {
DEBUGV("wps start failed\n");
return false;
}

esp_yield();
// will return here when wifi_wps_status_cb fires

return true;
}

/**
* WPS callback
* @param status wps_cb_status
*/
void wifi_wps_status_cb(wps_cb_status status) {
DEBUGV("wps cb status: %d\r\n", status);
switch(status) {
case WPS_CB_ST_SUCCESS:
if(!wifi_wps_disable()) {
DEBUGV("wps disable failed\n");
}
wifi_station_connect();
break;
case WPS_CB_ST_FAILED:
DEBUGV("wps FAILED\n");
break;
case WPS_CB_ST_TIMEOUT:
DEBUGV("wps TIMEOUT\n");
break;
case WPS_CB_ST_WEP:
DEBUGV("wps WEP\n");
break;
case WPS_CB_ST_UNK:
DEBUGV("wps UNKNOWN\n");
if(!wifi_wps_disable()) {
DEBUGV("wps disable failed\n");
}
break;
}
// TODO user function to get status

esp_schedule(); // resume the beginWPSConfig function
}

#endif // NO_EXTRA_4K_HEAP

bool ESP8266WiFiSTAClass::_smartConfigStarted = false;
bool ESP8266WiFiSTAClass::_smartConfigDone = false;

Expand Down
12 changes: 6 additions & 6 deletions libraries/ESP8266WiFi/src/ESP8266WiFiSTA.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@
#include "ESP8266WiFiType.h"
#include "ESP8266WiFiGeneric.h"
#include "user_interface.h"
#include "coredecls.h" // disable_extra4k_at_link_time()

extern bool beginWPSConfig(void);
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to be a freestanding function?


class ESP8266WiFiSTAClass {
// ----------------------------------------------------------------------------------------------
Expand Down Expand Up @@ -93,13 +95,11 @@ class ESP8266WiFiSTAClass {

public:

#ifdef NO_EXTRA_4K_HEAP
bool beginWPSConfig(void);
#else
inline bool beginWPSConfig(void) __attribute__((always_inline)) {
return WPS_is_unavailable_in_this_configuration__Please_check_FAQ_or_board_generator_tool();
inline bool beginWPSConfig(void) __attribute__((always_inline))
{
disable_extra4k_at_link_time(); // this call must always be inlined
Copy link
Member

Choose a reason for hiding this comment

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

as far as i can tell this can never be inlined, unless we enable LTO?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have updated the code/comment (yet to push) as follow:

    protected:
    
        static void WPSStatusCB(wps_cb_status status);
        bool beginRealWPSConfig(void);

    public:

        inline bool beginWPSConfig(void) __attribute__((always_inline))
        {
            /*
             Some notes:

             This call to disable_extra4k_at_link_time() must not be
             "compiled-in" when WPS is not in use.  Calling it from
             beginRealWPSConfig() would always link it.

             'inline ...  attribute(always_inline)' is not required since gcc
             will optimize this function, but it is left as a reminder.

             Moving beginRealWPSConfig from ESP8266WiFiSTA.cpp to
             ESP8266WiFiSTA-WPS.cpp only allows to same some flash space.
            */
            disable_extra4k_at_link_time();

            return beginRealWPSConfig();
        }

When it is not inlined, or not optimized by gcc, or if disable_extra4k_at_link_time() is called from beginRealWPSConfig(), heap stack is always linked in.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, i finally understood why you were mentioning inlining in other comments and why you needed to make it inline. This is because Arduino IDE does not combine object files from libraries into static archives, it links them directly. This is in contrast with the core object files, which are first placed into an archive.

However, recent Arduino versions support putting library object files into .a archive before linking, which should remove the need for inlining. Please check https://github.com/arduino/Arduino/wiki/Arduino-IDE-1.5:-Library-specification and add dot_a_linkage=true to ESP8266WiFi library.properties file.

Copy link
Collaborator Author

@d-a-v d-a-v Aug 9, 2018

Choose a reason for hiding this comment

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

It works with dot_a_linkage=true and the need for an inline function/method in the class is now useless.

Things are now more clear, thanks for this tip. I was not understanding in the first place why I had to use the inline trick and was wrongly thinking that objects files were stored in a library. I admit I don't often try to read the long-like-hell IDE commands.

But I had to cheat and create an empty archive {tmp}/libraries/ESP8266WiFi/ESP8266WiFi.a at the linker command request, while the build process put everything in {tmp}/arduino.ar.
It seems to be the same bug as this esp32 one. This issue is still present in both latest nightly and latest beta.

edit: now trying to find the arduino environment variable for the AR object
edit2: found it, it's {archive_file_path}. Now arduino.ar no longer exists, digging this out :)
edit3: found this, {...}/arduino.ar needs to be replaced by "{archive_file_path}" "-L{build.path}" (all doc is in your above link)

return ::beginWPSConfig();
}
#endif

bool beginSmartConfig();
bool stopSmartConfig();
Expand Down
4 changes: 2 additions & 2 deletions libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1260,13 +1260,13 @@ bool WiFiClientSecure::loadPrivateKey(Stream& stream, size_t size) {

extern "C" {
#include <cont.h>
extern cont_t g_cont;
extern cont_t *g_pcont;
Copy link
Member

Choose a reason for hiding this comment

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

let's put this into coredecls.h as well

extern size_t br_esp8266_stack_proxy_usage();

void _BearSSLCheckStack(const char *fcn, const char *file, int line) {
static int cnt = 0;
register uint32_t *sp asm("a1");
int freestack = 4 * (sp - g_cont.stack);
int freestack = 4 * (sp - g_pcont->stack);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for keeping this updated.

int freeheap = ESP.getFreeHeap();
static int laststack, lastheap, laststack2;
if ((laststack != freestack) || (lastheap != freeheap) || (laststack2 != (int)br_esp8266_stack_proxy_usage())) {
Expand Down
Loading