# Embedded FizzBuzz

Recently, I have started to enter the realms of embedded systems programming. And, as my first major project, I thought I'd do the obvious: FizzBuzz.

However, this is a little different: this is a game.

Like my FizzBuzz Challenge, the user must enter either Fizz, Buzz, FizzBuzz, or the number itself. Here is the general process of how the game works:

1. Increment the counter.
2. Find out the answer based on the counter.
4. If the answer and the user's answer do not match, the user is incorrect. Reset the counter.
5. Goto 1.

However, unlike my original FizzBuzz challenge, there is no place for the user to enter their answer on a keyboard: they must use buttons attached to the micro-controller.

# Setup

The micro-controller is setup like this:

Port A

• Pin 1: "N"
• Pin 2: "Fizz"
• Pin 3: "Buzz"
• Pin 4: "FizzBuzz"
• Pin 5: "Reset"

Port B

• Pin 1: "Incorrect!"
• Pin 2: "Correct!"

All the pins on Port A are buttons, while all the pins on Port B are LEDs.

# Code

/*
* Embedded_FizzBuzz.asm
*
*  Created: 7/26/2015 3:36:54 PM
*  Author: SirPython
*/

.INCLUDE "m16def.inc"

.EQU INPUT = 0
.EQU OUTPUT = 0xFF

.EQU N_PIN = 0b00000001
.EQU FIZZ_PIN = 0b00000010
.EQU BUZZ_PIN = 0b00000100
.EQU FIZZBUZZ_PIN = 0b00001000
.EQU RESET_PIN = 0b00010000

.EQU INCORRECT_LED_PIN = 0
.EQU CORRECT_LED_PIN = 1

.DEF COUNTER = r18

.DEF FIZZBUZZ_COUNTER = r19
.DEF BUZZ_COUNTER = r20
.DEF FIZZ_COUNTER = r21

.DEF TIMER_COUNTER1 = r23
.DEF TIMER_COUNTER2 = r24

.DEF IO = r16

.EQU DELAY_COUNT = 0xFF

.EQU FIZZBUZZ_VAL = 15
.EQU BUZZ_VAL = 5
.EQU FIZZ_VAL = 3

.EQU IS_FIZZBUZZ = 3
.EQU IS_BUZZ = 2
.EQU IS_FIZZ = 1
.EQU IS_N = 0

.ORG 0
rjmp init

init:
ldi IO, low(RAMEND);                initialize the stack (
out SPL, IO
ldi IO, high(RAMEND)
out SPH, IO;                        )

ldi IO, INPUT
out DDRA, IO;                       setup A as input
ldi IO, OUTPUT
out DDRB, IO;                       setup B as output

ldi COUNTER, 1

ldi FIZZBUZZ_COUNTER, FIZZBUZZ_VAL
ldi BUZZ_COUNTER, BUZZ_VAL
ldi FIZZ_COUNTER, FIZZ_VAL

main:
in IO, PINA

cpi IO, RESET_PIN
breq main_reset
cpi IO, FIZZBUZZ_PIN
breq main_fizzbuzz
cpi IO, BUZZ_PIN
breq main_buzz
cpi IO, FIZZ_PIN
breq main_fizz
cpi IO, N_PIN
breq main_n

rjmp main

main_reset:
ldi COUNTER, 1
ldi FIZZBUZZ_COUNTER, FIZZBUZZ_VAL
ldi BUZZ_COUNTER, BUZZ_VAL
ldi FIZZ_COUNTER, FIZZ_VAL

rjmp main

main_fizzbuzz:
rcall get_answer;                   get_answer must be called for each option (fizzbuzz, fizz, etc) so the counters are not falsely decreased
breq main_correct
rjmp main_incorrect

main_buzz:
breq main_correct
rjmp main_incorrect

main_fizz:
breq main_correct
rjmp main_incorrect

main_n:
breq main_correct
rjmp main_incorrect

main_correct:
inc COUNTER
rcall correct_led
rjmp main

main_incorrect:
call incorrect_led
rjmp main_reset

;--------------------------------------------------
; Sets ANSWER to the appropriate value based on input
;
; IN: COUNTER = value to check against
; OUT: ANSWER =  IS_FIZZBUZZ, IS_BUZZ, IS_FIZZ, IS_N
; REG: FIZZBUZZ_COUNTER, BUZZ_COUNTER, FIZZ_COUNTER
; ERR: NONE

dec FIZZBUZZ_COUNTER
dec BUZZ_COUNTER
dec FIZZ_COUNTER

cpi FIZZBUZZ_COUNTER, 0
breq ga_fizzbuzz

cpi BUZZ_COUNTER, 0
breq ga_buzz

cpi FIZZ_COUNTER, 0
breq ga_fizz

rjmp ga_n

ga_fizzbuzz:
ldi FIZZBUZZ_COUNTER, FIZZBUZZ_VAL
ldi BUZZ_COUNTER, BUZZ_VAL
ldi FIZZ_COUNTER, FIZZ_VAL

ret

ga_buzz:
ldi BUZZ_COUNTER, BUZZ_VAL

ret

ga_fizz:
ldi FIZZ_COUNTER, FIZZ_VAL

ret

ga_n:
ret

;--------------------------------------------------
; Stops program execution for a few seconds
;
; IN: NONE
; OUT: NONE
; REG: TIMER_COUNTER
; ERR: NONE

stop_execution:
ldi TIMER_COUNTER1, DELAY_COUNT
ldi TIMER_COUNTER2, DELAY_COUNT

se_stall:
dec TIMER_COUNTER1
cpi TIMER_COUNTER1, 0
brne se_stall

se_finished:
ldi TIMER_COUNTER1, DELAY_COUNT
dec TIMER_COUNTER2

cpi TIMER_COUNTER2, 0
brne se_stall

;--------------------------------------------------
; Turns on the "correct" LED
;
; IN: NONE
; OUT: NONE
; REG: IO
; ERR: NONE

correct_led:
sbi PORTB, CORRECT_LED_PIN

rcall stop_execution

cbi PORTB, CORRECT_LED_PIN
ret

;--------------------------------------------------
; Turns on the "incorrect" LED
;
; IN: NONE
; OUT: NONE
; REG: IO
; ERR: NONE

incorrect_led:
sbi PORTB, INCORRECT_LED_PIN

rcall stop_execution

cbi PORTB, INCORRECT_LED_PIN
ret


# Program Flow

Here is the general flow of the code, which is almost the same as the general game flow written above:

2. If it's Reset, goto 7
3. If it's N, goto 8
4. If it's Fizz, goto 9
5. If it's Buzz, goto 10
6. If it's FizzBuzz, goto 11
7. Reset the counter. Goto 1
8. Get the answer. If it's N, goto 12. If not, goto 13.
9. Get the answer. If it's Fizz, goto 12. If not, goto 13.
10. Get the answer. If it's Buzz, goto 12. If not, goto 13.
11. Get the answer. If it's FizzBuzz, goto 12. If not, goto 13. 12: Increment the counter. Light "Correct!", then goto 1.
12. Reset the counter. Light "Incorrect!", then goto 1.

As you can see, there is quite some repetition in here.

Originally, to calculate the answer, my first thought was to do some %ing around with the counter. However, this proved to be difficult with this instruction set.

Since we already know that we're looking for numbers divisible by 3, 5 or both, what would make far more sense is to simple keep countdown counters for both. - Edward.

To start, under the init label, I initialize three counters (FIZZBUZZ_COUNTER, BUZZ_COUNTER, and FIZZ_COUNTER) to their respective starting values. Then, every time get_answer is "called", these are decremented and checked against.

# Tools

I am not in possession of an actual micro-controller myself. I am using Atmel Studio's Simulator for the ATmega16 micro-controller.

I am not 100% how this will behave on an actual micro-controller, but after thorough testing on the simulator, the code works to the best of my knowledge.

# Workflow

Using Atmel studio's debugger, I setup breakpoints on...

• Line 66
• Line 82
• Line 91
• Line 97
• Line 103
• Line 109
• Line 114
• Line 119
• Line 147
• Line 155
• Line 161
• Line 167
• Line 204
• Line 220

So basically, I setup breakpoints under the LED subroutines and under the labels and sub-labels of main and get_answer.

Then, I activated the debugger using ALT + F5, opened up ports A and B in the IO view, and opened up the register view.

Now for testing, I manually flipped on and flipped off bits in the IO view and hit F5 to advance execution to the next breakpoint.

# Concerns

This is my first time doing any sort of embedded work, however this is not my first time doing assembly; which is more of a curse, because I am used to a higher level of assembly code.

Below are just a few concerns, I strongly encourage anything else that jumps out.

• Is my method of timing acceptable?

Right now, for timing, I am using a busy loop. However, since I am using the simulator, I'm not quite sure how effective this timing is. Would it be better to try and use a timer interrupt or a built in clock for timing, or is that overkill for such a simple task?

• I feel as though I am repeating myself quite a lot in this code. Is there any way I can add more structure to it, and reduce the constant "if/else if"s?

• Looking back at the code, I realize I don't have that many comments. Is this okay, or are there some things that are still pretty confusing?

• I think questions asking about assembly should always include an architecture tag. When you look at assembly for many different architectures all day, it's not always immediately discernible by a quick glance at the source code. – Jonathon Reinhart Aug 27 '15 at 1:54
• @JonathonReinhart If you read the "Tools" section, you can see that this is for the ATmega16. Perhaps, however, I could move that to the top of the code; that might be better. However, there are currently no tags for architectures. If you would like to share your reasoning, please open a post on meta. – SirPython Aug 27 '15 at 1:57
• Hmm, nevermind then. I'm a long-time Stack Overflow-er, but clearly new to Code Review. – Jonathon Reinhart Aug 27 '15 at 2:00
• @JonathonReinhart Welcome to Code Review, then! I strongly suggest that you do create the meta post, though. Here at Code Review, we are very friendly people and will give you honest feedback on your meta post. Plus, by creating it, you are being an invaluable help to the construction of this community. – SirPython Aug 27 '15 at 2:02
• Thanks! Asked. – Jonathon Reinhart Aug 27 '15 at 2:08

## Minimize register usage

With assembly language programming, and in particular in embedded systems work, minimizing the use of resources is often vital. One of the most precious resources is the processor's registers. In this case there are only 32 of them, so minimizing their use is often important. In this case, there is no need to have a separate FIZZBUZZ_COUNTER because it's actually just a combination of the events that BUZZ_COUNTER and FIZZ_COUNTER are zero. Use logic instead of an additional counter.

## Don't Repeat Yourself (DRY)

There are two places in the code that look like this:

ldi COUNTER, 1
ldi FIZZBUZZ_COUNTER, FIZZBUZZ_VAL
ldi BUZZ_COUNTER, BUZZ_VAL
ldi FIZZ_COUNTER, FIZZ_VAL


The second of those, at label main_reset can be completely eliminated. Just put the main_reset label immediately above the first ldi COUNTER, 1 line.

The get_answer routine start with this:

get_answer:
dec FIZZBUZZ_COUNTER
dec BUZZ_COUNTER
dec FIZZ_COUNTER

cpi FIZZBUZZ_COUNTER, 0
breq ga_fizzbuzz


However, the dec instruction already sets the Z flag if the resulting value is zero. This means that none of the cpi instructions are needed at all. Each branch can instead immediately follow the corresponding decrement.

The point to the program is to compare the calculated internal value to the value from the external pins and to get a single bit result (correct or incorrect). Comparing two values and getting a single bit result can be done in a single instruction: CPSE. This instruction is "Compare, Skip if Equal" which can be use very effectively for this case.

main:
in IO, PINA                     ; get user's answer
sbrc IO, RESET_BIT              ; if reset is set
rjmp main_reset                 ;    do a reset
ldi LEDSTATE,INCORRECT          ; assume incorrect
ldi LEDSTATE,CORRECT_LED_PIN    ;    it's incorrect
; now set the LED according to LEDSTATE
sbi PORTB, LEDSTATE             ; turn on LED
rcall stop_execution            ; delay
cbi PORTB, LEDSTATE             ; turn off LED
rjmp main


## Make routines match data requirements

The code above assumes that get_answer has the exact same bit pattern as the port pins, but since it's all under your control, why not? Here's how it might be done:

get_answer:
clt                             ; clear FIZZ flag
ldi ANSWER, N_PIN               ; assume N
dec FIZZ_COUNTER                ;
brne not_fizz                   ; keep going if not zero
ldi FIZZ_COUNTER, FIZZ_VAL      ; reload counter
set                             ; set FIZZ flag
not_fizz:
dec BUZZ_COUNTER                ;
brne not_fizzbuzz               ; keep going if not zero
ldi BUZZ_COUNTER, BUZZ_VAL      ; reload counter
ldi ANSWER, BUZZ_PIN            ; tentatively set buzz
brtc not_fizzbuzz               ;
ldi ANSWER, FIZZBUZZ_PIN        ; it was fizzbuzz
not_fizzbuzz:
ret


## Inline code that's only used once

It's handy to have things broken into subroutines, but if they're only used once, eliminating the time and code space overhead of a function call is easily done by simply inlining the code. In the rewritten code above, both stop_execution and get_answer can be inlined.

## Don't keep values that aren't needed

It may seem paradoxical, but the one value that isn't really needed in this program is COUNTER. The state is simply inferred by the two counters that are actually used. It can be completely eliminated (in either your original version or the rewrite) with no loss of functionality.

## Consider real world hardware and interface

I know you're new at this and don't have real hardware yet, but in real life there would be problems with this code. The problem is that if the input were via plain pushbutton switches, the inputs would need to be debounced and that's typically done in software. The other problem is that there is no human-perceptible delay between the time the result is turned off and when the input is read. This means that the human will have to be as fast as the computer or that the human must put in the next value as the appropriate LED is still lit for the previous value. An alternative approach would be to allow the human to advance the count instead of the computer. A button press could signify both "I have my answer" and "advance to the next number."

• Thanks for mentioning the thing about the debounce; I was going to leave that as a note to potential testers in my post. In the code snippet of Think very carefully about what your routines really need, you are calling get_answer every time main loops. However, get_answer decrements the counters every call so this would quickly drain the counters. – SirPython Aug 3 '15 at 19:13
• As I mentioned in Consider real world hardware and interface, the only pacing is done by the delay after the LED is set, as with your original version. Unlike the original code, however, get_answer is only called once each loop; the net effect is the same as the original version. – Edward Aug 3 '15 at 19:19