Question about incrementing and holding a value

Discussion in 'Embedded Systems and Microcontrollers' started by hunterage2000, Jun 11, 2016.

  1. hunterage2000

    Thread Starter Active Member

    May 2, 2010
    400
    0
    I've written the below code and I'm wondering why the variable led_state is not holding a shifted value when a button is pressed. Initially bit0 of 4 leds is on then when the button is pressed the led is shifted left so bit1 is on then bit2 then bit3 etc but after a while the led state go's back to bit0. I was expecting when button is 0 it would keep on returning the shifted value. Can anyone tell me why its not? I was thinking that it might have something to do with the scope of led_state but it is declared as a global variable.

    Code (Text):
    1. #pragma config FOSC = INTOSCIO
    2. #include <stdio.h>
    3. #include <pic16f785.h>
    4. #include <xc.h>
    5. #include <stdlib.h>
    6.  
    7. struct led_addr_bits {
    8. unsigned bit7 : 1;
    9. unsigned bit6 : 1;
    10. unsigned bit5 : 1;
    11. unsigned bit4 : 1;
    12. unsigned bit3 : 1;
    13. unsigned bit2 : 1;
    14. unsigned bit1 : 1;
    15. unsigned bit0 : 1;
    16. }led_addr_bits;
    17.  
    18. char inc_led_state(char);
    19. char led_state = 0x01;
    20. char inc_state;
    21. int main()
    22. {
    23. OSCCON = 0x39;
    24.  
    25. ANSEL1bits.ANS11 = 0;
    26. TRISBbits.TRISB5 = 1;
    27. TRISCbits.TRISC6 = 0;
    28. TRISCbits.TRISC7 = 0;
    29. TRISBbits.TRISB7 = 0;
    30.  
    31. while(1)
    32. {
    33. led_state = inc_led_state(led_state);
    34.  
    35.  
    36. led_addr_bits.bit3 = led_state >> 3;
    37. led_addr_bits.bit2 = led_state >> 2;
    38. led_addr_bits.bit1 = led_state >> 1;
    39. led_addr_bits.bit0 = led_state;
    40.  
    41. PORTBbits.RB7 = led_addr_bits.bit0;
    42. PORTCbits.RC7 = led_addr_bits.bit1;
    43. PORTCbits.RC6 = led_addr_bits.bit2;
    44. PORTCbits.RC3 = led_addr_bits.bit3;
    45.  
    46. }
    47. }
    48.      
    49.  
    50. char inc_led_state(char led_state)
    51. {
    52.     if(PORTBbits.RB5==0)
    53.     {
    54.     inc_state = led_state;
    55.     }
    56.    
    57.     if(PORTBbits.RB5==1)
    58.     {
    59.     while(PORTBbits.RB5==1);
    60.    
    61.     inc_state = led_state << 1;
    62.     }
    63.     return inc_state;
    64. }
    65.      
    66.    
    67.  
    68.  
     
  2. MrChips

    Moderator

    Oct 2, 2009
    12,442
    3,361
    As far as I can see the way your code is written, when the one is shifted to the left led_state will eventually become 00000000 again.
    How does it ever get back to 00000001?
     
  3. hunterage2000

    Thread Starter Active Member

    May 2, 2010
    400
    0
    This was just to see if the value would hold. If it did I would of made it not go beyond bit3 and have a increment right button to go back to and not go beyond bit0.
     
  4. MrChips

    Moderator

    Oct 2, 2009
    12,442
    3,361
    So what LED is lit when the button is pressed once?
     
  5. hunterage2000

    Thread Starter Active Member

    May 2, 2010
    400
    0
    If I press once led bit0 go's off and led bit1 go's on. I am thinking that once the button is not 1, the value that was incremented inside the if block is lost so returns back to the original value of 1. I'm assuming this has something to do with block scope but not sure how to get round this.
     
  6. dannyf

    Well-Known Member

    Sep 13, 2015
    1,811
    362
    then code to that.

    You probably want to read an intro-level book on C and definitely read the compiler manual. Your code doesn't do what you wanted it to do.
     
  7. hunterage2000

    Thread Starter Active Member

    May 2, 2010
    400
    0
    Yeah I've looked at a few books and I think the problem is led_state loses its incremented value when button = 0. I was thinking maybe send the led_state within the if statement and retrieve it in the while loop.
     
  8. Kermit2

    AAC Fanatic!

    Feb 5, 2010
    3,789
    945
    Sounds backwards to me. Wouldn't you want to sense the LED state in the IF statement?
    THEN code the response you expect for the sensed state?
    Consider addind a prior IF check for the exception state. I think it is the 0000000... state here, to force the desired 00000001 state before your IF loop executes.

    Disclaimer. I don't know the code being used, but I understand programming.
     
  9. hunterage2000

    Thread Starter Active Member

    May 2, 2010
    400
    0
    I tried this but got the same. Can someone explain why the value doesn't hold? It must be something to do with scope.

    Code (Text):
    1. #pragma config FOSC = INTOSCIO
    2. #include <stdio.h>
    3. #include <pic16f785.h>
    4. #include <xc.h>
    5. #include <stdlib.h>
    6.  
    7. struct led_addr_bits {
    8. unsigned bit7 : 1;
    9. unsigned bit6 : 1;
    10. unsigned bit5 : 1;
    11. unsigned bit4 : 1;
    12. unsigned bit3 : 1;
    13. unsigned bit2 : 1;
    14. unsigned bit1 : 1;
    15. unsigned bit0 : 1;
    16. }led_addr_bits;
    17.  
    18. char inc_led_state(char led_state);
    19. char led_state = 0x01;
    20. char inc_state;
    21. int main()
    22. {
    23. OSCCON = 0x39;
    24.  
    25. ANSEL1bits.ANS11 = 0;
    26. TRISBbits.TRISB5 = 1;
    27. TRISCbits.TRISC6 = 0;
    28. TRISCbits.TRISC7 = 0;
    29. TRISBbits.TRISB7 = 0;
    30.  
    31. while(1)
    32. {
    33.  
    34.     if(PORTBbits.RB5==0)
    35.     {
    36.     inc_led_state(led_state);
    37.     }
    38.  
    39.     if(PORTBbits.RB5==1)
    40.     {
    41.     while(PORTBbits.RB5==1);
    42.     led_state = led_state << 1;
    43.     inc_led_state(led_state);
    44.     }
    45.  
    46. led_addr_bits.bit3 = led_state >> 3;
    47. led_addr_bits.bit2 = led_state >> 2;
    48. led_addr_bits.bit1 = led_state >> 1;
    49. led_addr_bits.bit0 = led_state;
    50.  
    51. PORTBbits.RB7 = led_addr_bits.bit0;
    52. PORTCbits.RC7 = led_addr_bits.bit1;
    53. PORTCbits.RC6 = led_addr_bits.bit2;
    54. PORTCbits.RC3 = led_addr_bits.bit3;
    55.  
    56. }
    57. }
    58.      
    59.  
    60. char inc_led_state(char led_state)
    61. {
    62.     return led_state;
    63.    
    64. }
    65.      
    66.    
    67.  
    68.  
     
  10. dannyf

    Well-Known Member

    Sep 13, 2015
    1,811
    362
    i'm sure you did but none made it to your code.

    Here is a short list of what I think is wrong:

    1) you didn't read the compiler manual: or you wouldn't have included the header files the way you did;
    2) you didn't know the typical workflow for a PIC or your would have set up the configuration fuses correctly;
    3) you didn't read the data sheet or you would have initiated the device correctly;
    4) you didn't understand C or you would have assign values to the bits differently;
    5) you didn't thought through the logic of your code;
    6) you didn't recognize the RMW problem you are having here.
    .....

    Any of those issues would have gotten you in trouble and you have all of them in one short piece.

    I think getting back to the basics is the fastest way you could move forward.
     
  11. hunterage2000

    Thread Starter Active Member

    May 2, 2010
    400
    0
    I've been through the basics. The header files excluding pic16f785.h and xc.h are initially included. The config is set for an internal osc. The 4 output and 1 input was tested and works. Char values are used temporarily until I look into why I get errors when using the bit data type.

    The logic I thought was:

    initially led_state is 1 and LED bit0 is 'ON'
    IF button isn't pressed, led_state = led_state
    IF button is pressed, led_state is shifted left and becomes 2 and LED bit1 is 'ON'
    WHEN button is released button isnt pressed, led_state = led_state = 2

    then I expected this to hold at 2 until button is pressed and shifted left again.

    The value 2 holds a while then returns to 1.


    I think the scope of inc_state is the problem as it eventually reverts back to a value of 1 but not sure why as this is returned from within the if block.
     
Loading...