[En-Nut-Discussion] Bugs in Uart.c

[Nathan Moore]

>
> >
> >> 2: Still UsartRead:
> >> rc should be unsiged int. The is no reason to make it size_t as it will
> >> never become bigger then size. The disadvantage is, that for the avr
> >> size_t is an unsigned long, which costs performance.
> >>
> >
> > size_t is defined to an archetectually dependant type to specify which
> can
> > describe the size of any chunk of data.  It's probably already
> equavelent to
> > an unsigned int on your architecture.
> >
>
> Unfortunately it's unsigned long int. I googled for size_t and basically
> it seems to be "some kind" of unsigned integer. This is fine if you do
> have performance, but if every tick count's and registers are quite
> valuable IMHO one should know how a variable looks like. (I hate
> decisions which depend on the mood of your compiler. I therefore
> generally prefer uint8_t, etc. Even char is defined mostly signed.)


It's "some kind of unsigned integer" specifically so that it can be defined
to the right size of integer for the target processor by the
compiler/library people.  Using an 8 bit value would only work for a
processor that could only have 256 bytes of contiguous memory.
Normally it size_t would be an unsigned integer such that (sizeof(size_t) ==
sizeof(void *)).
The buffers for uarts can be larger than 256 bytes, so must user a bigger
size type to talk about their size.


>
>
> >
> >
> >> 4: Once again the return types are often much bigger then needed. This
> >> costs registers and performance.
> >>
> >
> > I agree that the types are often bigger than needed.  NutEventWait uses
> an
> > extra byte; the biggest impact of this is that it would cause every
> caller
> > who checks the result to have an extra instruction to check each byte on
> an
> > AVR.  It should return some sort of archetectually specific boolean
> type,
> > which would be a char (or unsigned char) for AVR or an int for other
> > architectures.
> >
> >
>
> The Impact is bigger. Depending on your function it's also one more byte
> that has to be pushed around and one more register which has to be
> saved. Are there any disadvantages on other architectures using one byte
> instead of two? Maybe use something like:
>
> enum boolean __attribute__  ((packed)) { //
> http://www.delorie.com/gnu/docs/gcc/gcc_63.html
>    true = 1,
>    false = 0
> } ;

The impact of using 2 bytes for this on AVR isn't that big within the
NutEventWait function itself, since this function's code only exists in one
place within the executable image so it's literally one extra instruction.
The additional code of the callers dealing with this has a bigger impace
since there are many.
As far as taking up registers it's not a big deal since those registers are
already clobbered by the arguments to NutEventWait and the ABI probably
specifies that they should not be assumed to be preserved across a call
anyway.  With some global optimization of the entire project (NutOs and your
code) a compiler could forget the ABI to some degree and do this, but that
gets complicated and the compiler would have already figured out that you
didn't even need that one byte but only one of the status flags to see the
result (if you look at the end of NutEventWait you'll see what I mean).
On architectures with only 16 bit registers you'd want to use a 16 bit value
for boolean values for general computation since 1) that's what's gonna be
used anyway and 2) the compiler won't think it's got to clear the top 8 bits
all the time.

I believe that c99 already defines a <bool.h> that has similar features to
the c++ bool type for this reason.

int_fast8_t would work.  It is defined to be big enough to hold an 8 bit
integer, but it can be any size >= 8 bits.
This works good for computations, but if you are storing it to a file or
transmitting it over a network you would probably want to do your
computations on int_fast8_t variables, then assign to a type that is only 8
bits before sending or storing the value.

Nathan