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

[Moritz Struebe]

I'll answer to this mail as it covers all topics.

Nathan Moore schrieb:
>> 1: UsartRead
>> if (NutEventWait(&rbf->rbf_que, dcb->dcb_rtimeout)) {
>> should be
>> if (NutEventWait(&rbf->rbf_que, dcb->dcb_rtimeout) == -1) {
>>     
>
> The first one is correct since it is the same as
>   NutEventWait(&rbt->rbf_que, dcb->dcb_rtimeout) != 0
> If the return value is 0, the first conditional will not be executed, but if
> the return value is -1 it will be with that comparison.  It will also be
> executed if the return value is some other nonzero value, but that never
> happens.
> The return value is used as a boolean, so the first comparison usually
> produces better code because just about all processors have better detection
> of == 0 (and therefore != 0) than other comparisons which often involve
> inlining or loading the compare to value in order to do the compare, which
> is bigger code.
>   

Ah ok. Still learning. :-)

>   
>> 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.)

>
>   
>> 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
} ;

With the packed attribute for 8bit uC?

No comments on 3?

Cheers
Morty