Check malloc return value

[antoinealb]

In pid_create we should check if malloc returns NULL, but what should we do then ? panic() ? return NULL ?

[Stapelzeiger]

Proper error handling of malloc is very difficult. This i why I wold prefer static allocation. On our motor cards we only have 2k words of ram and we'll probably use 99% of it, so in my opinion malloc is not an option for critical parts of the system.

[antoinealb]

On the other hand we won't be allocating new PIDs every second, so we could hard fail at boot if there is a malloc failure. But using malloc also results in higher fragmentation...

[Stapelzeiger]

The problem I see is that when you decide to add a new PID for some functionality and at boot there is already no more space left, the card crashes and you don't know why. If we allocate it statically the linker can tell you at compile-time that there is not enough RAM. The other problem I have with malloc is that it's not deterministic, since we use a shared pool for all threads. So there can be race conditions where the PID initialization takes a little longer and malloc fails. A solution for this can be single threaded initialization, but it's still one limitation more to keep in mind while coding.

[antoinealb]

The problem is : I don't want anyone fiddling with the internals of the PID especially if we add complex features discussed in other issues. One solution would be the bag of bytes approach where you declare a structure of the same size but without meaningful information.

[Stapelzeiger]

Since it's only us using the module I don't see a problem in declaring the struct in the header. We just have to agree not to access internals directly. This applies to all code and should probably be discussed with all involved.

I'm against the bag of bytes approach because it's a source of errors that are difficult to debug.

[antoinealb]

We have already hidden the internals of the state machine module using malloc, although I agree that this is less critical. Deciding together to only use the API and not touch the internal is a good option too, but we will have to enforce it when doing code review.

[pierluca]

I agree with Patrick, free-standing mallocs in embedded code are kinda bad. AFAIK, there are two approaches to this problem. One is going full-static, which in this kind of code seems reasonable. The other is using a memory pool, I would be surprised if uC/OS-III didn't have such a thing already implemented. They're sometimes called memory partitions. In this case, the memory is managed centrally and it's easier to report on error.

In any case, if we DO want dynamic memory allocation -> failure should systematically produce some debugging output in order to allow us to trace down the problem.

OT: In this sense, we might want to think of a small, centralized debugging/logging facility, don't you think?

[Stapelzeiger]

I would systematically panic("out of memory") if the error can't be handled reasonably. The uC/OS-II has memory pools, but if managed centrally it doesn't prevent race conditions.

[Stapelzeiger]

for the debugging facility I propose to direct stdout to a buffer that can be read after a panic.

[antoinealb]

Closing the issue now that this module doesn't do memory allocation anymore.

[froj]

IMO we should open an issue in the coding style repo.