Help optimising somem code

I am working on a little project with some APA102 LEDs.
Its been a while since I did much work with C so have been taking my time so far, but things are starting to come together.
As it stand I am using an uno to do development and get the project somewhere close to finality, but it will being going onto an atTiny85 due to space constraints.
I can’t help but feel the main loop so far is a little cumbersome. But I can’t think of a way of making it less so.
As things move forward it would like to be able to add different modes, you know the usual, static colour that the user chooses, and a few different animations, and be able to store user settings internally, it would be a pain to have to set thing every time its power cycled.

I understand that it may not be possible to condense the code, but it some times helps to get a fresh perspective on things.

//https://learn.adafruit.com/adafruit-dotstar-leds/arduino-library-use
#include <Adafruit_DotStar.h>

#define NUMPIXELS 52 // Number of LEDs in strip

//use these pins for uno development
// Here's how to control the LEDs from any two pins:
#define DATAPIN    8
#define CLOCKPIN   7

//encoder pins
#define CLK 3
#define DT 4
#define SW 2

//use these pins for atTiny85
// Here's how to control the LEDs from any two pins:
/*
#define DATAPIN    2
#define CLOCKPIN   1

//encoder pins
#define CLK 4
#define DT 3
#define SW 5
*/

int counter = 0;
int currentStateCLK;
int lastStateCLK;
int btnState;
int bright = 128;
int btnflag;
int red_led = 25;
int green_led = 25;
int blue_led = 25;
unsigned long lastButtonPress = 0;

Adafruit_DotStar strip(NUMPIXELS, DATAPIN, CLOCKPIN, DOTSTAR_BGR);
void setup() {

#if defined(__AVR_ATtiny85__) && (F_CPU == 16000000L)
  clock_prescale_set(clock_div_1); // Enable 16 MHz on Trinket
#endif
  Serial.begin(9600);
  pinMode(CLK,INPUT);
  pinMode(DT,INPUT);
  pinMode(SW, INPUT);
  attachInterrupt(digitalPinToInterrupt(CLK),rotary,FALLING);
  strip.begin(); // Initialize pins for output
  strip.show();  // Turn all LEDs off ASAP
  lastStateCLK = digitalRead(CLK);
  pixels();   //disp[lay initial LEDs 
}

void loop() {
  
  //wait for button press
  btnpress();
  //if button pressed go and set brightness
  if (btnflag ==1){
    btnflag = 0;

    //find some way of communiacting change of state to user.

    //section below seems a little cumbersome, but it works
    //now set brightness
    //set counter to brightness
    counter = bright;
    while(btnflag == 0){
      btnpress();
      bright = counter;
      pixels();     
    }
    btnflag = 0;
    //brightness set
    
    //now do red leds
    counter = red_led;
    while (btnflag ==0){
      btnpress();
      red_led = counter;
      pixels();
    }
    btnflag = 0;

    //now do green leds
    counter = green_led;
    while (btnflag ==0){
      btnpress();
      green_led = counter;
      pixels();
    }
    btnflag = 0;

    //now do blue leds
    counter = blue_led;
    while (btnflag ==0){
      btnpress();
      blue_led = counter;
      pixels();
    }
    btnflag = 0;
  }
  //store on to internal EEPROM
}

//***********************************************
//*************functions here********************
//***********************************************
//###This is an ISR###
//KY-040 encoder
void rotary(){  
    currentStateCLK = digitalRead(CLK);
      if (digitalRead(DT) != digitalRead(CLK)){
        counter ++;
        if(counter > 255){
          counter = 255;
        }
      } else {
        counter--;
        if(counter < 1){
          counter = 1;
        }
      }
    lastStateCLK = currentStateCLK;
}
//###End ISR###


void btnpress(){
  //button press
    btnState = digitalRead(SW);

  //If we detect LOW signal, button is pressed
  if (btnState == LOW) {
    //if 50ms have passed since last LOW pulse, it means that the
    //button has been pressed, released and pressed again
    if (millis() - lastButtonPress > 50) {
      //Serial.println("Button pressed!");
      btnflag = 1;
    }

    // Remember last button press event
    lastButtonPress = millis();
    while(digitalRead(SW) == LOW){
      //hold here until button let go
    }
  }
  pixels();
  delay(100);
  }

void pixels(){   
    uint32_t colour = strip.Color(red_led, green_led, blue_led);
    strip.fill(colour, 0, NUMPIXELS);
    strip.setBrightness(bright);
    strip.show();

 }

well since the=is section also occurs for every color it makes sense to change it into a loop

    while(btnflag == 0){
      btnpress();
      bright = counter;
      pixels();     
    }
    btnflag = 0;

the easiest way would be to let the brightness and color values be part of an array.

it’s not clear to me what your program is doing.

pixels() is called in btnpress() but it’s also called immediately after the multiple calls to btnpress() in loop(). shouldn’t it only need to be called once, per loop iteration

it seems pressing a button both sequencing to the next color(? including bright) as well as entering the sequence in loop. It might be less redundant if loop() checks for a button press and increments a state variable tested in a switch statement, regardless of a button press, to adjust bright, red_led, green_led OR blue_led. pixel() can be called each loop() iteration regardless of whether the button is pressed. delay (100) at the bottom of loop() will control the speed of the program as well as debounce the button presses.

btnpress() could return a boolean (true) when pressed. check for a change, butLst != butState and return true if in the on state.

The pixel() call in the btnpress, is an error that a noticed a few hours after posting. It is now only in the adjustment loops, but I am tempted to use an if statement to check to see if there has beena change to the setting before calling it meaning that it would not be called unless it was needed.
There is a in btnpress() a variable called btnflag which is set to 1 when the button it pressed, it is then reset to 0 when leaving the loop. The button press may become an interrupt as well, meaning that the routine would not have to be called during the loop.

I am looking into using an array right now, should clean the looping segment up nicely.

Hopefully I should have updated the code in a few hours, I will post that to see if it makes any more sense than it currently does.

Thanks for the suggestions so far, they have been very helpful.

Came across something interesting, that I can't find any real answer to

myArray[i] = 30;

works fine

mymArray[i] = variable;

does not update the information in the variable related to the index

'myArray' is not the same as 'mymArray' typo?

Outside the context of an entire sketch, your statements are meaningless. You most certainly can assign an array element a value from another variable.

yes that was a typo

here is the test sketch

//https://learn.adafruit.com/adafruit-dotstar-leds/arduino-library-use
#include <Adafruit_DotStar.h>

#define NUMPIXELS 24 // Number of LEDs in strip

//use these pins for uno development
// Here's how to control the LEDs from any two pins:

#define DATAPIN    8
#define CLOCKPIN   7

//encoder pins
#define CLK 3
#define DT 4
#define SW 2

//use these pins for atTiny85
// Here's how to control the LEDs from any two pins:
/*
#define DATAPIN    2
#define CLOCKPIN   1

//encoder pins
#define CLK 4
#define DT 3
#define SW 5
*/

int counter = 0;
int currentStateCLK;
int lastStateCLK;
int btnState;
int btnflag;
int i;
unsigned long lastButtonPress = 0;
int bright = 128;
int red_led = 25;
int green_led = 25;
int blue_led = 25;
int settingArray[4]={bright,red_led,green_led,blue_led};

Adafruit_DotStar strip(NUMPIXELS, DATAPIN, CLOCKPIN, DOTSTAR_BGR);

void setup() {

#if defined(__AVR_ATtiny85__) && (F_CPU == 16000000L)
  clock_prescale_set(clock_div_1); // Enable 16 MHz on Trinket
#endif
  Serial.begin(9600);
  pinMode(CLK,INPUT);
  pinMode(DT,INPUT);
  pinMode(SW, INPUT);
  attachInterrupt(digitalPinToInterrupt(CLK),rotary,FALLING);
  strip.begin(); // Initialize pins for output
  strip.show();  // Turn all LEDs off ASAP
  lastStateCLK = digitalRead(CLK);
  pixels();   //display initial LEDs
   
}

void loop() {
  //Serial.println("start");
  //wait for button press
  btnpress();
  //if button pressed go and set brightness
  if (btnflag == 1){
    btnflag = 0;

    //find some way of communiacting change of state to user.
    
    for (i=0;i<4;i++){
      counter = settingArray[i];
      while(btnflag == 0){
        btnpress();
        if(counter!=settingArray[i]){
          //now update appropiate varaible - ie fudge it
          if(i == 0){
            bright = settingArray[i];
          }
          else if (i == 1){
            red_led = settingArray[i];
          }
          else if(i == 2){
            green_led = settingArray[i];
          }
          else{
            blue_led = settingArray[i];
          }
          //this should update array but doesn't
          //still need for if statmemnt check
          settingArray[i] = counter;
          pixels();
          
        }
        
      }
      btnflag = 0;
    }
    
  }
  //store on to internal EEPROM
}

//***********************************************
//*************functions here********************
//***********************************************
//###This is an ISR###
//KY-040 encoder
void rotary(){
      if (digitalRead(DT) != digitalRead(CLK)){
        counter ++;
        if(counter > 255){
          counter = 255;
        }
      } else {
        counter--;
        if(counter < 1){
          counter = 1;
        }
      }
    lastStateCLK = currentStateCLK;
    //Serial.println(counter);
}
//###End ISR###


void btnpress(){
  //button press
    btnState = digitalRead(SW);

  //If we detect LOW signal, button is pressed
  if (btnState == LOW) {
    //if 50ms have passed since last LOW pulse, it means that the
    //button has been pressed, released and pressed again
    if (millis() - lastButtonPress > 50) {
      Serial.println("Button pressed!");
      btnflag = 1;
    }

    // Remember last button press event
    lastButtonPress = millis();
    while(digitalRead(SW) == LOW){
      //hold here until button let go
    }
  }
  
  delay(100);
  }

void pixels(){   
    uint32_t colour = strip.Color(red_led, green_led, blue_led);
    strip.fill(colour, 0, NUMPIXELS);
    strip.setBrightness(bright);
    strip.show();

 }

I tried a few different ways of doing things that my limited knowledge let me understand.

I have fudged it for a moment so that I could check things I hadn’t broken something else.

this should be declared 'volatile' int counter = 0;The variable is changed within an ISR and used outside of it, you should make sure that when it is read, the value is always taken from the actual address not from a register.
I would also really get rid of the different names for the variables and just use the array

int bright = 128;
int red_led = 25;
int green_led = 25;
int blue_led = 25;
int settingArray[4]={bright,red_led,green_led,blue_led};

also they can be uint8_t in size,
then you can remove all of this

//now update appropiate varaible - ie fudge it
          if(i == 0){
            bright = settingArray[i];
          }
          else if (i == 1){
            red_led = settingArray[i];
          }
          else if(i == 2){
            green_led = settingArray[i];
          }
          else{
            blue_led = settingArray[i];
          }

and just do

uint32_t colour = strip.Color(settingArray[1], settingArray[2], settingArray[i3);
strip.setBrightness(settingArray[0]);

instead of having counter, you could have pointer to either red, green or blue_led and encoder inc/decrement it directly.

the button presses determine what the pointer points to

consider

// -----------------------------------------------------------------------------
void loop () {
    if (btnpress ())  {
        if (NULL == pColor)
            pColor = & red_led;
        else if (&red_led == pColor)
            pColor = & green_led;
        else if (&green_led == pColor)
            pColor = & blue_led;
        else
            pColor = NULL;
    }

    // in place of use of Rotary
    if (NULL != pColor)
        *pColor = 255 > *pColor ? *pColor + 1 : 0;

    pixels ();
    delay (500);
}

// -----------------------------------------------------------------------------
// ISR,  KY-040 encoder
void rotary (){
    int currentStateCLK = digitalRead (CLK);

    if (lastStateCLK == currentStateCLK)
        return;

    lastStateCLK = currentStateCLK;

    if (digitalRead (DT) != digitalRead (CLK)) {
        *pColor = 255 > *pColor ? *pColor + 1 : 0;
    }
    else {
        *pColor = 0 < *pColor ? *pColor - 1 : 255;
    }
}

// -----------------------------------------------------------------------------
int
btnpress ()
{
    static byte btnLst   = HIGH;
           byte btnState = digitalRead (A1);

    if (btnLst != btnState) {
        btnLst = btnState;
        return ! btnState;
    }

    return 0;
}

// -----------------------------------------------------------------------------
void pixels (){
    uint32_t colour = strip.Color (red_led, green_led, blue_led);
    strip.fill (colour, 0, NUMPIXELS);
    strip.setBrightness (bright);
    strip.show ();
}

OK, i have implemented some changes, with thanks to Deva_Rishi.
gcjr, I am going to have a dive into that now. I haven't tried it yet but would the work:

int *p;

p = &settingarray[i];

//in encoder isr
*p++;
//or
*p--

I really do appreciate all the help, it been a good learning experience for me.

I will have a closer look at the code you just posted while I was writing this post