Implement 16-bit timers

[urish]

See #29 for details

[urish]

@gfeun can you please check you code with this new branch and report? All the WGM modes should now be defined correctly both for 8 and 16 bit timers, though not all the behavior is implemented: we still don't buffer the value of OCRx (it always updates Immediately), and TOV flag is always set on BOTTOM. This is not a regression, since we haven't implemented the OCRx / TOV behaviors before, even not for the 8-bit timers.

[gfeun]

Hey @urish,

So the timer1 works now ... but like a 8 bit one :smiley:

When i set OCR1A > 255, it doesn't work anymore. For example if OCR1A = 15624 (1 interrupt per second with a 1024 pre scaler and a 16Mhz µC ) it is treated as a u8: OCR1A = 8 and interrupts are called way more often :smile:

So I think it comes from the fact that internally all timer handling functions still assumes 8 bit: Beginning with OCR1A: https://github.com/wokwi/avr8js/blob/cf07a26ff7c6498cb8a435cbcb6229fefbf0a56e/src/peripherals/timer.ts#L53 It should be 16bits on 0x89 0x88, see p144 of the doc

Same for TCNT register: https://github.com/wokwi/avr8js/blob/cf07a26ff7c6498cb8a435cbcb6229fefbf0a56e/src/peripherals/timer.ts#L56 https://github.com/wokwi/avr8js/blob/cf07a26ff7c6498cb8a435cbcb6229fefbf0a56e/src/peripherals/timer.ts#L93

It should be 16 bits across 0x85 and 0x84, see p143 of doc

I don't understand how u8 and u16 types are handled since they are both defined as number. They don't seem to be treated differently. But it could also be necessary to update function signatures to take a u16 for 16 bit timers: https://github.com/wokwi/avr8js/blob/cf07a26ff7c6498cb8a435cbcb6229fefbf0a56e/src/peripherals/timer.ts#L196 https://github.com/wokwi/avr8js/blob/cf07a26ff7c6498cb8a435cbcb6229fefbf0a56e/src/peripherals/timer.ts#L234 https://github.com/wokwi/avr8js/blob/cf07a26ff7c6498cb8a435cbcb6229fefbf0a56e/src/peripherals/timer.ts#L319

[urish]

Hi Glenn, thanks for the detailed analysis!

Actually, u16 and u8 are both aliases for number, as you mentioned. They are leftovers from using AssemblyScript, which is supposed to compile TypeScript to Web Assembly. But we don't use that currently, so changing u8 to u16 will have no effect.

It probably has to be with the fact that we add CPU watches for a single address: https://github.com/wokwi/avr8js/blob/cf07a26ff7c6498cb8a435cbcb6229fefbf0a56e/src/peripherals/timer.ts#L201

I will update the code there to support 16 bit timers as well

[gfeun]

Thanks for the explanation !

Interesting to hear about assembly script, I'm learning about Web Assembly for work at the moment.

I'm going to test modifying CPU watches on my side too

[urish]

@gfeun I have updated the branch with some fixes, so hopefully now OCR1A should function as expected. Can you please have a look on the updated code?

[gfeun]

I think it works with the following changes:

diff --git a/src/peripherals/timer.ts b/src/peripherals/timer.ts
index ea66671..b3bc770 100644
--- a/src/peripherals/timer.ts
+++ b/src/peripherals/timer.ts
@@ -237,7 +237,7 @@ export class AVRTimer {
   set TCNT(value: u16) {
     this.cpu.data[this.config.TCNT] = value & 0xff;
     if (this.config.bits === 16) {
-      this.cpu.data[this.config.TCNT + 1] = (value >> 16) & 0xff;
+      this.cpu.data[this.config.TCNT + 1] = (value >> 8) & 0xff;
     }
   }

@@ -281,7 +281,7 @@ export class AVRTimer {
   private registerHook(address: number, hook: (value: u16) => void) {
     if (this.config.bits === 16) {
       this.cpu.writeHooks[address] = (value: u8) => {
-        hook(this.cpu.data[address + 1] | value);
+        hook((this.cpu.data[address + 1] << 8) | value);
       };
       this.cpu.writeHooks[address + 1] = (value: u8) => {
         hook((value << 8) | this.cpu.data[address]);

[gfeun]

Tested successfully with the following code runnable in the demo, and including my 2 changes, good job ! (I only tested the CTC mode)

void setup()
{
    // Builtin led as output
    DDRB |= 0x20;

    // initialize Timer1
    cli(); // disable global interrupts
    TCCR1A = 0; // set entire TCCR1A register to 0
    TCCR1B = 0; // same for TCCR1B

    // set compare match register to desired timer count:
    OCR1A = 15624;
    // turn on CTC mode:
    TCCR1B |= (1 << WGM12);
    // Set CS10 and CS12 bits for 1024 prescaler:
    TCCR1B |= (1 << CS10);
    TCCR1B |= (1 << CS12);
    // enable timer compare interrupt:
    TIMSK1 |= (1 << OCIE1A);
    // enable global interrupts:
    sei();
}

void loop()
{
}

ISR(TIMER1_COMPA_vect)
{
    PORTB ^= 0x20; // Toggle builtin led
}

[urish]

Yay!

Thanks :)

Can you try to come up with a failing test case(s) for the changes that you suggested?

[urish]

(I mean tests that fail with the current code, and pass after applying your changes)

[gfeun]

Update: I was wrong, this test passes for both with and without my modification.

    it('should set OCF0A flag when timer equals OCRA (16 bit mode)', () => {
      const timer = new AVRTimer(cpu, timer1Config);
      cpu.writeData(0x84, 0xff); // TCNT1 <- 0x00ff
      cpu.writeData(0x85, 0x00); // ...
      cpu.writeData(0x88, 0x00); // OCR1A <- 0x0100
      cpu.writeData(0x89, 0x01); // ...
      cpu.writeData(0x80, 0x0); // WGM1 <- 0 (Normal)
      cpu.writeData(0x81, 0x1); // TCCR1B.CS <- 1
      cpu.cycles = 1;
      timer.tick();
      expect(cpu.data[0x36]).toEqual(2); // TIFR0 should have OCF0A bit on
      expect(cpu.pc).toEqual(0);
      expect(cpu.cycles).toEqual(1);
    });

[urish]

Alright, I applied the fixes, added tests, fixed a few more issues I found - I think this is ready to go!

[gfeun]

Great, this also works on my side.

Thanks for finishing this, i couldn't write a test failing for the invalid code. I think I was missing the cpu.cycles = 2.

[urish]

Lovely! I will merge and release soon.

[urish]

Released as part of avr8js 0.8.0