• Locked Locked
  • Replies 12 replies
  • Subscribers 80 subscribers
  • Views 2665 views
  • Users 0 members are here
Support feedback
Options
Options
Related

This thread has been locked.

If you have a related question, please click the "Ask a related question" button in the top right corner. The newly created question will be automatically linked to this question.

MSP430 LaunchPad demo code does not work with all optimisation settings of IAR compiler

Jörg Becker
Prodigy 130 points

Seems as if the code does only run by chance and not by design.

I compiled first with optimisation 'none' and the code gets stuck in the first while loop in Transmit():

while (CCR0 != TAR)

   CCR0 = TAR;

Reason seems to be that in optimisation `none', there is a jump inserted between setting CCR0 and comparing with TAR and so the comparison is always true (?).

Transmit:
  while (CCR0 != TAR)                       // Prevent async capture
 00FB90    9292 0170 0172     cmp.w   &TAR,&TACCR0
 00FB96    2404               jeq     0xFBA0
    CCR0 = TAR;                             // Current state of TA counter
 00FB98    4292 0170 0172     mov.w   &TAR,&TACCR0
 00FB9E    3FF8               jmp     0xFB90
  CCR0 += Bitime;                     // Some time till first bit
 00FBA0    50B2 0034 0172     add.w   #0x34,&TACCR0

In higher optimisation settings the code is:

Transmit:
 00FB8E    3C03               jmp     0xFB96
    CCR0 = TAR;                             // Current state of TA counter
 00FB90    4292 0170 0172     mov.w   &TAR,&TACCR0
  while (CCR0 != TAR)                       // Prevent async capture
 00FB96    9292 0170 0172     cmp.w   &TAR,&TACCR0
 00FB9C    23F9               jne     0xFB90
  CCR0 += Bitime;                     // Some time till first bit
 00FB9E    50B2 0034 0172     add.w   #0x34,&TACCR0

which works.

But, in higher optimisation (at least in 'high') the code fails again, this time due to lacking volatile declaration of some variables (I did not try to find out which, I just declared mostly all of them volatile, which is always a good start  :-)).

All in all, I think that the code in its present state is not very well suited for beginners.

BR,

Jörg.

 Edit: variable 'timermode' is only set but not used in main loop. Has to be declared volatile, because it is used in interrupt.

 

 

 

 

  

  • Guru 227245 points

    This code makes me shudder.

    If TAR is clocked faster than with MCLK/7,  this while loop will loop eternally.
    Why? Because when I set CCR0=TAR, the next clock tick will increase TAR again.
    And between the setting and the next check inthe unoptimized version are 7 MCLK cycles (maybe 5, depending of when TAR is read during the CMP instruction).
    In the higher optimized version, there's only 5 MCLK cycles between write and compare (maybe 3).

    This kind of loops belongs to the 666 section. It is unsafe and highly depends on the compiler and its settings.

    If something like that is really necessary for some hardware reason, then it should be written in (inline) assembly, so the outcome is deterministic.
    Unfortunately, the syntax for inline assembly also depends on the compiler (yet not the optimisation level or other compiler options).

    Jörg Becker said:
    variable 'timermode' is only set but not used in main loop. Has to be declared volatile, because it is used in interrupt.

    No. 'volatile' does not mean 'used in interrupt'. It means 'may be changed outside the current program flow'.
    And since it is only set but never read in main, it doesn't matter whether it is changed inside an ISR. Also, since each ISR starts without any assumptions about variable contents, it doesn't matter too.
    A volatile declaration would only be necessary if nested interrupts are allowed and one ISR would alter it in the middle of the execution of another ISR that reads it more than once.

  • in reply to Jens-Michael Gross
    Prodigy 130 points

    Sorry Jens, but what you said is wrong:

    it is necessary to declare 'timermode' volatile for the code to work under all circumstances (especially high compiler optimizations). I did not say that 'it has to be volatile because it is used in interrupt' but that 'it has to be volatile..' and 'is set and not used' in main. This combination is the reason why it has to be declared volatile.

    Just try to compile with optimisation high and you will see that the compiler does not generate code for all occurences of 'timermode = something' in main. And this is ok, because the variable is never used in main. So, it is not a compiler bug.

    BR,

    Jörg.

     

     

     

  • in reply to Jörg Becker
    Guru 227245 points

    If it is set several times to different values (or even the same) and this has a meaning (e.g. triggering ot switching ISR functionality), then you're right, then the variable needs to be declared volatile.

    If there is a funciton call (even an unrelated one) after setting a new value, volatile is not necessary, as the compiler won't optimize it away as it cannot know whether this global variable is used in this function.
    But if no other functions are called between two settings of this variable, then of course the compiler might optimize it and omit the first set operation.

    I (mis-)interpreted your words in that the variable is only set once in main (initialized).

  • in reply to Jens-Michael Gross
    Prodigy 130 points

    Hi Jens,

    I am not a compiler or C guru, so I am not 100% sure about how the compiler has to work. So I think what you wrote is ok.

    Still, to be done and/or remembered:

    - 'timermode' has to be declared volatile for the program to work correctly.

    - volatile has to be used on all variables where the compiler shall not do any optimization (this is at least the official reading in the compiler manuals)

    - if you are unsure whether optimization on a variable is allowed, just use volatile as default. You can think about optimizing the code afterwards.

    - always be careful also with volatile variables, their value can change between two accesses:

    volatile uint16_t test:

    if( (test>10) && (test<20) )    // value of test can change between these two accesses

    {

        test can be smaller than 10 here !!!!

    }.........

     

    BR,

    Jörg.

     

     

  • in reply to Jörg Becker
    Guru 227245 points

    Jörg Becker said:
    I am not a compiler or C guru, so I am not 100% sure about how the compiler has to work


    Actually, the outcome of 'volatile' is not defined by the C standard and it is compiler implementation-dependent.
    e.g. if the compiler detects that there no reference exists to a volatile variable outside the current function, it may declare the volatile identifier meaningless. (this is why empty loops with a local variable might be eliminated even if the variable has been declared volatile)
    There is no 'compiler has to work' in case of volatile. That's unfortunate but that's how the standard defines it.

    I'm not a copiler guru myself (yet I have C(++) experience almost as long as this language exists - I even wrote some programs wiht a C compiler for the C64 computer).
    But we had an exhaustive discussion about this topic in the mspgcc mailing list recently.

    Jörg Becker said:
    'timermode' has to be declared volatile for the program to work correctly.

    There we agree, dependent on your description of the code. (I still didn't look into it :) )

    Jörg Becker said:
    - volatile has to be used on all variables where the compiler shall not do any optimization (this is at least the official reading in the compiler manuals)

    That's the compilers interpretation of this atribute. But not the original meaning. Not doing optimizations is, however, an appropriate handling.
    It would be more exact to say 'the compiler will not do any optimisations on variables marked volatile'. Else it would rather be a #pragma. :)

    Jörg Becker said:
    - if you are unsure whether optimization on a variable is allowed, just use volatile as default. You can think about optimizing the code afterwards.

    I cannot recommend this. The usage of the volatile keyword does not only inhibit optimization. It also limits the compiler in many ways. It may not implement it as a (temporary) register variable (which is not an optimization), requires memory-access operations where a register could be used, forces the compiler to evaluate the complete expression even if not necessary and much more. It has great impact on code size and speed, even with optimisations off. (see below)

    Jörg Becker said:
    - always be careful also with volatile variables, their value can change between two accesses:

    Yes, that's the exact meaning of volatile. The value may change between two accesses and the access itself (reading or writing) may have side-effects even if the same value is written. This is why the MSP hardware registers are usually defined as volatile variables on a fixed address.

    Jörg Becker said:
    if( (test>10) && (test<20) )    // value of test can change between these two accesses


    Not only that, the compiler needs to generate code that will do the 'test<20' check even if 'test>10' has already failed. And it has to do a memory access to test twice instead of reading it just once into a register and work with it.

    Jörg Becker said:
        test can be smaller than 10 here !!!!

    or >=20. In fact it can have any value. And still changing values if used more than once in this code block. This is why some code constructs working with fast-changing hardware registers such as TAR may or may not work, depending on optimisation level and current clock module settings. Not to mention that MUCH time can pass if an IRQ happens in between.

  • in reply to Jens-Michael Gross
    Prodigy 130 points

    Jens,

    I agree nearly 100%. I would still rather use one volatile declaration too much than one too little. If the code is fast and small enough, leave it as its is, otherwise use optimizations (of the compiler and of your code, like omitting volatile on carefully selected variables). I have unfortunately seen too many programs of too many ('experienced') software developers who do not know 'volatile' at all and which work just by coincidence. (Then these people tell you that they never use compiler optimization because the compiler then often does not work correctly :-) )

    BR,

    Jörg. 

    PS: before this becomes off-topic: I still hope that the demo code for the Launchpad will be modified (or better: corrected). In this context, could someone explain to me, why they choose this construction:

      while (CCR0 != TAR)                       // Prevent async capture
        CCR0 = TAR;                             // Current state of TA counter
      CCR0 += Bitime;                     // Some time till first bit

    What exactly could happen if I just do:

      CCR0 = TAR+Bitime;                  // Some time till first bit

     

  • in reply to Jörg Becker
    Guru 227245 points

    Jörg Becker said:
    I would still rather use one volatile declaration too much than one too little.

    Better safe than sorry. Agreed. Yet it doesn't help you when you use empty for-loops in your code to make a delay.

    Jörg Becker said:
    I have unfortunately seen too many programs of too many ('experienced') software developers who do not know 'volatile' at all and which work just by coincidence.

    me too :) That's why I call myself Software Engineer and not developer or (even worse) software designer.
    When studying software engineering (which is an engineer study primarily focused on IT rather than a plain IT study) I encountered many academic people (including some of the professors) who lost contact to the physical world (and even the physical laws) when drifting in higher spheres of IT theories. Resulting in nice and theoretically perfect code that just didn't work in real life. (e.g. this super-duper 5-axis robot which could translate any given location into the mathematically correct position of all of his axis - yet ripping his power-cable from the wall on every second calibration process)

    Jörg Becker said:
    Then these people tell you that they never use compiler optimization because the compiler then often does not work correctly :-)

    and ask for a faster MSP because the fastest the could get was unable to do the double-precision division of an integer value by an integer into an integer no fast enough. (I guess this guy never heard of the 'int' type at all)
    Yes, sometimes ony can only shake the head in astonishment.
    As Albert Einstein said: there are only two thing that are limitless: the universe and human silliness. And he was not sure about the universe.

    Jörg Becker said:
    explain to me, why they choose this construction:

    Reading TAR until it is equal to the just read value ensures that you don't read a fractional/distorted value when TAR in incremented just during the read. This might happen when TA is clocked by an external clock or an internal one that is not synchronized with MCLK. That's the only explanation that makes some sense and only if TACLK is much (>10) slower than MCLK. Which isn't in this case and causes the whole construct to fail with optimisation on.

  • in reply to Jens-Michael Gross
    Prodigy 130 points

    Jens-Michael Gross said:
    Yet it doesn't help you when you use empty for-loops in your code to make a delay.

    Is that really true? I have never seen any compiler remove an empty for or while loop with a volatile counter variable (IAR does not do it). I do not think that this would be correct, as the volatile variable could also be a peripheral register where writing changing values to it could fulfill a purpose.

    Jens-Michael Gross said:
    Reading TAR until it is equal to the just read value ensures that you don't read a fractional/distorted value when TAR in incremented just during the read. This might happen when TA is clocked by an external clock or an internal one that is not synchronized with MCLK. That's the only explanation that makes some sense and only if TACLK is much (>10) slower than MCLK. Which isn't in this case and causes the whole construct to fail with optimisation on.

    Ok, thanks for this explanation. Makes sense (or not, as you said :-) )

     

     

  • in reply to Jörg Becker
    Guru 227245 points

    Jörg Becker said:
    Yet it doesn't help you when you use empty for-loops in your code to make a delay.

    Is that really true? I have never seen any compiler remove an empty for or while loop with a volatile counter variable (IAR does not do it). I do not think that this would be correct, as the volatile variable could also be a peripheral register where writing changing values to it could fulfill a purpose.[/quote]
    You're right for global variables (and that's what the hardware registers are to the compiler: global volatile variables).
    I was talking about constructs like "for(volatile int i = 0; i<100;i++);". Or where the loop variable is a function-local variable that isn't used for anything else. The compiler will detect that 1) it is generated on the stack or in a CPU register and 2) nobody else can possibly know the reference to it and the volatile qualifier is therefore void.

  • in reply to Jens-Michael Gross
    Prodigy 130 points

    Understood, thanks for the explanation, Jens.

    I always assumed, that it would only be reasonable to declare global variables as volatile. But as you said, it would also be possible to declare local variables as volatile. I just never do that and so the consequences were not clear to me. 

    You always learn something new during these discussions!!!

     

  • in reply to Jörg Becker
    Guru 227245 points

    For a for-loop, one usually uses a local variable. Then some people encountered that their 'smart' delay wasn't delaying properly. Then they learned about compiler optimization and the volatile qualifyer. And it still didn't work :)

    Jörg Becker said:
    You always learn something new during these discussions!!!

    That's why I'm here: to learn from the problems others have before I have them myself. :)

  • in reply to Jens-Michael Gross
    TI__Guru 69642 points

    Regarding the piece of code in question...

    while (CCR0 != TAR)
       CCR0 = TAR;

    Yes this is to ensure a correct read of TAR despite of the asynchronous nature (in respect to MCLK) that the timer clock can have. Also agreed that this isn't the best way of doing this due to the discussed limitations especially in regards to C-compiler dependency.

    The recommended way to capture an asynchronous timer counter register value (TAR, TBR, ...) is actually to use one of the capture/compare blocks in capture mode to for example capture the ‘rising edge’ (CM_1), and then switch the capture compare input select bits from 'GND' to 'Vcc' (CCIS_2 --> CCIS_3). This way the timer capture logic will be used to capture the timer counter register in a bullet-proof way not affected by any potential race conditions caused by asynchronous clocks.

    We'll make these changes in the next SW revision of the LaunchPad code and we'll also add a GCC version of the code to our official archive. Moving forward the plan is to release more of our software in a GCC compatible version in addition to the CCS / IAR versions that we have done traditionally.

    Thanks and Regards,
    Andreas

**Attention** This is a public forum