6
\$\begingroup\$

I am trying to perform a loop and then stop the loop when the last line of code in the if statement is printed to screen saving on CPU cycles.

I want to start a while loop and then kill the loop after it is complete, but not breaking out of the loop so I can re-initiate the loop once again by pressing a GPIO button "B1".

My end goal is to have the RGB LED cycle and not continue to loop once it is complete.

Setup: RPi 3, RGB LED, button and breadboard.

#include <stdio.h>
#include <string.h>
#include <sys/types.h>
#include <bcm2835.h>

#define LED_RED RPI_GPIO_P1_12 //GPIO 18
#define LED_GRN RPI_GPIO_P1_16 //GPIO 23
#define LED_BLU RPI_GPIO_P1_18 //GPIO 24
#define B1 RPI_GPIO_P1_22 //GPIO 25


int main(int argc, char **argv)
{
    if (!bcm2835_init()) return 1;
    bcm2835_gpio_fsel(LED_RED, BCM2835_GPIO_FSEL_OUTP);
    bcm2835_gpio_fsel(LED_GRN, BCM2835_GPIO_FSEL_OUTP);
    bcm2835_gpio_fsel(LED_BLU, BCM2835_GPIO_FSEL_OUTP);
    bcm2835_gpio_fsel(B1, BCM2835_GPIO_FSEL_INPT);
    bcm2835_gpio_set_pud(B1, BCM2835_GPIO_PUD_UP);

    bcm2835_gpio_len(B1);

    unsigned int delay = 1000;
    while (1){
        printf("Light cycle machine\n");

        if(bcm2835_gpio_eds(B1)!=0){

            printf("Button Pressed!!!\n");
            printf("\n");
            bcm2835_gpio_set(LED_RED);
            printf("Red LED on!!!\n");
            bcm2835_delay(delay);
            bcm2835_gpio_clr(LED_RED);
            printf("Red LED off!!!\n");
            bcm2835_gpio_set(LED_GRN);
            printf("Green LED on!!!\n");
            bcm2835_delay(delay);
            bcm2835_gpio_clr(LED_GRN);
            printf("Green LED off!!!\n");
            bcm2835_gpio_set(LED_BLU);
            printf("Blue LED on!!!\n");
            bcm2835_delay(delay);
            bcm2835_gpio_clr(LED_BLU);
            printf("Blue LED off!!!\n");
            delay(delay);
            bcm2835_gpio_set_eds(B1);
            printf("\n");
            printf("System Idle\n");
            }

        else{


            delay(delay);
            //printf("Done!\n");

            }
        if (bcm2835_gpio_eds(B1)!=1)
            {       
            }
        else{

            }

}
    return 0;
}
\$\endgroup\$
6
  • 4
    \$\begingroup\$ Does this code already work as intended? \$\endgroup\$ – 200_success Jun 19 '16 at 16:43
  • \$\begingroup\$ Yes and no... yes, it runs through the loop and activates 1 sequence loop of lights. no, it still continues the loop through the else function taking up CPU cycles causing unnecessary use of the CPU. \$\endgroup\$ – rosskopp Jun 19 '16 at 18:13
  • \$\begingroup\$ for some reason my main comment was a bit truncated. sorry for the confusion... \$\endgroup\$ – rosskopp Jun 19 '16 at 18:37
  • 1
    \$\begingroup\$ You should probably remove any section in your question that seems to ask 'how to do' x since that is outside the scope of this site. \$\endgroup\$ – Legato Jun 19 '16 at 20:13
  • \$\begingroup\$ will do Legato. \$\endgroup\$ – rosskopp Jun 19 '16 at 21:13
5
\$\begingroup\$

Naming

Keep your #define constant names consistent. You specify that LED_RED, LED_GRN, and LED_BLU are LEDs, but you don't specify what B1 is. A better name would be BTN_B1.

Bracing

Keep your bracing consistent. Stick to one indentation style. For example in your code, you have both

if(...){ ... }

and also

if (...) { ... }

Delay Variable

Use the const keyword if you're not going to be changing delay.

Main Args

If you're not going to be using int argc, char **argv, then they don't need to be there.

Use Functions

The first 7 lines of main are setting up the GPIO pins, you should put that into a setup() function.

Make good use of whitespace

The long block of statements inside the if(bcm2835_gpio_eds(B1)!=0) would be better off separated by some spaces, to make it more readable.

Resulting Code

#include <stdio.h>
#include <string.h>
#include <sys/types.h>
#include <bcm2835.h>

#define LED_RED RPI_GPIO_P1_12 //GPIO 18
#define LED_GRN RPI_GPIO_P1_16 //GPIO 23
#define LED_BLU RPI_GPIO_P1_18 //GPIO 24
#define BTN_B1 RPI_GPIO_P1_22 //GPIO 25

void setup_gpio(void);

int main()
{
    if (!bcm2835_init()) return 1;
    setup_gpio();


    unsigned int delay = 1000;
    while (1) {
        printf("Light cycle machine\n");

        if (bcm2835_gpio_eds(BTN_B1)!=0) {
            printf("Button Pressed!!!\n\n");

            bcm2835_gpio_set(LED_RED);
            printf("Red LED on!!!\n");
            bcm2835_delay(delay);
            bcm2835_gpio_clr(LED_RED);
            printf("Red LED off!!!\n");

            bcm2835_gpio_set(LED_GRN);
            printf("Green LED on!!!\n");
            bcm2835_delay(delay);
            bcm2835_gpio_clr(LED_GRN);
            printf("Green LED off!!!\n");

            bcm2835_gpio_set(LED_BLU);
            printf("Blue LED on!!!\n");
            bcm2835_delay(delay);
            bcm2835_gpio_clr(LED_BLU);
            printf("Blue LED off!!!\n");

            delay(delay);
            bcm2835_gpio_set_eds(BTN_B1);
            printf("\nSystem Idle\n");
            }
        else {
            delay(delay);
            //printf("Done!\n");
            }
        }

    return 0;
}

void setup_gpio()
{
    bcm2835_gpio_fsel(LED_RED, BCM2835_GPIO_FSEL_OUTP);
    bcm2835_gpio_fsel(LED_GRN, BCM2835_GPIO_FSEL_OUTP);
    bcm2835_gpio_fsel(LED_BLU, BCM2835_GPIO_FSEL_OUTP);
    bcm2835_gpio_fsel(BTN_B1, BCM2835_GPIO_FSEL_INPT);

    bcm2835_gpio_set_pud(BTN_B1, BCM2835_GPIO_PUD_UP);
    bcm2835_gpio_len(BTN_B1);
}

As for your question, you say you want to kill the loop but not break out of it. I'm not exactly sure what you mean. But, perhaps you're looking for interrupts?

\$\endgroup\$
3
  • \$\begingroup\$ AWESOME! thanks for the excellent pointers. i tried an interrupt but still had problems with the loop still running in the background. im still learning and what you have given me for tools will absolutely help me in my endeavor into C. \$\endgroup\$ – rosskopp Jun 19 '16 at 21:11
  • \$\begingroup\$ "Use the const keyword if you're not going to be changing delay" --> Why? - not see that for speed as an optimizing compiler will detect delay is not changed and optimize for it const-ness. \$\endgroup\$ – chux - Reinstate Monica Jun 21 '16 at 2:16
  • \$\begingroup\$ Declaring a function without a signature like void setup_gpio(); does not check for parameter matching. "The empty list in a function declarator that is not part of a definition of that function specifies that no information about the number or types of the parameters is supplied" C11 §6.7.6.3 14 So main() can call setup_gpio(1,2,3) without an error. Better to use void setup_gpio(void); \$\endgroup\$ – chux - Reinstate Monica Jun 21 '16 at 2:21
3
\$\begingroup\$

Remove empty conditional blocks

Anytime you have a series of empty conditional blocks like this:

  if (bcm2835_gpio_eds(B1)!=1)
            {       
            }
        else{

            }

You're better off removing it. It clutters the code, impedes readability and serves no purpose. Even if it's meant as some "TODO" it would be better if it were a comment instead of adding lines of code to scroll through and that exist in the compiler's evaluation.

Be consistent

On the topic of readability, some places in your code your brackets are on the same line as the statement that precedes. While in others, the brackets are on the following line. This is the case even in the aforementioned code I quoted for a different reason.

Though there are C conventions whichever style you choose is up to you and what you find comfortable, but whatever that is: be consistent about it. Otherwise, It's distracting and adds additional noise that isn't conducive to digestion and understanding of the code which surrounds it. This also applies to your use of spacing and tabs.

Be consistent so the focus is on what tools you're using, and what you want to do rather than the several different ways you happen to hold your tools.

\$\endgroup\$
0
1
\$\begingroup\$

I see some things that may help you improve your program.

Consider separating I/O from the algorithm

Right now, everything is done in main. Better practice is to separate things into functions. In particular, I'd recommend separating the input/output routines to make them more consistent and easier to use. For example:

void setLED(int led, bool state) {
    static const char *ledname[3] = { "RED", "GREEN", "BLUE" };
    static const int ledpin[3] = { LED_RED, LED_GRN, LED_BLU };
    if (led < 0 || led > 2) {
        return;
    }
    if (state) {
        bcm2835_gpio_set(ledpin[led]);
    } else {
        bcm2835_gpio_clr(ledpin[led]);
    }
    printf("%s LED %s!!!\n", ledname[led], state ? "ON" : "OFF");
}

Note that to use bool you will need to #include <stdbool.h>.

Write human-readable code

The code includes lines like this:

if (bcm2835_gpio_eds(B1) != 0) {

That's technically fine, but a human reader might puzzle over what it actually means. Consider instead if it were written like this:

if (button_pressed(B1)) {

You could easily understand what was meant without knowing anything about how the device is physically wired. It's easy to write a small wrapper function that does this.

Eliminate useless code

The compound if statement that begins if (bcm2835_gpio_eds(B1)!=1) does nothing whether the expression is true or false, so it could be eliminated.

Use consistent function names

Some points in the code use bcm2835_delay() while others use delay(). On the Pi, these are aliases for the same function. I'd recommend choosing one and using it consistently.

Use consistent formatting

The code as posted has inconsistent indentation which makes it hard to read and understand. Pick a style and apply it consistently.

Avoid using the same name for different things

Lines like this:

delay(delay);

while technically unambiguous, are a bit peculiar looking to human readers. I'd recommend instead naming the delay time something more descriptive such as one_second, so the line would then be very descriptive: delay(one_second);.

Use const where practical

Numbers like delay (or one_second if you follow the suggestion above) are never intended to be altered, so they should be declared as const as in const unsigned one_second = 1000;.

Prefer portable functions

The bcm2835_delay() routine only works on the Raspberry Pi and doesn't even necessarily work on the Pi2 (unless device-tree support is enabled). Instead, you could use sleep() or nanosleep() which are both POSIX standard functions and therefore more portable. Portable code is generally better because a useful program tends to live far beyond its original application, and we all try to write useful code.

Eliminate unused variables

The variables argc and argv in your code are but never used. Since unused variables are a sign of poor code quality, you should seek to eliminate them. Your compiler is probably smart enough to warn you about such things if you know how to ask it to do so.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.

\$\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.