Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

timer1: mode 9 is not working anymore #111

Open
fjangfaragesh opened this issue Oct 22, 2021 · 13 comments
Open

timer1: mode 9 is not working anymore #111

fjangfaragesh opened this issue Oct 22, 2021 · 13 comments
Assignees
Labels
bug Something isn't working

Comments

@fjangfaragesh
Copy link

In real (tested with Arduino Uno), the LED on pin 10 is blinking fast. In simulation nothing happens.

#ifndef F_CPU
#define F_CPU 16000000UL // 16 MHz clock speed
#endif

int main(void){
  DDRB |=  (1 << PORTB2); //Define OCR1B as Output (pin number arduino: 10)
  TCCR1A |= (1 << COM1A0) | (1 << COM1B1) | (1 << WGM10); // PB2: output, PWM mode 9
  TCCR1B |= (1 << CS10) | (1 << CS11) | (1 << WGM13);// prescaler=64, mode 9
  OCR1A = 4000;
  OCR1B = 1000;
  while(1) {
  }
}
@urish urish added the bug Something isn't working label Oct 22, 2021
@urish urish self-assigned this Oct 22, 2021
@urish
Copy link
Contributor

urish commented Oct 22, 2021

Thanks for reporting! Do you have any idea if it worked in previous version of the simulator?

@urish
Copy link
Contributor

urish commented Oct 22, 2021

Initial analysis: it seems like the issue happens if OCR1A is still 0 when you configure the timer. A workaround would be to first set OCR1A / OCR1B, and then enable the timer:

int main(void){
  DDRB |=  (1 << PORTB2); //Define OCR1B as Output (pin number arduino: 10)
  OCR1A = 4000;
  OCR1B = 1000;
  TCCR1A |= (1 << COM1A0) | (1 << COM1B1) | (1 << WGM10); // PB2: output, PWM mode 9
  TCCR1B |= (1 << CS10) | (1 << CS11) | (1 << WGM13);// prescaler=64, mode 9
  while(1) {
  }
}

Here's a complete functional example: https://wokwi.com/arduino/projects/313183501205635648

To fix it, we need to figure out what to do if the timer starts and TOP (OCR1A) == 0. Should it generate compare match on every single timer cycle?

@fjangfaragesh
Copy link
Author

fjangfaragesh commented Oct 23, 2021

Thanks for reporting! Do you have any idea if it worked in previous version of the simulator?

It worked in version 0.11.4.

The change of the OCR1A register is apparently only adopted after the change of direction at the old top:
(i think this behavior is wrong)
grafik
(blue: ORC1A, red:TCNT1, yellow:ORC1B, at tick 26,000,000 and 32,500,000 OCR1A is changed and TCNT1 is set to 0)

@urish
Copy link
Contributor

urish commented Oct 23, 2021

Thanks!

Is the graph based on version 0.11.4 or on an actual chip?

@fjangfaragesh
Copy link
Author

version 0.18.2

@urish
Copy link
Contributor

urish commented Oct 23, 2021

The change of the OCR1A register is apparently only adopted after the change of direction at the old top:

As far as I can tell from the datasheet, this is the correct behavior, isn't it?

image

@fjangfaragesh
Copy link
Author

fjangfaragesh commented Oct 23, 2021

I've tested it. That doesn't seem like the right behavior.
I have written a program that reads TCNT1 and changes OCR1A. At the end, the results are transmitted via the serial interface.

int results[500];
int main(void){
  Serial.begin(9600);
  DDRB |=  (1 << PORTB2); //Define OCR1B as Output (pin number arduino: 10)
  OCR1A = 2000;
  OCR1B = 500;
  TCCR1A |= (1 << COM1A0) | (1 << COM1B1) | (1 << WGM10); // PB2: output, PWM mode 9
  TCCR1B |= (1 << CS10) | (1 << CS11) | (1 << WGM13);// prescaler=64, mode 9
  for (int i = 0; i < 250; i++) {
    results[i] = TCNT1;
    _delay_us(100);
  }
  OCR1A = 1000;
  OCR1B = 100;
  TCNT1 = 0;
  for (int i = 0; i < 250; i++) {
    results[i+250] = TCNT1;
    _delay_us(100);
  }
  for (int i = 0; i < 500; i++) Serial.println(results[i]);
  Serial.flush();
  while(1) {
  }
}

Results: (wimulation compared with real Arduino Uno)
grafik

@urish
Copy link
Contributor

urish commented Oct 23, 2021

Thank you for doing this comparison, it's really useful!

I need to search the data sheet to see if I can find where the exact behavior is defined, and adjust the simulation accordingly

@urish
Copy link
Contributor

urish commented Oct 29, 2021

Ok, we have a fix. I verified it it the following test program:

int main(void) {
  uint8_t v0, v1, v2, v3;
  Serial.begin(9600);
  asm(R"(
    CLR r1          ; r1 is our zero register
    LDI r16, 0x0    ; OCR1AH = 0x0;
    STS 0x89, r1    
    LDI r16, 0x8    ; OCR1AL = 0x8;
    STS 0x88, r16  
    ; Set waveform generation mode (WGM) to PWM Phase/Frequency Correct mode (9)
    LDI r16, 0x01   ; TCCR1A = (1 << WGM10);
    STS 0x80, r16  
    LDI r16, 0x11   ; TCCR1B = (1 << WGM13) | (1 << CS00);
    STS 0x81, r16  
    STS 0x85, r1    ; TCNT1H = 0x0;
    STS 0x84, r1    ; TCNT1L = 0x0;

    LDI r16, 0x5   ; OCR1AL = 0x5; // TCNT1 should read 0x0
    STS 0x88, r16  ; // TCNT1 should read 0x2 (going up)
    STS 0x84, r1   ; TCNT1L = 0x0;
    LDS %0, 0x84  ; // TCNT1 should read 0x1 (going up)
    LDS %1, 0x84  ; // TCNT1 should read 0x3 (going up)
    LDS %2, 0x84  ; // TCNT1 should read 0x5 (going down)
    LDS %3, 0x84  ; // TCNT1 should read 0x3 (going down)
  )" : "=r"(v0) , "=r"(v1), "=r"(v2), "=r"(v3) );
  Serial.println(v0);
  Serial.println(v1);
  Serial.println(v2);
  Serial.println(v3);
  Serial.flush();
  for (;;);
}

When running on a physical board, I get the following output:

1
3
5
3

I added a test case that validates this behavior (it fails with the previous version, passes with the fix).

Would love to get your feedback on the new version with the fix (0.18.5)

@fjangfaragesh
Copy link
Author

The behavior of TCNT1 now seems to be correct.

@urish
Copy link
Contributor

urish commented Nov 1, 2021

Lovely! Thanks again for reporting it and all your help in tracing the issue!

@urish urish closed this as completed Nov 1, 2021
@fjangfaragesh
Copy link
Author

But the timer output does not seem to be correct at the beginning.
The first pulse is missing.

#define DLY 135
#define N 128

int results[N*2];
int resultsPINB[N*2];
int main(void){
  Serial.begin(9600);
  DDRB |=  (1 << PORTB2); //Define OCR1B as Output (pin number arduino: 10)
  OCR1A = 2000;
  OCR1B = 1500;
  TCCR1A |= (1 << COM1A0) | (1 << COM1B1) | (1 << WGM10); // PB2: output, PWM mode 9
  TCCR1B |= (1 << CS10) | (1 << CS11) | (1 << WGM13);// prescaler=64, mode 9
  TCNT1 = 0;
  for (int i = 0; i < N; i++) {
    results[i] = TCNT1;
    resultsPINB[i] = PINB;
    _delay_us(DLY);
  }
  OCR1B = 100;
  for (int i = 0; i < N; i++) {
    results[i+N] = TCNT1;
    resultsPINB[i+N] = PINB;
    _delay_us(DLY);
  }
  // send results
  for (int i = 0; i < N*2; i++) {
    Serial.print(results[i]);
    Serial.print("\t");
    Serial.println(((resultsPINB[i] >> 2) & 1)*1000);
  }
  Serial.flush();
  while(1) {
  }
}

There is a difference in the behavior of the physical and the simulated:
image

@urish urish reopened this Nov 1, 2021
@urish
Copy link
Contributor

urish commented Nov 14, 2021

Thanks for providing detailed reproduction with graphs! I haven't had the time to look into it yet, but it's on my list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants