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.SetupThe micro-controller is setup like this:Port APin 1: NPin 2: FizzPin 3: BuzzPin 4: FizzBuzzPin 5: ResetPort BPin 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 initinit:    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_VALmain:    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 mainmain_reset:    ldi COUNTER, 1    ldi FIZZBUZZ_COUNTER, FIZZBUZZ_VAL    ldi BUZZ_COUNTER, BUZZ_VAL    ldi FIZZ_COUNTER, FIZZ_VAL    rjmp mainmain_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_incorrectmain_buzz:    rcall get_answer    cpi ANSWER, IS_BUZZ    breq main_correct    rjmp main_incorrectmain_fizz:    rcall get_answer    cpi ANSWER, IS_FIZZ    breq main_correct    rjmp main_incorrectmain_n:    rcall get_answer    cpi ANSWER, IS_N    breq main_correct    rjmp main_incorrectmain_correct:    inc COUNTER    rcall correct_led    rjmp mainmain_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: NONEget_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_nga_fizzbuzz:    ldi FIZZBUZZ_COUNTER, FIZZBUZZ_VAL    ldi BUZZ_COUNTER, BUZZ_VAL    ldi FIZZ_COUNTER, FIZZ_VAL    ldi ANSWER, IS_FIZZBUZZ    retga_buzz:    ldi BUZZ_COUNTER, BUZZ_VAL    ldi ANSWER, IS_BUZZ    retga_fizz:    ldi FIZZ_COUNTER, FIZZ_VAL    ldi ANSWER, IS_FIZZ    retga_n:    ldi ANSWER, IS_N    ret;--------------------------------------------------; Stops program execution for a few seconds;; IN: NONE; OUT: NONE; REG: TIMER_COUNTER; ERR: NONEstop_execution:    ldi TIMER_COUNTER1, DELAY_COUNT    ldi TIMER_COUNTER2, DELAY_COUNTse_stall:    dec TIMER_COUNTER1    cpi TIMER_COUNTER1, 0    brne se_stallse_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: NONEcorrect_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: NONEincorrect_led:    sbi PORTB, INCORRECT_LED_PIN    rcall stop_execution    cbi PORTB, INCORRECT_LED_PIN    retProgram FlowHere is the general flow of the code, which is almost the same as the general game flow written above:Read Pin AIf it's Reset, goto 7If it's N, goto 8If it's Fizz, goto 9If it's Buzz, goto 10If it's FizzBuzz, goto 11Reset the counter. Goto 1Get the answer. If it's N, goto 12. If not, goto 13.Get the answer. If it's Fizz, goto 12. If not, goto 13.Get the answer. If it's Buzz, goto 12. If not, goto 13.Get the answer. If it's FizzBuzz, goto 12. If not, goto 13. 12: Increment the counter. Light Correct!, then goto 1.Reset the counter. Light Incorrect!, then goto 1.As you can see, there is quite some repetition in here.Calculating the answerOriginally, 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.ToolsI 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.WorkflowUsing Atmel studio's debugger, I setup breakpoints on...Line 66Line 82Line 91Line 97Line 103Line 109Line 114Line 119Line 147Line 155Line 161Line 167Line 204Line 220So 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.ConcernsThis 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 ifs?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?
Embedded FizzBuzz
game;fizzbuzz;assembly;embedded
I see a number of things which could help you improve your code.Minimize register usageWith 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, 1ldi FIZZBUZZ_COUNTER, FIZZBUZZ_VALldi BUZZ_COUNTER, BUZZ_VALldi FIZZ_COUNTER, FIZZ_VALThe 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 setThe get_answer routine start with this:get_answer:    dec FIZZBUZZ_COUNTER    dec BUZZ_COUNTER    dec FIZZ_COUNTER    cpi FIZZBUZZ_COUNTER, 0    breq ga_fizzbuzzHowever, 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 needThe 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 mainMake routines match data requirementsThe 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 flagnot_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 fizzbuzznot_fizzbuzz:    retInline code that's only used onceIt'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 neededIt 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 interfaceI 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 differentlyThe 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.