[avr-gcc-list] Re: Newbie question

[David Brown]

David VanHorn wrote:
>     I'd be surprised if either the original code or your modified code
>     actually works - since TOV1 is 2, the conditional can never be true.
>     The code you *really* want is:
>
>            if (TIFRx & (1 << TOV1)) {
>
>  
> This section I'd not tested yet, and it may be pretty much irrelevant 
> since the pulses are pretty short, and very unlikely to ever roll the 
> counter.
> However, I had intended to trap that condition, so thanks for the catch.
>  

It's up to you to determine the relevance (overflow certainly can 
happen, and it may give you the wrong measurements if it does - whether 
such glitches are an issue or not is dependent on the rest of your program).

>     You also seem to have forgotten to enable optimisation.  There is no
>     point trying to hand-optimise your code if you don't let the
>     compiler do its job properly!  Use "-Os" optimisation unless you
>     have particular reason to do anything else.  
>
>  
> I had -Os selected in the makefile I was using. Which is another reason 
> that I was surprised to see the pushing and popping of unused 
> registers.  It's possible that the optimization was somehow not on, or 
> not working as I noticed that when I commented out major routines in 
> main, so that they never were executed, the compiled code size wasn't 
> changing by more than a few bytes. Apparently it wasn't "seeing" that 
> those blocks were never used.  Another alligator that I was going to 
> look into at some point once I drained the swamp a bit.
>  

It's possible that optimisation was on, but it did not appear that way 
from the generated code.  However, that was out of context - 
occasionally other parts of the code will affect the optimisation of 
conditionals and other constructs.

>  
>
>     Also enable compiler warnings - at a minimum, use these flags:
>
>            -Wall -Wextra -Wunreachable-code
>
>     Then the compiler would have told you of your error here.
>
>  
> I had -Wall on, but not the others.  I'm VERY new to this, and makefiles 
> are yet another thing I have to learn.

I generally have a lot more warnings enabled than just these.  It's not 
quite as good as lint, but gcc's warnings can find a lot of potential 
errors or stylistic mistakes.

-Wall -Wextra -Wbad-function-cast -Wcast-qual -Wcast-align 
-Wpointer-arith -Wmissing-prototypes -Wmissing-declarations 
-Wmissing-noreturn -Wredundant-decls -Wnested-externs -Wno-multichar 
-Winline -Wunreachable-code -Wpadded -Wc++-compat

This covers most of the possible warnings, although there are a few that 
I omit (like -Wshadow, because it's not a mistake to shadow another 
name, and it gives far too many false positives).  Sometimes I have to 
omit others to reduce the number of minor warnings, especially if I'm 
working with code from other sources.

One thing I like about the -Wmissing-prototypes, -Wmissing-declarations 
and -Wredundant-decls is that it enforces strict policies about extern 
and static identifiers - a function or variable is *either* global and 
has an "extern" declaration (in an appropriate header, although the 
compiler can't check that), *or* it is declared "static".  There is no 
middle ground with functions that are uselessly defined globally (i.e., 
no "static") but inaccessible (no "extern") - such global code pollutes 
your global name space and hinders optimisation.



> I also just switched today, to using Studio as the IDE in front of GCC, 
> rather than Programmers Notepad.
> I'll have to figure out how to set that up in Studio.

My preference is to have a decent Makefile, and run make from a command 
box - I'm not much into IDEs (perhaps I'm old fashioned).  Programmer's 
Notepad is a better editor than AVR Studio, so I'd stick to that for 
editing, command-line make for building (and burning to the avr), and 
AVR Studio for debugging if I can't avoid it.

> I do try to create code (asm or C) that produces no warnings. Warnings 
> make me nervous.
> At the moment, I'm trying to figure out how to get rid of the warning 
> generated by my main not having a return type of Int.
> There was a "standalone" flag in my previous makefile that took care of 
> this, but again, I need to learn more about makefiles.
> Cool, I got that figured out.  I wonder why it's not the default, but oh 
> well..
>  
> Even more amusing.. -Wunreachable-code generated 16 warnings, and none 
> in what I wrote.
> A bunch in the LCD library, and one in the delay library.
>  

The disadvantage of setting yourself high standards is that other people 
fail to live up to them...



>  
>
>     Also, when posting to the mailing list, it is much more helpful if
>     you include your actual C code (preferably a minimal compilable
>     snippet) and a note of your compiler version and command-line flags.
>      That will let others test your code and give you more help.  Some
>     people, such as me, *like* the challenge of finding ways to get
>     smaller and faster code for a given function.  But I don't like
>     trying to dig C code out of the comments of an assembling listing.
>
>  
> I try to post as much relevant detail as needed, and no more.
> I can always send the full package to anyone interested.

That's a good aim.

> Given my level of experience in C, I expect that my mistakes will be 
> pretty simple and obvious to a skilled practitioner.  :)
>  

Since it's the C code that's relevant, post the C code.  Don't post a 
complete program - that's more than we want or need.  But the interrupt 
function source code is highly relevant, together with the definitions 
of any global variables used.

>  
>
>     It's also not very polite to say that the compiler "irritates you no
>     end" - Eric and many others have spend a great deal of time and
>     effort writing the compiler and tools so that people like you have
>     high quality development tools that are free (as in "free speech")
>     and zero cost. The developers are always happy to listen to
>     constructive criticism, but your complaints should be well-grounded
>     and politely worded.
>
>  
> It may not be what you want to hear, but it's factual.  I appreciate the 
> work that has been done, but when I see ISRs compile like this, I'm just 
> left speechless..  The whole point of ISRs is to get in, and get out, as 
> fast as possible.  Pushing registers that aren't even used is just 
> pointlessly wasting cycles.  If I wrote ASM like that, I'd get laughed at. 
>  

This may not be what *you* want to hear, but this is factual - C is not 
assembly.  There are things that can be done in assembly that cannot be 
done efficiently in C.  This is partly due to the way the C language 
works, and partly due to the way compilers work.

The C language has no concept of jumping between sections of code as one 
does freely in assembly - it is all within a function.  When generating 
code for a function, the compiler will collect the pre-amble and 
post-amble for all branches of the function, and will attempt to 
optimise the re-use of registers and stack frame space.  Thus if any 
branch of the function requires a register to be preserved, it will be 
stacked at the entry to the function.  It is *conceivable* that a 
compiler could be smarter than this, and only stack such registers 
before they are used.  In practice, I think this would be very hard to 
achieve and have very little usage outside a few situations like this one.

The C language does not have any concept of interrupt functions - this 
is an extension to the language that the compiler supports by using a 
special preamble and postamble.  avr-gcc establishes a bare minimum of 
safety by preserving R0, R1 and the flags, and clearing R1, since many 
of the instruction generation sequences assume that R1 is always 0, and 
that R0 is always available as a scratch register.  Again, it is 
possible to write a compiler that does not have these assumptions - and 
again, the work would be great and the rewards small (it would arguably 
have been better to have different registers - say R2 and R3 - rather 
than R0 and R1, but that's easier to see in hindsight).  The compiler 
does a pretty good job of preserving the minimum of registers - I've 
seen plenty of compilers for various architectures (including some very 
expensive compilers) that preserve all the compiler's "volatile" 
register set on entry to an interrupt, whether they are needed or not.

>  
>
>     As a newbie to C, here's a couple of tips in writing your code - use
>     plenty of spaces *consistently* in your code, it makes it easier to
>     read, and easier to search.  And *always* write your if's as:
>
>            if (xxx) {
>                    foo();
>            }
>
>     or
>            if (xxx) {
>                    foo();
>            } else {
>                    bar();
>            }
>
>  
> Sigh..  This is my second program in C.. On the first one, I got beat up 
> for NOT doing the brackets the way I am now..
> The nice part about standards is that there are so many to choose from.  
> The forms that you're suggesting were how I learned it from K+R, and 
> they use the "expert" form pretty much interchangably with the one above.
>  

The way you space and indent the brackets is up to you, as long as it is 
readable and consistent.  My point is just that you should have the 
brackets - omitting them makes it easy to get mixed up as to what is 
within the conditional's scope, and what is not.

>     Do the same with for and while loops.  Once you are a C expert,
>     you'll occasionally find the short form more appropriate :
>
>            if (xxx) foo();
>
>  
> And I'm using that form where I consider it to be appropriate, when the 
> contitional is relatively short.
>  
>