Threading API, only stack allocation for now

[antoinealb]

I am working on the threading API ( #9 ), for now I only thought about basic functionalities (start / delete a thread and allocate a stack). See the README for details.

I will start working on the code now.

[antoinealb]

I added the allocators (static and dynamic) for the thread stacks.

[antoinealb]

Apparently Travis detects an error that I cannot reproduce on my machine. Spinning up a new VM to see if the problem can be reproduced.

[antoinealb]

Ok, the Travis CI error was because designated initializers (a la C99) were only supported in G++ >= 4.7 and Travis still runs a GCC 4.6 by default. I removed the designated initializers and it should pass now.

edit : yay, build is passing. edit2 : yay, build 42.

[antoinealb]

Well in fact, I think we could already merge this and develop the rest of the proposed API in another PR.

[Stapelzeiger]

Do we really want that much complexity for the stacks? We could just have a stacksize argument to the thread create function. Then we could remove the macro, the custom stack type, the alloc and the delete function. I think this would make the code simpler and cleaner.

[antoinealb]

If we want static allocation of stacks (as in link time checking), some complexity is required. And I prefer to have a macro that handles stack size for me rather than recall to change stack size in many different places.

edit : also, this enforces a "standard" way of allocating a stack rather than just "anyone declares their array".

[pierluca]

LGTM in this case, but as a rule of thumb: CanAccessMiddleOfStack seems marginally relevant to me. Test first and last item to check for off-by-one errors.

[Stapelzeiger]

What about the sizeof macro? I think the current implementation adds a lot of code/indirectness only to economize one parameter. I would prefer to keep code as simple as possible. Inventing a new wrapper for every buffer makes code less readable and passing the buffer length as parameter (with use of sizeof) is just the C way pass a buffer. About the standard way to allocate stacks: we could define a macro/type for the stack, like in uC/OS-II. I think this solution is transparent and sufficiently convenient to use.

[antoinealb]

You cannot sizeofdynamically allocated memory. The indirection layer comes from the fact that we indeed have two different implementations that need to be abstracted : one where you allocate your memory statically, and the other where you do dynamic allocation.

edit : plus, if you read the example in the README, you will see that this is not very different from just having a macro/type to do stack allocation. The only case where you have to care about the new type is when doing dynamically allocating threads, which would be specific in any case.

[Stapelzeiger]

I've read the example in the readme, you have a macro that looks like a function call, but must be put outside a function context. It's just not very transparent. for the dynamic case, if you really think having a size paramter is a problem: why not have two functions for task creation; a static and a dynamic one. Or alternatively one function that allocates the stack if the buffer-argument is NULL. In general I don't like putting buffers and their size in a struct, especially if it serves only for parameter passing.

[froj]

If the problem is the struct wrapping the buffer, then I see the possibility to split statically and dynamically allocated stacks like this:

/* static stack */
THREAD_STACK(mystack, 2048);
thread_create(mythread, &mystack, sizeof(mystack), myprio, NULL);

and

thread_create_with_dynamic_stack(mythread, mystack_size, myprio, NULL);

[Stapelzeiger]

If you get rid of the wrapper you no longer need the macro. we could replace THREAD_STACK(mystack, 2048); with something like THREAD_STACK mystack[2048];, which is more transparent IMO.

[pierluca]

THREAD_STACK(mystack, 2048); has one advantage, if we have to change for one reason or another the underlying implementation, it's much easier to do than with THREAD_STACK mystack[2048]; And yes, sizeof is a compile-time operator. The code is rather simple, I really don't think it's over-engineered. The only thing I would do is rename THREAD_STACK in order to make it exceedingly clear that it's for static allocation when using autocomplete :-)

[antoinealb]

The other advantage I see when using a custom data type which encapsulates data is that it makes it harder for you to shoot yourself in the foot. Once the allocator is tested, you know it works. If you pass buffer and size separately, you could make mistakes.

[antoinealb]

Oh and one more argument for the struct is that when the threading module needs to free the thread stack when exiting a given thread, it has to check if the stack was statically or dynamically allocated and behave differently.

[msplr]

The other advantage I see when using a custom data type which encapsulates data is that it makes it harder for you to shoot yourself in the foot. Once the allocator is tested, you know it works. If you pass buffer and size separately, you could make mistakes.

I don't see the connection between the encapsulation of the stack and the stack allocator. The allocator is a separate piece of code and can be tested independently.

int thread_create(int(*func)(void*), void *stack, size_t stack_size, int prio, void *arg);

I think the stack argument should be a void pointer, because the stack is cpu specific and has no types. Our C code should not bother about data types in the stack memory. (In the application code the stack argument will be directly passed on to some low level C/assembly code anyway.) Also, I think we don't need a mock implementation of stacks for testing. So I don't see an advantage in packing the statically allocated stack into a struct.

I'm for something like this:

/* static stack */
THREAD_STACK mystack[2048];
thread_create(mythread, mystack, sizeof(mystack), myprio, NULL);

/* dynamically allocated stack */
thread_create(mythread, myprio, NULL, 1024, NULL);

And also: Where and how is the thread struct allocated? It might be useful to pack the uC/OS OS_TCB into a struct with additional info about the thread (like if the stack needs to be freed after exit).

[antoinealb]

I have started from scratch in another pull request (#23) taking in account your remarks. I am closing this pull request.