Timer: Broken timing in CTC mode

[frej]

The current timer implementation fails to correctly handle code which sets the OCRxA register in (at least) CTC mode.

The following program, when compiled for an atmega328 and run on real hardware, will produce a square wave at ~300Hz with a duty on cycle of 50% on PB3. Running it on simavr gives ~234Hz and a duty cycle of 40%.

/* Fuses: E:07, H:D9, L:D7 */
#define F_CPU 16000000UL
#include <avr/io.h>
#include <avr/interrupt.h>
#include <util/delay.h>
ISR(TIMER2_COMPA_vect)
{
    static int phase = 0;
    PORTB ^= _BV(3);
    if (phase)
        _delay_ms(1.0);
    phase = !phase;
    OCR2A = 25;
}
int main (void)
{
    sei(); /* Enable interrupts */
    DDRB |= _BV(3);
    /* Timer in CTC mode, interrupt when compare match */
    OCR2A = 25;
    TIMSK2 = _BV(OCIE2A);
    TCCR2A = _BV(WGM21);
    TCCR2B = _BV(CS22) | _BV(CS21) | _BV(CS20);

    while (1)
        ;
    return 0;
}

Looking at the implementation, it appears that there is a bug in how the cycle timer delay is calculated. When the delay to the next compare match is calculated the current TCNTx is not considered. Unfortunately it is not clear to me how this can be fixed without subtly breaking the other timer modes.

[bsekisser]

As a thought... have you tried this on older versions of simavr to see if this is pre-existing condition or recently introduced? I have been looking at the timer code for quite some time and may myself have been seeing evidence of incorrect behavior... have considered putting together some test code to further understand the what is going on.

On Sun, 2015-07-26 at 10:07 -0700, frej wrote:

       From: 
         frej

[email protected] Reply-to: buserror/simavr <reply +0044e4b10d0c90b2f749097dd439dc400099bb8bc67d85de92cf0000000111ccd5d592a169ce05cd34a5@reply.github.com> To: buserror/simavr [email protected] Subject: [simavr] Timer: Broken timing in CTC mode (#125) Date: Sun, 26 Jul 2015 10:07:33 -0700 (07/26/2015 01:07:33 PM)

The current timer implementation fails to correctly handle code which sets the OCRxA register in (at least) CTC mode.

The following program, when compiled for an atmega328 and run on real hardware, will produce a square wave at ~300Hz with a duty on cycle of 50% on PB3. Running it on simavr gives ~234Hz and a duty cycle of 40%.

/* Fuses: E:07, H:D9, L:D7 _/ #define F_CPU 16000000UL #include <avr/io.h> #include <avr/interrupt.h> #include <util/delay.h> ISR(TIMER2_COMPA_vect) { static int phase = 0; PORTB ^= _BV(3); if (phase) delay_ms(1.0); phase = !phase; OCR2A = 25; } int main (void) { sei(); / Enable interrupts _/ DDRB |= BV(3); / Timer in CTC mode, interrupt when compare match */ OCR2A = 25; TIMSK2 = _BV(OCIE2A); TCCR2A = _BV(WGM21); TCCR2B = _BV(CS22) | _BV(CS21) | _BV(CS20);

while (1)
    ;
return 0;

} Looking at the implementation, it appears that there is a bug in how the cycle timer delay is calculated. When the delay to the next compare match is calculated the current TCNTx is not considered. Unfortunately it is not clear to me how this can be fixed without subtly breaking the other timer modes.

— Reply to this email directly or view it on GitHub.

[frej]

As a thought... have you tried this on older versions of simavr to see if this is pre-existing condition or recently introduced? I have been looking at the timer code for quite some time and may myself have been seeing evidence of incorrect behavior...

I did now, and it appears that you are in the clear :) cc54eda5e96867b724ffb9a6e3610fce557faad5 before your changes to avr_timer.c has the same problem, so it's not something recent.

have considered putting together some test code to further understand the what is going on.

As far as I can tell, a write to OCRx[AB] also triggers a reset of TCNT or at least that is the effect. The current implemention which does all the timing calculations based on frequency and some magic numbers stored in the timer structure makes my brain hurt.

I'm currently working on a refactoring of the timer code, the new design is based on cycles which makes it much easier to make it behave correctly and also simulate the interaction with the prescaler (resetting it and synchronizing it to other events).

[frej]

Thanks @bsekisser but I don't think that patch will help my problem. My problem is not the precision, it is that changing the divisor clears the timer count (as described above)...

[bsekisser]

Check out issue #169... similar if not the same here... @messani looks to be on to something.

[frej]

Yes I saw that, It appears that @messani has also concluded that the reconfiguration logic clears the timer count when the hardware does not. Hopefully he/she will fix it.

[djfd]

May I ask you guys,

I noticed that _BV macro is mostly used in FUSES definitions (and some other minor use cases), and only for a cores definitions. Quick grepping gives

$ grep -rn _BV | egrep -v FUSE
cores/sim_core_declare.h:35:#define _BV(v) (v)
cores/avr/iotn2313.h:79: *  Example: PORTB |= _BV(PORTB7); Set MSB of PORTB.
cores/avr/iom163.h:686:#define SLEEP_MODE_ADC          _BV(SM0)
cores/avr/iom163.h:687:#define SLEEP_MODE_PWR_DOWN     _BV(SM1)
...

So, being impossible to find different definition of _BV macro, I am concluding that it is simply identity macro. But its usage is misleading then. It seems that expected outcome of the macro would be something like to (1<<v)

Could anybody shed a few light?

Thanks