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

Renamings for that esp_yield is really suspend, replacing delay(0), shimmable suspend-CONT API for async esp_suspend() / esp_schedule() #7148

Merged
merged 22 commits into from
Oct 16, 2021
Merged

Renamings for that esp_yield is really suspend, replacing delay(0), shimmable suspend-CONT API for async esp_suspend() / esp_schedule() #7148

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
4df7186
delay0isyield squash commit.
dok-net Jan 1, 2021
b64622d
_notifyPWM may be called indirectly from ISR, cannot yield() from ISR…
dok-net Jan 10, 2021
9bb4894
Fix for 656a33e6f8
dok-net Mar 15, 2021
97d59ae
Consolidate renaming of identifiers to clarify strict meaning of susp…
dok-net Apr 8, 2021
08ed7d0
Non-recurring scheduled functions may yield, therefore run_scheduled_…
dok-net Apr 10, 2021
397408f
Add HAVE_ESP_SUSPEND define to allow 3rd party libraries to detect AP…
dok-net Apr 8, 2021
0bb470c
Merge branch 'esp_yield_mt' into delay0isyield
dok-net Apr 9, 2021
971ad28
Make detectBaudrate safe from SYS context, for what it's worth.
dok-net Apr 19, 2021
99ed6fc
Add comment about how optimistic_yield() is safe for SYS, like in cal…
dok-net Apr 19, 2021
cd8d2d3
Adapt logic to feed scheduled recurrent functions in hostByName from …
dok-net Jul 20, 2021
070eb48
Add clarifying comments to esp_suspend and esp_delay overloads.
dok-net Oct 6, 2021
d11be8b
Refactoring as seen in PR #8317
dok-net Oct 6, 2021
4b92d28
Removed redundant duplicate type definition.
dok-net Oct 7, 2021
208c4a3
Use function template syntax to save using std::function objects.
dok-net Oct 9, 2021
3720ac0
Fix uninitialized status for immediately expired timeout.
dok-net Oct 9, 2021
3be70a4
Specification for try_esp_delay added as code comment.
dok-net Oct 9, 2021
67ced12
Apply suggestions from code review
dok-net Oct 13, 2021
62af940
By review feedback, maintain esp_ as prefix.
dok-net Oct 13, 2021
863c482
Code cleanup as per review feedback.
dok-net Oct 13, 2021
679ecb1
Enable the startWaveform functions for calling from SYS context. Remo…
dok-net Oct 14, 2021
6199eb9
Adopt same code for host-test version of `esp_try_delay`.
dok-net Oct 14, 2021
7cfaeea
Merge branch 'master' into delay0isyield
dok-net Oct 16, 2021
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
8 changes: 3 additions & 5 deletions cores/esp8266/Esp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,20 +115,18 @@ void EspClass::wdtFeed(void)
system_soft_wdt_feed();
}

extern "C" void esp_yield();

void EspClass::deepSleep(uint64_t time_us, WakeMode mode)
{
system_deep_sleep_set_option(static_cast<int>(mode));
system_deep_sleep(time_us);
esp_yield();
esp_suspend();
}

void EspClass::deepSleepInstant(uint64_t time_us, WakeMode mode)
{
system_deep_sleep_set_option(static_cast<int>(mode));
system_deep_sleep_instant(time_us);
esp_yield();
esp_suspend();
}

//this calculation was taken verbatim from the SDK api reference for SDK 2.1.0.
Expand Down Expand Up @@ -200,7 +198,7 @@ void EspClass::reset(void)
void EspClass::restart(void)
{
system_restart();
esp_yield();
esp_suspend();
}

[[noreturn]] void EspClass::rebootIntoUartDownloadMode()
Expand Down
2 changes: 1 addition & 1 deletion cores/esp8266/HardwareSerial.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ unsigned long HardwareSerial::detectBaudrate(time_t timeoutMillis)
if ((detectedBaudrate = testBaudrate())) {
break;
}
delay(100);
esp_delay(100);
}
return detectedBaudrate;
}
Expand Down
3 changes: 2 additions & 1 deletion cores/esp8266/PolledTimeout.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <limits> // std::numeric_limits
#include <type_traits> // std::is_unsigned
#include <core_esp8266_features.h>
#include <coredecls.h>

namespace esp8266
{
Expand All @@ -45,7 +46,7 @@ struct DoNothing

struct YieldOrSkip
{
static void execute() {delay(0);}
static void execute() {esp_yield();}
};

template <unsigned long delayMs>
Expand Down
18 changes: 7 additions & 11 deletions cores/esp8266/Schedule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,6 @@ bool schedule_recurrent_function_us(const std::function<bool(void)>& fn,

void run_scheduled_functions()
{
esp8266::polledTimeout::periodicFastMs yieldNow(100); // yield every 100ms

// prevent scheduling of new functions during this run
auto stop = sLast;
bool done = false;
Expand All @@ -161,13 +159,10 @@ void run_scheduled_functions()
recycle_fn_unsafe(to_recycle);
}

if (yieldNow)
{
// because scheduled functions might last too long for watchdog etc,
// this is yield() in cont stack:
esp_schedule();
cont_yield(g_pcont);
}
// scheduled functions might last too long for watchdog etc.
// yield() is allowed in scheduled functions, therefore
// recursion into run_scheduled_recurrent_functions() is permitted
optimistic_yield(100000);
mcspr marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down Expand Up @@ -241,9 +236,10 @@ void run_scheduled_recurrent_functions()
if (yieldNow)
{
// because scheduled functions might last too long for watchdog etc,
// this is yield() in cont stack:
// this is yield() in cont stack, but need to call cont_suspend directly
// to prevent recursion into run_scheduled_recurrent_functions()
esp_schedule();
cont_yield(g_pcont);
cont_suspend(g_pcont);
}
} while (current && !done);

Expand Down
20 changes: 10 additions & 10 deletions cores/esp8266/cont.S
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@
.section .irom0.text
.align 4
.literal_position
.global cont_yield
.type cont_yield, @function
cont_yield:
.global cont_suspend
.type cont_suspend, @function
cont_suspend:
/* a1: sp */
/* a2: void* cont_ctx */
/* adjust stack and save registers */
Expand All @@ -35,10 +35,10 @@ cont_yield:
s32i a0, a1, 16
s32i a2, a1, 20

/* &cont_continue -> cont_ctx.pc_yield */
/* &cont_continue -> cont_ctx.pc_suspend */
movi a3, cont_continue
s32i a3, a2, 8
/* sp -> cont_ctx.sp_yield */
/* sp -> cont_ctx.sp_suspend */
s32i a1, a2, 12

/* a0 <- cont_ctx.pc_ret */
Expand All @@ -56,7 +56,7 @@ cont_continue:
l32i a2, a1, 20
addi a1, a1, 24
ret
.size cont_yield, . - cont_yield
.size cont_suspend, . - cont_suspend

////////////////////////////////////////////////////

Expand Down Expand Up @@ -108,7 +108,7 @@ cont_run:
/* sp -> cont_ctx.sp_ret */
s32i a1, a2, 4

/* if cont_ctx.pc_yield != 0, goto cont_resume */
/* if cont_ctx.pc_suspend != 0, goto cont_resume */
l32i a4, a2, 8
bnez a4, cont_resume
/* else */
Expand All @@ -119,12 +119,12 @@ cont_run:
jx a2

cont_resume:
/* a1 <- cont_ctx.sp_yield */
/* a1 <- cont_ctx.sp_suspend */
l32i a1, a2, 12
/* reset yield flag, 0 -> cont_ctx.pc_yield */
/* reset yield flag, 0 -> cont_ctx.pc_suspend */
movi a3, 0
s32i a3, a2, 8
/* jump to saved cont_ctx.pc_yield */
/* jump to saved cont_ctx.pc_suspend */
movi a0, cont_ret
jx a4

Expand Down
12 changes: 6 additions & 6 deletions cores/esp8266/cont.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ typedef struct cont_ {
void (*pc_ret)(void);
unsigned* sp_ret;

void (*pc_yield)(void);
unsigned* sp_yield;
void (*pc_suspend)(void);
unsigned* sp_suspend;

unsigned* stack_end;
unsigned unused1;
Expand All @@ -55,12 +55,12 @@ extern cont_t* g_pcont;
void cont_init(cont_t*);

// Run function pfn in a separate stack, or continue execution
// at the point where cont_yield was called
// at the point where cont_suspend was called
void cont_run(cont_t*, void (*pfn)(void));

// Return to the point where cont_run was called, saving the
// execution state (registers and stack)
void cont_yield(cont_t*);
void cont_suspend(cont_t*);

// Check guard bytes around the stack. Return 0 in case everything is ok,
// return 1 if guard bytes were overwritten.
Expand All @@ -70,9 +70,9 @@ int cont_check(cont_t* cont);
// and thus weren't used by the user code. i.e. that stack space is free. (high water mark)
int cont_get_free_stack(cont_t* cont);

// Check if yield() may be called. Returns true if we are running inside
// Check if cont_suspend() may be called. Returns true if we are running inside
// continuation stack
bool cont_can_yield(cont_t* cont);
bool cont_can_suspend(cont_t* cont);

// Repaint the stack from the current SP to the end, to allow individual
// routines' stack usages to be calculated by re-painting, checking current
Expand Down
4 changes: 2 additions & 2 deletions cores/esp8266/cont_util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ int cont_get_free_stack(cont_t* cont) {
return freeWords * 4;
}

bool IRAM_ATTR cont_can_yield(cont_t* cont) {
bool IRAM_ATTR cont_can_suspend(cont_t* cont) {
return !ETS_INTR_WITHINISR() &&
cont->pc_ret != 0 && cont->pc_yield == 0;
cont->pc_ret != 0 && cont->pc_suspend == 0;
}

// No need for this to be in IRAM, not expected to be IRQ called
Expand Down
77 changes: 61 additions & 16 deletions cores/esp8266/core_esp8266_main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ cont_t* g_pcont __attribute__((section(".noinit")));
static os_event_t s_loop_queue[LOOP_QUEUE_SIZE];

/* Used to implement optimistic_yield */
static uint32_t s_cycles_at_yield_start;
static uint32_t s_cycles_at_resume;

/* For ets_intr_lock_nest / ets_intr_unlock_nest
* Max nesting seen by SDK so far is 2.
Expand All @@ -80,6 +80,10 @@ const char* core_release =
#else
NULL;
#endif

static os_timer_t delay_timer;
#define ONCE 0
#define REPEAT 1
} // extern "C"

void initVariant() __attribute__((weak));
Expand All @@ -106,32 +110,71 @@ extern "C" void __preloop_update_frequency() {
extern "C" void preloop_update_frequency() __attribute__((weak, alias("__preloop_update_frequency")));

extern "C" bool can_yield() {
return cont_can_yield(g_pcont);
return cont_can_suspend(g_pcont);
}

static inline void esp_yield_within_cont() __attribute__((always_inline));
static void esp_yield_within_cont() {
cont_yield(g_pcont);
s_cycles_at_yield_start = ESP.getCycleCount();
static inline void esp_suspend_within_cont() __attribute__((always_inline));
static void esp_suspend_within_cont() {
cont_suspend(g_pcont);
s_cycles_at_resume = ESP.getCycleCount();
run_scheduled_recurrent_functions();
}

extern "C" void __esp_yield() {
if (can_yield()) {
esp_yield_within_cont();
extern "C" void __esp_suspend() {
if (cont_can_suspend(g_pcont)) {
esp_suspend_within_cont();
}
}

extern "C" void esp_yield() __attribute__ ((weak, alias("__esp_yield")));
extern "C" void esp_suspend() __attribute__ ((weak, alias("__esp_suspend")));

extern "C" IRAM_ATTR void esp_schedule() {
ets_post(LOOP_TASK_PRIORITY, 0, 0);
}

// Replacement for delay(0). In CONT, same as yield(). Whereas yield() panics
// in SYS, esp_yield() is safe to call and only schedules CONT. Use yield()
// whereever only called from CONT, use esp_yield() if code is called from SYS
// or both CONT and SYS.
extern "C" void esp_yield() {
esp_schedule();
esp_suspend();
}

void delay_end(void* arg) {
(void)arg;
esp_schedule();
}

extern "C" void __esp_delay(unsigned long ms) {
if (ms) {
os_timer_setfn(&delay_timer, (os_timer_func_t*)&delay_end, 0);
os_timer_arm(&delay_timer, ms, ONCE);
}
else {
esp_schedule();
}
esp_suspend();
if (ms) {
os_timer_disarm(&delay_timer);
}
}

extern "C" void esp_delay(unsigned long ms) __attribute__((weak, alias("__esp_delay")));

bool esp_try_delay(const uint32_t start_ms, const uint32_t timeout_ms, const uint32_t intvl_ms) {
uint32_t expired = millis() - start_ms;
if (expired >= timeout_ms) {
return true;
}
esp_delay(std::min((timeout_ms - expired), intvl_ms));
return false;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about commenting the choice of this name (is it interrupting tasks calling (delay()) from cont stack) and also that it is equivalent to delay(0) ?

Copy link
Collaborator

@d-a-v d-a-v Jan 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If new function names are to be added, with restrictions on usage, like "use esp_break() if code is called from SYS", I suggest adding _from_sys in its name to make it clear.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yield() historically panics on purpose when it is called from SYS.
esp_break() is exactly yield() without panic(), callable from both cont and sys.
Considering that the yield()'s panic() is avoided by delay(0) or its new flavour esp_break(), then we may just update yield() to not panic no ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what happened, but you must not have available the current sources somehow.
The actual comment now states, I think I adapted it after an earlier discussion:

// Replacement for delay(0). In CONT, same as yield(). Whereas yield() panics
// in SYS, esp_break() is safe to call and only schedules CONT. Use yield()
// whereever only called from CONT, use esp_break() if code is called from SYS
// or both CONT and SYS.

Meaning, esp_break() is intended for both SYS and CONT.
With regard to yield() panicking in SYS, whereas delay(0) does not and esp_break() of course does neither, I think this was discussed and it was stated that yield() shall intentionally continue panicking when called from anywhere else but CONT.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I forgot to disable my previous answer before the last one. Sorry that this confused you.

So you propose esp_break() for both sys and cont because yield() is for cont only, while they both do the same job, like delay(0) which currently is used to replace yield() everywhere where it is needed for sys and cont.
Then, you replace delay(0) by the new sys+cont yield() taste.
So we have now yield(), delay(0), esp_break() ?
Why not simply yield() with a comment ?

If the earlier discussion you refer to is that one, it also says:

We could fix it for a major release

@devyte maybe you can elaborate on this:

The expressibility issue has been discussed before (a long time ago shortly after I arrived). It was deemed valid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@d-a-v Thanks, one can never look too many times at the code. I'm going to explain next that it's a bit different than you think, but that wasn't helped by me replacing delay(0) by yield() on time too often - instead of by esp_break() (5c39094).

Then, you replace delay(0) by the new sys+cont yield() taste.
So we have now yield(), delay(0), esp_break() ?
Why not simply yield() with a comment ?

Delay(0) is either pointless, in those places where the code runs only ever from CONT, so for final clarity and to let the runtime panic to express that contract, I'm replacing it by yield(). Again, only in those places.
Wherever code may run from SYS (and perhaps from CONT, too), the new esp_break() must be used to replace delay(0). This is to make the intention clear not to panic in SYS, which I find obfuscated by a zero time delay(0) call.
As you can see, yield() (intentional panic in SYS) is not the same as esp_break(), and my reservations about delay(0) I've explained above.

extern "C" void __yield() {
if (can_yield()) {
if (cont_can_suspend(g_pcont)) {
esp_schedule();
esp_yield_within_cont();
esp_suspend_within_cont();
}
else {
panic();
Expand All @@ -140,14 +183,17 @@ extern "C" void __yield() {

extern "C" void yield(void) __attribute__ ((weak, alias("__yield")));

// In CONT, actually performs yield() only once the given time interval
// has elapsed since the last time yield() occured. Whereas yield() panics
// in SYS, optimistic_yield() additionally is safe to call and does nothing.
extern "C" void optimistic_yield(uint32_t interval_us) {
const uint32_t intvl_cycles = interval_us *
#if defined(F_CPU)
clockCyclesPerMicrosecond();
#else
ESP.getCpuFreqMHz();
#endif
if ((ESP.getCycleCount() - s_cycles_at_yield_start) > intvl_cycles &&
if ((ESP.getCycleCount() - s_cycles_at_resume) > intvl_cycles &&
can_yield())
{
yield();
Expand Down Expand Up @@ -207,16 +253,16 @@ static void loop_wrapper() {

static void loop_task(os_event_t *events) {
(void) events;
s_cycles_at_yield_start = ESP.getCycleCount();
s_cycles_at_resume = ESP.getCycleCount();
ESP.resetHeap();
cont_run(g_pcont, &loop_wrapper);
ESP.setDramHeap();
if (cont_check(g_pcont) != 0) {
panic();
}
}
extern "C" {

extern "C" {
struct object { long placeholder[ 10 ]; };
void __register_frame_info (const void *begin, struct object *ob);
extern char __eh_frame[];
Expand Down Expand Up @@ -253,7 +299,6 @@ static void __unhandled_exception_cpp()
}
#endif
}

}

void init_done() {
Expand Down
6 changes: 3 additions & 3 deletions cores/esp8266/core_esp8266_postmortem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,12 +245,12 @@ static void print_stack(uint32_t start, uint32_t end) {
}
}

static void uart_write_char_d(char c) {
static void IRAM_ATTR uart_write_char_d(char c) {
uart0_write_char_d(c);
uart1_write_char_d(c);
}

static void uart0_write_char_d(char c) {
static void IRAM_ATTR uart0_write_char_d(char c) {
while (((USS(0) >> USTXC) & 0xff)) { }

if (c == '\n') {
Expand All @@ -259,7 +259,7 @@ static void uart0_write_char_d(char c) {
USF(0) = c;
}

static void uart1_write_char_d(char c) {
static void IRAM_ATTR uart1_write_char_d(char c) {
while (((USS(1) >> USTXC) & 0xff) >= 0x7e) { }

if (c == '\n') {
Expand Down
2 changes: 1 addition & 1 deletion cores/esp8266/core_esp8266_waveform_phase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ int startWaveformClockCycles_weak(uint8_t pin, uint32_t highCcys, uint32_t lowCc
}
std::atomic_thread_fence(std::memory_order_acq_rel);
while (waveform.toSetBits) {
delay(0); // Wait for waveform to update
esp_yield(); // Wait for waveform to update
std::atomic_thread_fence(std::memory_order_acquire);
}
return true;
Expand Down
Loading