Assembly executing ButtonClick state only once

Question

I'm working on a simple button-led project using Atmega2560 microcontroller. I have a problem with the buttons. When I click the button, main loop stops working and button function keeps running infinite times as long as I'm pressing the button. Main-loop should not stop when I press the button. And the button function must only be run once. How can i do that ?

.def LEDS = R16
.def LED_DATA = R21

.org 0
    rjmp MAIN

MAIN:
    ldi LEDS, 0xFF  ; 0xFF = 1111 1111

    ldi LED_DATA, 0x01

    out DDRC, LEDS  ; PORTC
    sbi PORTB, 0
    sbi PORTB, 1

LOOP_MAIN:
    sbis PINB, 0        ; If PORTB-0's pin = 1, skip next
    rjmp BUTTON_CLICK_H

    sbis PINB, 1
    rjmp BUTTON_CLICK_Y 

    call DELAY
    out PORTC, LED_DATA

    lsl LED_DATA
    brcc SHIFT_0_IN     ;if carry not set, the MSB was not set, so skip setting the LSB
    ori LED_DATA, 1

    SHIFT_0_IN:         ; keep LSB as 0 -> do nothing, just continue
    rjmp LOOP_MAIN

    END:
    rjmp LOOP_MAIN


BUTTON_CLICK_H:
    lsl LED_DATA
    cpi LED_DARA, 0x40
    breq SPEED_RESET
    rjmp SPEED_END
    SPEED_RESET:
    ldi LED_SPEED, 0x04
    SPEED_END:
    rjmp LOOP_MAIN

EDIT: (11/26/2017)

I wrote a simple button controller. The purpose of this controller is, to check whether the button is pressed or not or kept pressing. button code should only work once. But it does not work when I press the button. Where am I making mistakes?

.def BTN_STATE_FIRST = R23
.def BTN_STATE_CHANGED = R24
.def BTN_STATE_PINB = R25
.def BTN_STATE_TEMP = R26

LOOP_MAIN:
    out PORTC, LED_DATA         
    call LOOP_BUTTON        
    call DELAY  

    ; ...Codes

    rjmp LOOP_MAIN

LOOP_BUTTON:

    ; (Is the button pressed, not pressed or kept pressed?) Controller

    in BTN_STATE_PINB, PINB     ; Read PINB data and put it current state in BTN_STATE_PINB
    mov BTN_STATE_TEMP, BTN_STATE_PINB  ; Move it to BTN_STATE_TEMP
    eor BTN_STATE_PINB, BTN_STATE_FIRST ; XOR BTN_STATE_PINB and BTN_STATE_FIRST. And write result to the BTN_STATE_PINB
    mov BTN_STATE_FIRST, BTN_STATE_TEMP ; Move it the BTN_STATE_FIRST
    breq BUTTON_PRESSED
    brne BUTTON_NOTPRESSED

    BUTTON_PRESSED:
    cpi BTN_STATE_PINB, 0x01    ; 1st button 0x01, 2nd button 0x02, 3rd button 0x04
    breq BUTTON_CHANGED
    rjmp BUTTON_NOTPRESSED

    BUTTON_CHANGED:
    cpi BTN_STATE_CHANGED, 0x01 ; When pressed and held, have been processed before ? 0x01 true, 0x00 false
    breq BUTTON_UP              ; If yes, branch to BUTTON_UP
    brne BUTTON_DOWN            ; Otherwise, branch to BUTTON_DOWN

    BUTTON_UP:
    dec BTN_STATE_CHANGED       ; Decrement the BTN_STATE_CHANGED to 0x00

    ldi LED_DATA, 0x40

    rjmp BUTTON_END

    BUTTON_DOWN:
    inc BTN_STATE_CHANGED       ; Increment the BTN_STATE_CHANGED to 0x01

    ldi LED_DATA, 0x80


    BUTTON_END:
    BUTTON_NOTPRESSED:

    ret

Answer 1

When you press button, the input pin signal is like:

___---------------------------------___--__--___-_______
   ^ here the press starts     ^ released  ^ bounces (physically)

Sometimes some bouncing may happen even at the beginning, or some noise in the main signal, if the contact is not solid enough.

If it would be perfect clean digital signal like:

_______-------------------------------_______________

then all you would need to do is to keep in main previous state of pin, the check whether button was clicked then looks like:

current reading | previous state | action
-------------------------------------------------------------------
 0              | 0              | N/A (or refresh "previous")
 1              | 0              | previous=1, call click function
 1              | 1              | N/A (or refresh "previous")
 0              | 1              | previous=0

But because of the physical bouncing of the actual switch in button, you will have to code more robust logic in the code, which upon "previous" bit change will reset also some "debounce timer", and until that timer (count-down counter) will reach zero, the "previous" state is locked, ignoring any state changes read on the real I/O line. So then debounced logic will turn:

___---------------------------------___--__--___-_______
   ^ here the press starts     ^ released  ^ bounces (physically)

into:

real-time in from button pin:
___-_--_------------------------------___--__--___-____________
"previous" state:
___-----------------------------------_________________________
"debounce timer": (active means "> 0") (preventing change of previous)
___--------------------_______________--------------------_____
action in code:
   *1                  *2             *3                  *4

Actions:

• *1: previous = 1, debounce = ~30ms, call onClick handler
• *2: debounce reached zero (till here "previous" was locked)
• *3: previous = 0, debounce = ~30ms
• *4: debounce reached zero (till here "previous" was locked - any button click up till now would be ignored)

---

And if you want to achieve illusion of "main not stopping", you need to keep the onClick handler very short (non blocking, non delaying), and keep any delaying logic for the "main", which can loop infinitely and update any timers/counters as needed (including the "debounce" timers for each input bit) and use additional complex logic to run short-quick functions upon certain events fired by either some timer or input state.

---

EDIT: some notes on the new code, which I did try to understand partially.

I have problem with your very basic architecture of that thing, it looks like you keep all values related to that button in fixed registers, which will make the LOOP_BUTTON fixed to the particular pin/button, not reusable elsewhere.

I would suggest you to design subroutines in more generic way, configuring specific functionality through arguments, not through code of subroutine (whenever it makes sense and the resulting code is reasonably simple).

I would design it in a way to take in one register the address of button object instance, and in another register value of pin, like:

DISCLAIMER: I was unable to verify if this is valid AVR asm syntax, or even if that code works, so use mostly as "idea" source (I used this link to write instructions and their syntax: http://www.avr-tutorials.com/sites/default/files/Instruction%20Set%20Summary.pdf ):

    ... main loop code continues with button tests...
    ; read current state of PINB into R23
    in      R23,PINB
    ; button 1 check (data in memory at button1_data, pin: bit 0 of PINB)
    ldi     ZH,high(button1_data)
    ldi     ZL,low(button1_data)
    ldi     R24,0b00000001      ; bit 0 bitmask
    rcall   BUTTON_HANDLER      ; R24 = 0/1 when onClick should be called
    sbrc    R24,0               ; skip onClick call, when result was 0
    rcall   BTN_1_ON_CLICK      ; BTN1 was clicked, call onClick handler
        ; ^^ must preserve R23!
    ; button 2 check (data in memory at button2_data, pin: bit 1 of PINB)
    ldi     ZH,high(button1_data)
    ldi     ZL,low(button1_data)
    ldi     R24,0b00000010      ; bit 1 bitmask
    rcall   BUTTON_HANDLER      ; R24 = 0/1 when onClick should be called
    sbrc    R24,0               ; skip onClick call, when result was 0
    rcall   BTN_2_ON_CLICK      ; BTN2 was clicked, call onClick handler
    ; button 3 check (data in memory at button3_data, pin: bit 2 of PINB)
    ldi     ZH,high(button1_data)
    ldi     ZL,low(button1_data)
    ldi     R24,0b00000100      ; bit 2 bitmask
    rcall   BUTTON_HANDLER      ; R24 = 0/1 when onClick should be called
    sbrc    R24,0               ; skip onClick call, when result was 0
    rcall   BTN_3_ON_CLICK      ; BTN3 was clicked, call onClick handler
    ... continuation of main loop ...

The button data are defined in .dseg as:

.dseg
button1_data:
    .byte   2    ; two bytes of storage per button
button2_data:
    .byte   2    ; two bytes of storage per button
button3_data:
    .byte   2    ; two bytes of storage per button

Don't forget to clear them during program init, probably like this:

main:
    ; during program init don't forget to clear button data in memory
    clr     R1                  ; R1 = 0
    sts     button1_data, R1
    sts     button1_data+1, R1  ; Not sure if this is legal syntax :/
    sts     button2_data, R1
    sts     button2_data+1, R1
    sts     button3_data, R1
    sts     button3_data+1, R1

Finally the button input handler routine, which will take in R23 current state of buttons (pins), R24 is bitmask of button to check, and Z should point to button data. It will return R24 = 0/1 in state whether button was clicked:

; button input handler:
; R23 = pins state (preserved), R24 = pin bitmask, Z = button data
; returns R24 = 0/1 when onClick should be called
;       button data structure: +0 = previous state, +1 = debounce timer
BUTTON_HANDLER:
    ; check debounce timer first, if > 0, state is locked
    ldd     R0,Z+1              ; debounce timer is second byte
    tst     R0                  ; is it zero?
    breq    BUTTON_HANDLER_DEBOUNCE_OK
    ; debounce timer is > 0, just decrement it and ignore input
    dec     R0
    std     Z+1,R0
    clr     R24                 ; R24 = 0 (no click)
    ret
BUTTON_HANDLER_DEBOUNCE_OK:
    ; process input
    ld      R0,Z                ; R0 = previous state of bit
    and     R24,R23             ; R24 = current state
    cpse    R0,R24              ; if previous == current, skip change
    rjmp    BUTTON_HANDLER_CHANGE_DETECTED
    clr     R24                 ; R24 = no click (no change on pin)
    ret
BUTTON_HANDLER_CHANGE_DETECTED:
    st      Z,R24               ; store new state into "previous" data

    ; EDIT - bugfix added, debounce timer need to be set too!
    ldi     R0,DEBOUNCE_DELAY   ; amount of main_loops to pass
    std     Z+1,R0

    tst     R24                 ; when new state is zero => released button
    breq    BUTTON_HANDLER_RELEASED ; return 0
    ldi     R24,1               ; when new state is non-zero, return 1 (click!)
BUTTON_HANDLER_RELEASED:
    ret

Then when some button was clicked you will call your "onClick" routine for particular button:

; button 1 onClick handler (must preserve R23 (input pins of buttons)).
BTN_1_ON_CLICK:
    ; TODO anything you wish to do upon BTN1 pressed
    ret

And define some debounce time constant DEBOUNCE_DELAY which is amount of main_loops to pass until the button will start to react to current state (if you loop for example once per 1ms, then you can try DELAY 30).

Oh wait, so instead of commenting your code, I just produced my own... even when I can't even verify it works... sorry. :)

(and if it works, then I guess it's not very efficient AVR assembly, as I felt I'm always hitting problems from a x86 point of view and missing instructions to help me, like why does AVR has so many instructions to set flags directly (zero/carry/... all of them), but not the other way, to set register to 0/1 according to flag, etc.)

Please let me know if it worked for you in some form + suggest fixes to my code to make it valid, to make this a bit better answer (if you will not respond, I would probably remove it over time, as I'm afraid it may be completely wrong).