Try using a value of 15999 for TOP in OCR1.
You do not need to account for the time it takes to execute the ISR. The timer is free running. In CTC mode, it starts at 0 and counts up to the TOP value, then clears back to zero and starts over again. It can optionally generate an interrupt each time it clears, but this is a side effect - it will count like this completely on its own with or without an interrupt.
The timer continues counting while your ISR executes. Your ISR will get called again the next time the clear happens regardless of how long it takes to execute, as long as it is finished executing before the next clear and the next interrupt.
Therefore, you can also eliminate the lines...
LDI WR1, BYTE1(988) ; 12 clock cycles are passed
LDI WR2, BYTE2(988) ; We want to wait 1000 ms
LDI WR3, BYTE3(988)
...since they have no effect.
Note that with the above code, your LED will not toggle exactly every 2 seconds because of jitter due to the fact that the foreground task can be interrupted at random times by the ISR. If it gets interrupted after the compare happens, the LED will not toggle until the next pass. You will still have a very solid 0.5Hz output frequency, there will just be a few dozen cycles of jitter on any given transition. If you want exactly the output to be jitter free +/- 1 cycle, you can enable one of the output compare pins to toggle mode and let the hardware blink the LED for you.
Make sense?
The final ASM code is very nice, but here are a few suggestions...
-I do not think you need the NOP after the Timer1comp vector. This NOP is actually landing in the next vector. While it does no harm here, it is at least superfluous.
-The CS01 bit in the line LDI WR1, (1<<WGM12)|(1<<CS10) ; Timer in CTC mode, TOP = OCR1A actually enables the timer and starts it counting. Since you have not yet assigned the OCR registers, they are set to thier initial values of 0, which means that a match occurs immediately. Again, this doesn't really cause any problems in the current program but once you start adding functionality it can make some hard to find bugs. Better to set up all the timer registers and then enable it as a final step when everything is ready and in a know state. This would involve changing the above line to LDI WR1, (1<<WGM12) and then adding an additional line LDI WR1, (1<<WGM12)|(1<<CS10) at the end of the setup.
-I would not manually set the output pins high at startup. Instead, let them get set at the first ms timer roll over. This will delay the start of the signal generation, but will make sure that the first pulse is the correct width. As written, the first pulse will be short by the time between when you start the timer and when you set the pins high. If you really must have the signal generation begin as soon as possible after reset, then you could set the pins high at the very beginning of the program, then initialize the TCNT to account for the lost time on the first pass.
-You can replace the lines...
IN WR1, PORTD ; Toggle port D
SER WR2
EOR WR1, WR2
OUT PORTD, WR1
...with the slightly more efficient...
SER WR2
OUT PIND, WR1
From the data sheet:
14.2.2 Toggling the Pin
Writing a logic one to PINxn toggles the value of PORTxn, independent on the value of DDRxn.
-Clearing the Tx registers in the main thread could cause a potential race condition and lost ticks. You do not know the value of Tx when you pass :Loop because they could in theory have been updated in the background by the ISR since the last time you looked at them. In practice, this is impossible with the current code but if you start adding stuff it would lead to hard to find bugs. Imagine, for instance, that the Tx increments from 1,000 to 1,001 between when you compare it and when you clear it. You have now lost this millisecond forever and then next pulse will be 1ms short. In general, it is considered bad practice to update a value from 2 different threads for this reason.
One way around this would be to subtract 1,000 from Tx at the top of loop rather than clearing it to 0. Another way around this would be to check for the match to 1,000 inside the ISR and instead just set a flag that the main loop then checks and clears (this is how the Ardunio timer code does it).
But I think for this code, the easiest and most efficient solution would be to move the increment, the test, the action, and the rest all into the ISR. While you generally want to do as little as possible in the ISR, by moving things around we can make the ISR still be very short. I see you appreciate carry bit juggling, so I instead of counting from 0 up to 1,000, I instead am counting from -1000 to 0. I think this makes slightly more efficient boundary checking inside the ISR. I also moved the Tx registers to be able to LDI them. Here is this final modified code...
.def WR1 = R16 ; the working registers
.def WR2 = R17
.def WR3 = R18
.def T1 = r19 ; A 24 bits value used to hold the
.def T2 = r20 ; number of milliseconds
.def T3 = r21 ; In T1 is the LSB, and in T3 the MSB
.cseg
.org $0000
RJMP Start
.org OC1Aaddr ; The address of the Timer1A compare match interrupt
JMP Timer1comp
.org INT_VECTORS_SIZE
;---------------;
; Settings ;
;---------------;
Start:
; Initialize the Stack Pointer
LDI WR1, HIGH(RAMEND)
OUT SPH, WR1
LDI WR1, LOW(RAMEND)
OUT SPH, WR1
LDI T1, BYTE1(-1000) ; Init the ms counter to -1000
LDI T2, BYTE2(-1000) ; we start a a negative number and count up becuase it
LDI T3, BYTE3(-1000) ; is more efficient to test if we got to 0
; Timer1A setup
LDI WR1, (1<<WGM12) ; Timer in CTC mode, TOP = OCR1A
STS TCCR1B, WR1
LDI WR1, HIGH(15999) ; Must wait 16000 clock cycles in order to
LDI WR2, LOW(15999) ; have a frequency of interrupt of 1 kHz.
STS OCR1AH, WR1 ; ISR doesn't interrupt timer, so have
STS OCR1AL, WR2 ; to set OCR1A to 16000 - 1
LDI WR1, 1 << OCIE1A ; Enable Output Compare A match interrupt
STS TIMSK1, WR1
; I/O settings
SER WR1 ; The LED is plugged on any pin of port D
OUT DDRD, WR1
; Enable Timer
LDS WR1, TCCR1B
ORI WR1, (1<<CS10) ; clock prescaler=1
STS TCCR1B, WR1
; Enable interrupts
SEI
;---------------;
; Main Loop ;
;---------------;
Loop:
RJMP Loop
;-----------------;
; Timer1A ISR ;
;-----------------;
Timer1comp:
; Save the current state on the stack
PUSH WR1
IN WR1, SREG
PUSH WR1
LDI WR1,1 ; Increment the ms counter LSB by 1
ADD T1,WR1
LDI WR1,0 ; Increment the rest by zero so only the carry will propigate
ADC T2,WR1
ADC T3,WR1
BRCC Timer1compEnd ; If carry is set, then we rolled the ms counter
;Reset the ms counter back to -1000
LDI T1, BYTE1(-1000) ; Init the ms counter
LDI T2, BYTE2(-1000)
LDI T3, BYTE3(-1000)
SER WR1
OUT PIND, WR1 ;; Toggle all PORTD output pins
Timer1compEnd:
; Reset the state
POP WR1
OUT SREG, WR1
POP WR1
RETI
This code is functionally equivalent but about 15% smaller, and eliminates some race conditions that could have turned into bugs in future versions. Let me know if I have missed something.
Again, let me reiterate that your original code was very nice and a lot cleaner than lots of commercial production code I've seen. Considering that you are new to asm programming, I think with a little experience you will quickly become an asm master!