Stm32 optimization exercise

Question

I am writing a program to turn on leds on a stm 32 board (model KAmeleon-STM32L496ZGT6).

I have LEDs 7 6 5 4 3 2 1 0

When I run the program it goes from 0 to 7 than bounces back and goes 7 to 0 and so on.

When I click joystick(GPIOE, GPIO_PIN_15), it changes direction.

My code below works, but I have a feeling that it could be clearer and/or more optimized. Is it possible to rewrite main (I would rather not rewrite the setSingleLED function).

void setSingleLED(int which)
{
    if(which > 7 || which < 0)
    {
        return;
    }
    GPIO_TypeDef * ports[8] = {GPIOC, GPIOC, GPIOC, GPIOC, GPIOE, GPIOD, GPIOE, GPIOE};
    uint16_t pins[8] = {GPIO_PIN_6, GPIO_PIN_7, GPIO_PIN_8, GPIO_PIN_9, GPIO_PIN_4, GPIO_PIN_3, GPIO_PIN_5, GPIO_PIN_6};
    for (int i = 0; i < 8; i++)
    {
        HAL_GPIO_WritePin(ports[i], pins[i], GPIO_PIN_RESET);
    }
    HAL_GPIO_WritePin(ports[which], pins[which], GPIO_PIN_SET);
}
int main(void)
{
    int zmienna=0;
    HAL_Init();
    SystemClock_Config();
    MX_GPIO_Init();
    HAL_GPIO_WritePin(GPIOC, GPIO_PIN_7, RESET);
    int current_diode = 0;
    int dir = 1;
    while (1)
    {
        if (current_diode < 0)
        {
            zmienna=0;
            current_diode = 1;
            dir=1;
        }
        else if (current_diode > 8)
        {
            zmienna=1;
            current_diode=7;
            dir=1;
        }
        setSingleLED(current_diode);
        //HAL_Delay(100);
        unsigned int i = 0;
        int ischange=0;
        while (i < 5000)
        {
            if((HAL_GPIO_ReadPin(GPIOE, GPIO_PIN_15) == 0)&&(ischange==0))
            {
                ischange=1;
                if(dir==1)
                {
                    dir= -1;
                }
                else
                {
                    dir= 1;
                }
            }
            i++;
        }
        if(zmienna>0)
        {
            current_diode -= dir;
        }
        else
        {
            current_diode += dir;
        }
    }
}

Answer 1

Here are some ideas how to improve the code:

• Move the ports and pins array outside the function as to ensure they are only initialized once
• i == which to select the LED state. That way there is no need to check if which is within range.
• Merge zmienna and dir into a single variable dir as it doesn't any difference if the direction needs to be changed due to user interaction or due to reaching the upper or lower bound.
• Remove the loop to 5000, which seems to be a way to debounce the button. This shouldn't be needed as the loop contains a HAL_Delay() to control the speed of the LED movement. If it's not sufficient, add more HAL_Delay() within the if clause for detecting the button press.
• Use dir = -dir to change the direction (instead of an if clause)
• Rearrange code such that update of current_diode is closer together

static GPIO_TypeDef * ports[8] = {GPIOC, GPIOC, GPIOC, GPIOC, GPIOE, GPIOD, GPIOE, GPIOE};
static uint16_t pins[8] = {GPIO_PIN_6, GPIO_PIN_7, GPIO_PIN_8, GPIO_PIN_9, GPIO_PIN_4, GPIO_PIN_3, GPIO_PIN_5, GPIO_PIN_6};


void setSingleLED(int which)
{
    for (int i = 0; i < 8; i++)
    {
        HAL_GPIO_WritePin(ports[i], pins[i], i == which ? GPIO_PIN_SET : GPIO_PIN_RESET);
    }
}

int main(void)
{
    HAL_Init();
    SystemClock_Config();
    MX_GPIO_Init();
    HAL_GPIO_WritePin(GPIOC, GPIO_PIN_7, RESET);
    int current_diode = 0;
    int dir = 1;

    while (1)
    {
        // light up selected LED
        setSingleLED(current_diode);

        // check if button has been pressed
        if (HAL_GPIO_ReadPin(GPIOE, GPIO_PIN_15) == 0)
        {
            dir = -dir;  // change direction
        }

        // update selected LED position
        current_diode += dir;
        if (current_diode < 0)
        {
            current_diode = 0;
            dir = 1;
        }
        else if (current_diode > 7)
        {
            current_diode = 7;
            dir = -1;
        }

        // delay to control speed of effect
        HAL_Delay(100);
    }
}