0
\$\begingroup\$

I think this question is ok in the electronics stack and not the programming stack (I hope).

I have a simple code written in C, on a TM4C123 evaluation board. It uses the two on board switches to control the colour of the on board RGB LED.

The switches are configured in software with pull up resistors and the PORTF configurations are all correct. I had the same functionality working using switch case statements, but tried to re-create it with if else statements but it doesn't work as it is supposed to.

Below is the original working code:

#include "include/tm4c123gh6pm.h"
#define RED_LED (1U << 1)
#define BLUE_LED (1U << 2)
#define GREEN_LED (1U << 3)

int main()
{
    SYSCTL_RCGCGPIO_R = 0x20U;
    GPIO_PORTF_LOCK_R = 0x4C4F434BU;
    GPIO_PORTF_CR_R = 0xFFU;
    GPIO_PORTF_DIR_R = 0x0EU;
    GPIO_PORTF_PUR_R = 0x11U;
    GPIO_PORTF_DEN_R = 0x1FU;


        while(1)
        {
        GPIO_PORTF_DATA_R &= (0x011U);

            switch(GPIO_PORTF_DATA_R & 0x11U)
            {
                case 0x11:
                GPIO_PORTF_DATA_R |= RED_LED;
                break;

                case 0x01:
                GPIO_PORTF_DATA_R |= BLUE_LED;
                break;

                case 0x10:
                GPIO_PORTF_DATA_R |= GREEN_LED; 
                break;

                default:

                break;
            }

        }

}

Here is the troublesome code:

#include "include/tm4c123gh6pm.h"
#define RED_LED (1U << 1)
#define BLUE_LED (1U << 2)
#define GREEN_LED (1U << 3)

    int main()
{
    SYSCTL_RCGCGPIO_R = 0x20U;
    GPIO_PORTF_LOCK_R = 0x4C4F434BU;
    GPIO_PORTF_CR_R = 0xFFU;
    GPIO_PORTF_DIR_R = 0x0EU;
    GPIO_PORTF_PUR_R = 0x11U;
    GPIO_PORTF_DEN_R = 0x1FU;

        while(1)
        {
        GPIO_PORTF_DATA_R &= (0x011U);

            if(GPIO_PORTF_DATA_R & 0x01U)
                {
                    GPIO_PORTF_DATA_R |= RED_LED;
                }   

            if(GPIO_PORTF_DATA_R & 0x10U)
                {   
                    GPIO_PORTF_DATA_R |= BLUE_LED;
                }

            if(GPIO_PORTF_DATA_R & 0x011U)
                {
                    GPIO_PORTF_DATA_R |= GREEN_LED; 
                }

            else
                {
                    GPIO_PORTF_DATA_R &= 0x0U; 
                }
            }
}

The switches are stored in PORTF bits 0 and 4. And the red, blue and green LED is stored in port F bits 1, 2 and 3 respectively.

On line 63 I have an 'AND' assignment to check if bits 0 or 4 is high (button(s) been pressed).

Next the data register is logical 'ANDED' with 1's on bits 0 and 4 to check if it is a 1.

Then the IF statements should switch on the LED's accordingly.

Operation should be:

No switches pressed (bits 4 and 0 = 00) , no LED on

Switch 1 pressed (bits 4 and 0 = 01), green LED on

Switch 2 pressed (bits 4 and 0 = 10), blue LED on

Both switches pressed (bits 4 and 0 = 11), both LED's on

When I step through the code with the de-bugger, the IF statements all appear true with the red, blue and green LED all coming on in order. The else statement is appearing false.

Am I making a mistake that with the pull up resistors, the bits 4 and 0 will be active low (1 when not pressed, and a 0 when pressed)?

Thanks for any replies.

\$\endgroup\$
  • 3
    \$\begingroup\$ If the input data is in fact 0x011 then all 3 if statements will evaluate true. After the first AND (which flushes out bits of no importance here) you should be evaluating for equality; if GPIO_PORTF_DATA_R == <01><10><11> where <xxx> are the actual tests. I would also use else if after the first if statement for clarity at least. \$\endgroup\$ – Peter Smith Sep 7 at 15:24
  • 2
    \$\begingroup\$ Aside from your conditionals, which others are exploring, please explain a bit more clearly what you intend the switches to do. Your lack of corresponding &= statements to turn LEDs 'off' makes me think you want the LEDs to be 'sticky', and your verbal description is ambiguous, but implies by omission no such dependency on switch history. \$\endgroup\$ – Spehro Pefhany Sep 7 at 15:25
  • \$\begingroup\$ Sorry, they are two normally momentary (non-latching) push buttons. The according LED(s) should only be ON when the button(s) are pressed. \$\endgroup\$ – David777 Sep 7 at 15:27
  • \$\begingroup\$ @PeterSmith Ah I see what you mean. I changed the condition checking to a Boolean equal to (==) and the code now works as expected. \$\endgroup\$ – David777 Sep 7 at 15:33
  • 1
    \$\begingroup\$ You still have inconsistent indentation. Answers go in the answer form, not the question. \$\endgroup\$ – Chris Stratton Sep 7 at 21:34
1
\$\begingroup\$

As you noted, you have a logic flaw where several of your if statements might be true simultaneously. This should be solved with the equivalent of switch, namely if() else if() .... This is the least of the problems though.

The root of your problems seems to be the lack of "debouncing", which is mandatory for all forms of buttons and switches. This is absolutely necessary unless you have external RC filters, which is most often not the case, since HW designers count on software to deal with it. I won't describe "debouncing" here since it's typically addressed early on in any beginner embedded course.

Your switch statement reads the GPIO register once, so it seems to work, by luck. But the if-else reads the same register in multiple places, over time.

Another problem is that in general, you should not treat hardware registers like ordinary variables. They are volatile qualified, meaning that each access to them is unique. The compiler is not allowed to optimize the access, so several reads over time might produce different results.

Therefore best practice is to isolate access read/write to registers on lines of their own. Don't mix such access with complex expressions.

What you should do no matter if-else or switch, is to read the register once, store the result in a temporary local variable and then work with that variable when determining which button that was pressed.

\$\endgroup\$
  • \$\begingroup\$ Yeah, I am familiar with de-bouncing and the issues caused by it. But in this code I have purposely ignored it. What you said about reading the GPIO register is interesting, I never thought about it being volatile meaning some bits could change state between the if statements being scanned. I suppose in my case this isn't an issue as the mechanical push buttons are very slow. I think I will use a temp variable from now on. \$\endgroup\$ – David777 Sep 9 at 15:32
  • \$\begingroup\$ Also what do you mean when you say above, "Therefore best practice is to isolate access read/write to registers on lines of their own. Don't mix such access with complex expressions"? \$\endgroup\$ – David777 Sep 9 at 15:33
  • 1
    \$\begingroup\$ @David777 It means that you should write code like for example uint8_t portx = PORTX; portx &= ~(portx & mask); PORTX = portx; rather than PORTX &= ~(PORTX & mask); \$\endgroup\$ – Lundin Sep 10 at 6:27
1
\$\begingroup\$

This is one of the possible solutions to my question, which is based on Peter Smith's comment. I replaced the AND assignments in the IF statement condition with a Boolean equal to operator.

Below is the corrected code which is now working and functions how I described it should in the question:

#include "include/tm4c123gh6pm.h"
#define RED_LED (1U << 1)
#define BLUE_LED (1U << 2)
#define GREEN_LED (1U << 3)

    int main()
{
    SYSCTL_RCGCGPIO_R = 0x20U;
    GPIO_PORTF_LOCK_R = 0x4C4F434BU;
    GPIO_PORTF_CR_R = 0xFFU;
    GPIO_PORTF_DIR_R = 0x0EU;
    GPIO_PORTF_PUR_R = 0x11U;
    GPIO_PORTF_DEN_R = 0x1FU;

        while(1)
        {
        GPIO_PORTF_DATA_R &= (0x011U);

            if(GPIO_PORTF_DATA_R == 0x01U)
                {
                    GPIO_PORTF_DATA_R |= RED_LED;
                }   

            if(GPIO_PORTF_DATA_R == 0x10U)
                {   
                    GPIO_PORTF_DATA_R |= BLUE_LED;
                }

            if(GPIO_PORTF_DATA_R == 0x0U)
                {
                    GPIO_PORTF_DATA_R |= GREEN_LED; 
                }

            else
                {
                    GPIO_PORTF_DATA_R &= 0x0U; 
                }
        }
}
\$\endgroup\$

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.