Make LED blink using AVR assembly

Question

I have an ATmega328P Arduino and am trying to make the LED blink using assembly without any additional includes. The goal is to have it blink for 1 second on, then 1 second off, however, it blinks incredibly quickly and is barely perceptible. Am I doing something incorrectly?

; DDRB = 0x04
; PORTB = 0x05
; TCCROB = 0x25
; TCNT0 = 0x26
; 0b00000000 = OFF
; 0b00100000 = ON
setup:
 ldi r16, 0b00100000 ; Set r16 to LED ON value
 out 0x04, r16 ; Set DDRB to output
 out 0x05, r16 ; Set PORTB to ON
 ldi r16, 0b00000101 ; Set r16 with prescaler 1024 value
 out 0x25, r16 ; Set the TCCROB to 1024
 ldi r20, 0 ; Reset r20
 ldi r21, 1 ; Hold value 1 to increment r20
loop:
 in r17, 0x26 ; Read the timer 
 cpi r17, 128 ; Check to see if the timer is halfway done (1/2 second, max 255 value)
 brbc 0, increment ; If r17 <= 128 (max 255 val before resets to 0), invert current LED state
 rjmp loop
increment:
 adc r20, r21 ; Add r21 (1) to r20 (with carry)
 brbs 0, reset ; If the carry branch is set, reset the register
 rjmp loop ; Otherwise, run the clock
reset:
 ldi r20, 0 ; Reset the delay register
 cpi r16, 0b00000000 ; Check if r16 is set to LED OFF
 brbs 1, switchon ; If it is, change it to ON
 ldi r16, 0b00000000 ; Otherwise, change it to OFF
 out 0x05, r16 ; Set the LED to the new state
 rjmp loop ; Run the clock
switchon:
 ldi r16, 0b00100000 ; Set r16 to LED ON value
 out 0x05, r16 ; Set the LED to the new state
 rjmp loop ; Run the clock

• Josue Espinosa: It's part of my code. I thought that line should be there, but when it isn't commented, it doesn't blink, it remains solidly lit (as far as I can tell).
• aaa: How long is your delay? It might be too small to see the LED blink.
• Edgar Bonet: "incredibly quickly"? It's so slow I can even see it blinking!

Answer 1

There is a bug with the logic of your program: once TCNT0 gets larger than 128, it will continue to be larger than 128 for quite some time... during which time you are continuously incrementing your r20 counter. The simplest fix would be to reset TCNT0 right after the increment label, with something like out TCNT0, zero_reg. Then you may want to compare TCNT0 with 61 instead of 128, in order to get closer to the desired blink frequency of 0.5 Hz.

Other than that, there are quite a few problems in the way you write your code. jwpat7 has already pointed out some of them, and here are more:

• You should not increment a register with adc, but with plain add. Both instructions update the carry bit, but adc also adds the previous value of the carry bit to the target register.
• Better yet, increment with inc, as it's purpose is more obvious. But then you should test the zero bit, as inc does not update the carry bit.
• There is little point in resetting r20 once it has overflowed, as it is zero at this point.

There are also quite a few ways the program could me made simpler. The simplest would probably be to configure timer 1 to deliver an interrupt every second, and toggle the LED in the ISR. If you do not want interrupts, you can configure counter 1 in much the same way, only without the "interrupt enable" bit, and check the interrupt flag in a busy loop.

Below is a slightly simplified version of your program. I tried to keep it as close as possible to yours, without using timer 1, nor interrupts, and not even the interrupt flag. The code checks for timer 0 overflowing by comparing its current value with the previous one.

.set PINB, 0x03
.set DDRB, 0x04
.set TCCR0B, 0x25
.set TCNT0, 0x26
.set LED_MASK, 0b00100000
.set PS_1024, 0b00000101
setup:
 ldi r16, PS_1024 ; Set r16 with prescaler 1024 value
 out TCCR0B, r16 ; Set the TCCROB to 1024
 ldi r16, LED_MASK ; Set r16 to the LED bit
 out DDRB, r16 ; Set LED pin to output
 clr r18 ; Clear the saved timer
loop:
 ldi r20, 61 ; Initialize our software counter
check_timer:
 in r17, TCNT0 ; Read the timer
 cp r17, r18 ; Compare with previous value
 mov r18, r17 ; Save current value
 brsh check_timer ; unless the timer has decreased, repeat
decrement:
 dec r20 ; decrement the software counter
 brne check_timer ; if not zero, go back to checking the timer
toggle:
 out PINB, r16 ; toggle the LED
 rjmp loop

If we can use the timer overflow interrupt flag, then the test is simpler, and we do not need to store the previous and current values of the timer in registers:

check_timer:
 sbis TIFR0, TOV0 ; unless the timer has overflowed
 rjmp check_timer ; check again for overflow
 sbi TIFR0, TOV0 ; otherwise clear the overflow flag

• Josue Espinosa: Thank you. I am new to assembly and the low-level mindset and this was very informative and helpful. The reason I didn't use more self-documenting instructions (like dec) is because I'm limited to an instruction subset as a challenge for an assignment.
• Josue Espinosa: Would you mind detailing a little bit more on why you use the value 61 exactly?
• Edgar Bonet: @JosueEspinosa: The Uno is clocked at F_CPU = 16 MHz. The timer ticks at F_T0 = F_CPU/1024. It overflows with a frequency F_TOV0 = F_T0/256. You want the toggling of the LED to happen at 1 Hz ≈ F_TOV0/61.
• James Waldby - jwpat7: Just to clarify what out PINB, r16 does, Atmel datasheets say "Writing a logic one to PINxn toggles the value of PORTxn, independent of the value of DDRxn."

Answer 2

It appears that your intent is to set TCCROB up with a 1024x prescale, and each time it counts up to 128, increment r20, and toggle the LED each time r20 overflows to zero.

That sequence ought to take 1024/16M · 128 · 256 seconds, or about 2.1 seconds, a fairly leisurely toggle rate. If instead it's flashing a hundred or more times too fast, probably one of the comparisons is backwards.

Some of the following items might make it easier to debug the problem:

• Instead of brbc 0,..., say brcc ...
• Instead of brbs 0,..., say brcs ...
• Instead of brbs 1,..., say breq ...

That is, use proper mnemonic branch instruction names, rather than referencing SREG bits.

• If my arithmetic was correct and your branch tests were fixed, your rate would be off by a factor of two. To fix this problem, you could replace one of the more problematic tests (testing for carry clear after cpi r17, 128) with cpi r17, 0 followed by a breq, and change the prescale factor to 256 instead of 1024, which gives 256/16M · 256 · 256 seconds, or about 1.05 seconds.

• Use named constants for register names instead of 0x04, 0x05, 0x25, and 0x26.

• Instead of your lengthy toggle sequence use EOR to toggle the value. That is, replace

 cpi r16, 0b00000000 ; Check if r16 is set to LED OFF
 brbs 1, switchon ; If it is, change it to ON
 ldi r16, 0b00000000 ; Otherwise, change it to OFF
 out 0x05, r16 ; Set the LED to the new state
 rjmp loop ; Run the clock
switchon:
 ldi r16, 0b00100000 ; Set r16 to LED ON value
 out 0x05, r16 ; Set the LED to the new state
 rjmp loop ; Run the clock

with

 eor r18, r16 ; Toggle LED bit
 out PORTB, r18 ; Set the LED to the new state
 rjmp loop ; Go run the clock

where r16 and r18 are initialized as follows in setup:

 ldi r16, 0b00100000 ; bit mask for LED pin
 mov r18, r16