Fix DEBUG_PRINT stack size check

[x3ro]

Checking if the current thread's stacksize is larger than
KERNEL_CONF_STACKSIZE_PRINTF seems like a mistake, and the
check should actually be for larger or equal.

Or have I misunderstood the intention of this check? 😕

[OlegHahm]

See #2183 and #1244 (comment) ;)

[OlegHahm]

You could change this PR to implement my suggestion to get this finally fixed. ;)

[x3ro]

Whoops, sorry for the oversight. Seems like MINIMUM_STACK_SIZE (as suggested in your comment) is not actually used anymore, is it? Should the check then be like this?:

stack_size  >= (KERNEL_CONF_STACKSIZE_DEFAULT + KERNEL_CONF_STACKSIZE_PRINTF)

Looking at the current RIOT master, one could argue that KERNEL_CONF_STACKSIZE_PRINTF should maybe be removed from anything but kernel.h/cpu-conf.h in its entirety, since either it is used like this:

#define NHDP_STACK_SIZE      (KERNEL_CONF_STACKSIZE_DEFAULT + KERNEL_CONF_STACKSIZE_PRINTF)

which is equivalent to

#define NHDP_STACK_SIZE KERNEL_CONF_STACKSIZE_MAIN

or like this:

#define THREAD1_STACKSIZE   (KERNEL_CONF_STACKSIZE_PRINTF)

which, if I have understood the previous discussions correctly, is wrong because this creates a stack that has no room for anything but printf 😄

So, in summary, the check in the debug macro could be "simplified" to

stack_size >= KERNEL_CONF_STACKSIZE_MAIN

right?

Is there a concept of "private constants" in RIOT, e.g. prepending KERNEL_CONF_STACKSIZE_PRINTF with an underscore to indicate that it shouldn't be used?

[OlegHahm]

See also #732

[PeterKietzmann]

#2747 was merged today I guess this is related somehow? Can we close this PR?

[x3ro]

I'm confused. As far as I've understood the past discussion, merging #2747 is a mistake because the STACKSIZE_PRINTF macro is not intended to be used the way the patch implies.

[PeterKietzmann]

Don't know, didn't look inside. I just noticed the happening. @authmillenon, @brummer-simon?

[brummer-simon]

I didn't know about the discussion before i made that pull request. I thought that KERNEL_CONF_STACKSIZE_PRINTF meant a stacksize for a Thread thats using printf.

I didn't know KERNEL_CONF_STACKSIZE_PRINTF is intended for printf alone

[x3ro]

@brummer-simon No sweat :D I made the same mistake just a couple of days ago! But still, I guess it should be reverted and the macro names changed. I'll draft a proposal for that asap.

[miri64]

I did not know of this discussion either >.<

[OlegHahm]

At least it becomes more and more obvious that the name is very misleading.

[x3ro]

So after reading through the (hopefully) relevant parts of the core and the docs, here's my proposal for re-structuring/re-naming the STACKSIZES constants/macros.

First of all, here's the relevant documentation:

The constants for these stack sizes are

• KERNEL_CONF_STACKSIZE_IDLE
• KERNEL_CONF_STACKSIZE_DEFAULT
• KERNEL_CONF_STACKSIZE_PRINTF
• KERNEL_CONF_STACKSIZE_MAIN

and can be used by including kernel.h. ARM based platforms additionally
define KERNEL_CONF_STACKSIZE_PRINTF_FLOAT, because newlibs printf
implementation uses more memory for printing floating point numbers.

KERNEL_CONF_STACKSIZE_IDLE is the stack size for the idle thread and
probably the smallest sensible stack size. KERNEL_CONF_STACKSIZE_DEFAULT
is a default size for any typical thread, not using printf.

KERNEL_CONF_STACKSIZE_PRINTF defines additional stack space needed if the
thread needs to call printf (which requires additional memory when using
newlib). KERNEL_CONF_STACKSIZE_MAIN is the stack size for the main thread
and probably a good size for your application. (Note, that on most
non-newlib dependent platforms this will probably equal
KERNEL_CONF_STACKSIZE_DEFAULT.

So as far as I understand from what I've read and discussed with @OlegHahm, what we currently have is a set of "base stacksizes" such as DEFAULT and IDLE that are used to be directly to create stack buffers.

Then there's a number of "supplemental" stacksizes that are meant to be summed up to these base stacksizes, such as PRINTF and PRINTF_FLOAT. Using only the latter to create a new stack buffer is an error.

E.g.:

// Correct
static char thread_stack[KERNEL_CONF_STACKSIZE_DEFAULT];

// Correct
static char thread_stack[KERNEL_CONF_STACKSIZE_DEFAULT + KERNEL_CONF_STACKSIZE_PRINTF];

// Erroneous
static char thread_stack[KERNEL_CONF_STACKSIZE_PRINTF];

I think that there are two problems with the current naming.

1. The KERNEL_CONF prefix conveys that these macros are to be used in or by the kernel only.
2. The naming along does not indicate that there is a difference between e.g. ...STACKSIZE_PRINTF and ...STACKSIZE_DEFAULT, which IMO contributes to the confusion in Fix DEBUG_PRINT stack size check #2725, debug.h: check stacksize fixed #2747, core: debug: make stack size check more sensible #2183 and Fix check of stack size in debug #1244.

My proposal is thus the following:

1. For base stacksizes: drop the KERNEL_CONF prefix in favor of THREAD.
   • KERNEL_CONF_STACKSIZE_DEFAULT becomes THREAD_STACKSIZE_DEFAULT.
2. For supplemental stacksizes: drop the KERNEL_CONF prefix in favor of EXTRA.
   • KERNEL_CONF_STACKSIZE_PRINTF becomes THREAD_EXTRA_STACKSIZE_PRINTF.

Examples:

static char thread_stack_buffer[THREAD_STACKSIZE_DEFAULT + THREAD_EXTRA_STACKSIZE_PRINTF];

static char thread_stack_buffer[THREAD_STACKSIZE_DEFAULT];

or

static char thread_stack_buffer[THREAD_STACKSIZE_IDLE + THREAD_EXTRA_STACKSIZE_PRINTF];

An erroneous usage would then look as follows, which IMO makes it easier to spot as a wrong usage of the macro:

static char thread_stack_buffer[THREAD_EXTRA_STACKSIZE_PRINTF];

Now you might argue that this results in very long lines. Keep in mind, though, that the current implementation requires:

static char thread_stack_buffer[KERNEL_CONF_STACKSIZE_DEFAULT + KERNEL_CONF_STACKSIZE_PRINTF];

[jnohlgard]

@x3ro 👍 for clarifying the intent of the define by adding _EXTRA_ to the names of the macros

The CONF part of the current naming is probably just to indicate that this is meant to be modified by the user for their own application, which some other macros indeed are not meant to. However, going along these lines then most of the macros in periph_conf.h should also use the word CONF in their names, which I think would look ridiculous and make the application and device driver code less readable.

In summary: I like @x3ro's proposed new naming convention for the stack sizes

[OlegHahm]

I second @gebart. Please update this PR accordingly. But keep in mind that renaming the defines from KERNEL_CONF... to THREAD... requires also a relocating into core/include/thread.h.

[x3ro]

Will do. Thanks for tip!

[OlegHahm]

Any progress?

[x3ro]

Not really 😕 My priority is currently getting the crypto PR merged, to which I will dedicate some time later today. I'll see if I have time for more.

[x3ro]

Superseded by #2881