14
\$\begingroup\$

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.
  3. Ask the user for their answer.
  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 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;                       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
    cpi ANSWER, IS_FIZZBUZZ
    breq main_correct
    rjmp main_incorrect

main_buzz:
    rcall get_answer
    cpi ANSWER, IS_BUZZ
    breq main_correct
    rjmp main_incorrect

main_fizz:
    rcall get_answer
    cpi ANSWER, IS_FIZZ
    breq main_correct
    rjmp main_incorrect

main_n:
    rcall get_answer
    cpi ANSWER, IS_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

get_answer:
    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

    ldi ANSWER, IS_FIZZBUZZ
    ret

ga_buzz:
    ldi BUZZ_COUNTER, BUZZ_VAL

    ldi ANSWER, IS_BUZZ
    ret

ga_fizz:
    ldi FIZZ_COUNTER, FIZZ_VAL

    ldi ANSWER, IS_FIZZ
    ret

ga_n:
    ldi ANSWER, IS_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:

  1. Read Pin A
  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.


Calculating the answer

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.

Instead, I am using subtraction to find the answer.

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?

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Jonathon Reinhart Aug 27 '15 at 1:54
  • \$\begingroup\$ @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. \$\endgroup\$ – SirPython Aug 27 '15 at 1:57
  • \$\begingroup\$ Hmm, nevermind then. I'm a long-time Stack Overflow-er, but clearly new to Code Review. \$\endgroup\$ – Jonathon Reinhart Aug 27 '15 at 2:00
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ – SirPython Aug 27 '15 at 2:02
  • \$\begingroup\$ Thanks! Asked. \$\endgroup\$ – Jonathon Reinhart Aug 27 '15 at 2:08
11
\$\begingroup\$

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. More typical is to choose some column and line up semicolons (if any). It's just a convention, but a very widespread and useful one. I think you'll find that most code will look like that.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – SirPython Aug 3 '15 at 19:13
  • \$\begingroup\$ 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. \$\endgroup\$ – Edward Aug 3 '15 at 19:19
  • \$\begingroup\$ Ah, I see. Thank you for clearing that up. \$\endgroup\$ – SirPython Aug 3 '15 at 19:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.