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:

• Increment the counter.

• Find out the answer based on the counter.

• Ask the user for their answer.

• If the answer and the user's answer do not match, the user is incorrect. Reset the counter.

• 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 ANSWER = r22

.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;

I see a number of things which could help you improve your code.

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.

Know your instruction set

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.

Think very carefully about what your routines really need

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:
    rcall get_answer                ; calculate answer
    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
    cpse IO, ANSWER                 ; if IO != ANSWER
    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
    ldi ANSWER, FIZZ_PIN            ; tentatively set answer
    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."

Consider formatting comments differently

The semicolons for your comments are immediately after the end of the code line. This is technically fine (the assembler doesn't care) but it's not typically how human beings format code. Mor

StackExchange Code Review Q#98916, answer score: 11