OtherwiseThe 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.
-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 looksbut 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
OUT PORTD, 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 a 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 see. Considering that you are new to asm programming, I think with a little experience you will be an asm master!
