Malloc wrapper + Panic function

[antoinealb]

I wrote a wrapper for malloc to do error handling, as described in issue #18. There is also an implementation of a function to report critical failures. panic is defined as a pointer to be able to replace it with a test double when we need it.

Those wrappers are not feature-complete yet but they are already usable and I will add features in other PR. Features I would like to see :

• Find a way to make malloc fail to test if the code properly recovers.
• Add a location (file + line) information to panic message.

[pierluca]

LGTM, more or less. Modifications as described would be welcome. Stylistically, I would prefer void* function() than void *function(), but that's minor. I assume there's going to be an alternative to "printf" for the ARM uCs though, right?

If you need inspiration, either with testing for fails or file+line information, check the fluid studios memory manager, you might have something there.

[antoinealb]

Modifications as described would be welcome.

For the two modifications, I will put them up in another pull request once this one gets merged. Smaller pull requests make the code review process easier IMHO.

Stylistically, I would prefer void* function() than void *function(), but that's minor.

Ok, will change.

I assume there's going to be an alternative to "printf" for the ARM uCs though, right?

On ARM we will probably either :

1. For debug : do a while (1) {} loop after shutdowning the motor output and disabling interrupts, to allow to attach a debugger
2. For production (aka once the cards are wired in the robot) : Reboot the card.

[pierluca]

1. For production (aka once the cards are wired in the robot) : Reboot the card.

... wondering ...

In this case, but also in the other, is it worth trying to set a global "is_panicing" state and send a message along the lines of "I crashed" on the CAN bus (knowing that it can fail) ?

This would enable the master to know immediately that a card crashed and to gracefully handle the reinitialization. Maybe though it's easier to avoid this by using a message like "I just booted" or a mere heartbeat.

[antoinealb]

I would prefer a "I just booted" message, or eventually : "I just did a hot reboot". Because by definition, when you crashed, probably nothing works anymore and you cannot expect your CAN stack to be operational.

[Stapelzeiger]

We should also add xrealloc(). For the void *function() vs void* function() I like the first better. I think it's more common. (It's also Linux kernel coding style) The rest looks good.

[pierluca]

Won't start a coding style crusade here. The document by Linus, "Linux Kernel Coding Style" doesn't specify that for functions.

Rationale:

int *d, c; // d is pointer to integer, c is integer
int* d, c; // would lead you to believe that both are pointers

That's why I was suggesting.

int* function(); // return type is pointer to integer,
// * does not modify function and it has no relation to its name

Ultimately, whatever you prefer. I just find the void* cleaner for this reason;

[Stapelzeiger]

When declaring pointer data or a function that returns a pointer type, the preferred use of '*' is adjacent to the data name or function name and not adjacent to the type name.

https://www.kernel.org/doc/Documentation/CodingStyle

I agree, putting the '*' adjacent to the type would be more logical for return types, but it doesn't seem to be very common.

[pierluca]

Sorry, you're right.. and I can't say I'm sleep deprived today :-D I had skimmed through it, found confirmation for data and completely missed the second part of the sentences. -.-"

[antoinealb]

This * discussion is pretty useless IMHO. If you feel that it is really important, please open an issue or a PR in coding style to discuss it.

For xrealloc, should it panic on zero size ? I think not. Also, should xmalloc panic in case of zero size ? I would say no too, to stay consistent with xrealloc.

[Stapelzeiger]

I agree, if the size is zero, it shouldn't panic.

[pierluca]

agreed.

[antoinealb]

I added a wrapper for realloc. I think this can be merged and a separate issue opened for the "malloc fail" features if we really want it.