Excessive use of error code returns

[niansa]

Hi!

I've noticed error code returns are used quite excessively. uart_init returns an error code even though that error code can only be UART_OK and most functions check their arguments.

Since the ch32v003 is quite a weak platform I feel like at least argument checking should be optional and a lot of functions could simply return void instead, saving cycles on checking and propagating return values.

I'd do this by defining a type that is conditionally either uart_err_t or void depending on if argument checking is enabled and return that type from all functions where the only errors that could occur come from argument checking.

Additionally, you should add the __attribute__((__nonnull__)) attribute to all pointer arguments that can't be null if the compiler is GCC or Clang to allow compile time errors and better static analysis:

The nonnull attribute specifies that some function parameters should be non-null pointers. For instance, the declaration:

extern void *my_memcpy (void *dest, const void *src, size_t len) __attribute__((nonnull (1, 2)));

causes the compiler to check that, in calls to my_memcpy, arguments dest and src are non-null. If the compiler determines that a null pointer is passed in an argument slot marked as non-null, and the -Wnonnull option is enabled, a warning is issued. The compiler may also choose to make optimizations based on the knowledge that certain function arguments will not be null.

If no argument index list is given to the nonnull attribute, all pointer arguments are marked as non-null. To illustrate, the following declaration is equivalent to the previous example:

extern void *my_memcpy (void *dest, const void *src, size_t len) __attribute__((nonnull));

https://gcc.gnu.org/onlinedocs/gcc-4.7.2/gcc/Function-Attributes.html

[ADBeta]

Fixed init() I want to check some internal registers later to have actual valid return values, other than just INVALID_ARGS. Didn't have time to dig into figuring out a state determination with the registers, but will add it one day. For now, init() is a void but will change this later also eventually.

As for the attributes, at runtime pointer variables may be passed as NULL, which is when bugs actually happen. I don't think the compiler attribute will help this, and it's good enough to return the state

(Keep in mind, even at the highest speed, the UART is very slow compared to the HB Clock, so speed isn't something that needs excessive focus for the time being)

[niansa]

(Keep in mind, even at the highest speed, the UART is very slow compared to the HB Clock, so speed isn't something that needs excessive focus for the time being)

But also keep in mind extra code consumes extra space both flash and stack wise :D

[niansa]

I don't think the compiler attribute will help this

They may not normally detect this, true. But static analyzers will oftentimes be able to find code paths where null pointers may be passed to nonnull arguments in some way. Check out clangs static analysis tools, they actually go though the code taking different branches to see if something like that may happen. It helped me fix a lot of bugs in the past.

Without the nonnull attribute it will not warn about this because it expects that passing null pointers to the function is normal behavior.