reducing the code, need help!

juanmol · November 12, 2013, 7:45am

Hi, i need to reduce the next code as much as possible. I'm a bit noob and i think i can redice a lot the code, but i don't know how:

#include <Manchester.h>

int SDI = 1; //el amarillo
int CKI = 2; //Green wire
long strip_colors[1];

String yosoy = 100;
int todos = 254;
int led = 0;
//int colorrecibido = 0;
String color = 1;
long color1 = 0xFF0000;
long color2 = 0xFF0000;
int estado = 0;

void setup()
{
  man.setupReceive(4, MAN_1200);
  man.beginReceive();
  pinMode(SDI, OUTPUT);
  pinMode(CKI, OUTPUT);
  strip_colors[0] = 0;
}
void loop() {
  
  strip_colors[0] = color1;
  delay(100);
  
  for (int x = 0; x <= 30000; x++){
  if (man.receiveComplete()) {
    int m = man.getMessage();
    man.beginReceive();
    if (m > 99 || m == todos){
      led = m;
    }
    if (m > 9 && m < 100){
        //colorrecibido = m;
        if (estado == 1){
          estado = 0;
          switch (m){
            case 10:
              color1 = 0x000000;
              color2 = 0x000000;
            break;
            case 11:
              color1 = 0xFFFFFF;
              color2 = 0xFFFFFF;
            break;
            case 12:
              color1 = 0x0000FF;
              color2 = 0x0000FF;
            break;
            case 13:
              color1 = 0x00FF00;
              color2 = 0x00FF00;
            break;
            case 14:
              color1 = 0xFF0000;
              color2 = 0xFF0000;
            break;
            case 15:
              color1 = 0xFF00FF;
              color2 = 0xFF00FF;
            break;
            case 16:
              color1 = 0xFF0000;
              color2 = 0x000000;
            break;
            case 17:
              color1 = 0x0000FF;
              color2 = 0x00FF00;
            break;
            case 18:
              color1 = 0x0000FF;
              color2 = 0xFF0000;
            break;
            case 19:
              color1 = 0x0000FF;
              color2 = 0xFF00FF;
            break;
            case 20:
              color1 = 0x00FF00;
              color2 = 0xFF0000;
            break;
            case 21:
              color1 = 0x00FF00;
              color2 = 0xFF00FF;
            break;
            case 22:
              color1 = 0xFF0000;
              color2 = 0xFF00FF;
            break;
            case 23:
              color1 = 0xFFFF00;
              color2 = 0xFFFF00;
            break;
            case 24:
              color1 = 0xFFFF00;
              color2 = 0x000000;
            break;
            case 25:
              color1 = 0xFFFF00;
              color2 = 0x0000FF;
            break;
            case 26:
              color1 = 0xFFFF00;
              color2 = 0x00FF00;
            break;
            case 27:
              color1 = 0xFFFF00;
              color2 = 0xFF0000;
            break;
          }
          led = 0;
        }
    }
    if (estado == 0){
      if (yosoy == led || todos == led){
        estado = 1;
      }
    }
  }
  } //aqui acaba el for
  
  if (color == 1){
    strip_colors[0] = color1;
      for(byte color_bit = 23 ; color_bit != 24 ; color_bit--) {
      digitalWrite(CKI, LOW); //Only change data when clock is low
      long mask = 1L << color_bit;
      if(strip_colors[0] & mask) 
        digitalWrite(SDI, HIGH);
      else
        digitalWrite(SDI, LOW);
  
      digitalWrite(CKI, HIGH);
    }
    color = 2;
  }
  else{
    strip_colors[0] = color2;
      for(byte color_bit = 23 ; color_bit != 24 ; color_bit--) {
      digitalWrite(CKI, LOW);
      long mask = 1L << color_bit;
      if(strip_colors[0] & mask) 
        digitalWrite(SDI, HIGH);
      else
        digitalWrite(SDI, LOW);
  
      digitalWrite(CKI, HIGH);
    }
    color = 1;
  }
}

it's an attiny (expected to be the attiny25) receiving rf and controlling an only ws2801 led. I've got the code from examples and tutorials. Please help!

nickgammon · November 12, 2013, 7:57am

Lose the String class for a start.

It doesn't even compile, for me. Are you asking for help with the compiler errors?

sketch_nov12d:7: error: invalid conversion from 'int' to 'const char*'
sketch_nov12d:7: error: initializing argument 1 of 'String::String(const char*)'
sketch_nov12d:11: error: invalid conversion from 'int' to 'const char*'
sketch_nov12d:11: error: initializing argument 1 of 'String::String(const char*)'
sketch_nov12d.ino: In function 'void setup()':
sketch_nov12d:18: error: 'man' was not declared in this scope
sketch_nov12d:18: error: 'MAN_1200' was not declared in this scope
sketch_nov12d.ino: In function 'void loop()':
sketch_nov12d:30: error: 'man' was not declared in this scope
sketch_nov12d:118: error: invalid conversion from 'int' to 'const char*'
sketch_nov12d:118: error: initializing argument 1 of 'unsigned char String::operator==(const char*) const'
sketch_nov12d:125: error: invalid conversion from 'int' to 'const char*'
sketch_nov12d:125: error: initializing argument 1 of 'unsigned char String::operator==(const char*) const'
sketch_nov12d:137: error: invalid conversion from 'int' to 'const char*'
sketch_nov12d:137: error: initializing argument 1 of 'String& String::operator=(const char*)'
sketch_nov12d:151: error: invalid conversion from 'int' to 'const char*'
sketch_nov12d:151: error: initializing argument 1 of 'String& String::operator=(const char*)'

juanmol · November 12, 2013, 8:01am

compiles for me
in fact it's working on an ATTiny45, but i need to put it on an ATTiny25.

dc42 · November 12, 2013, 8:42am

Lose the String class as Nick says.
Variables whose values never change (e.g. SDI, CKI, todos) should be declared "const".
It may not reduce the size, however it would be better to declare strip_colours, mask, color1 and color2 "unsigned long" instead of "long".
It may be possible to reduce the size of the Manchester library, particularly if it includes functions that you are not using.
If the code is still too big, then don't use an Arduino core. Instead, write it directly in Atmel Studio C++, and use direct port manipulation instead of digitalWrite etc.

nilton61 · November 12, 2013, 10:32am

There are a number of places for improvement:

There i absolutely no point of creating an array with one element

long strip_colors[1];

should be

long strip_colors;

all lines containig

strip_colors[0]

sould then be chages to

strip_colors

The endless case statement can be replaced with a pre initialised arrays

const unsigned long c1[] = {0x000000, 0xFFFFFF, 0x0000FF, 0x00FF00, 0xFF0000, 
                            0xFF00FF, 0xFF0000, 0x0000FF, 0x0000FF, 0x0000FF,
                            0x00FF00, 0x00FF00, 0xFF0000, 0xFFFF00, 0xFFFF00, 
                            0xFFFF00, 0xFFFF00, 0xFFFF00};
const unsigned long c2[] = {0x000000, 0xFFFFFF, 0x0000FF, 0x00FF00, 0xFF0000,
                            0xFF00FF, 0x000000, 0x00FF00, 0xFF0000, 0xFF00FF,
                            0xFF0000, 0xFF00FF, 0xFF00FF, 0xFFFF00, 0x000000,
                            0x0000FF, 0x00FF00, 0xFF0000};

The case statement then becomes:

color1 = c1[m-10];
color2 = c2[m-10]

You have this loop in two places

      for(byte color_bit = 23 ; color_bit != 24 ; color_bit--) {
      digitalWrite(CKI, LOW); //Only change data when clock is low
      long mask = 1L << color_bit;
      if(strip_colors[0] & mask) 
        digitalWrite(SDI, HIGH);
      else
        digitalWrite(SDI, LOW);
  
      digitalWrite(CKI, HIGH);
      }

That means you can factor it into a function which in turn can be improved:

void send24bit_msbFirst(unsigned long value){
  value &=0xFFFFFF;//strip all exeeding bits;
  for(int i = 0; i<24; i++){
     digitalWrite(CKI, LOW); //Only change data when clock is low
     digitalWrite(SDI, (value&0x800000)==0x800000);//Send data
     digitalWrite(CKI, HIGH);
     value <<=1;//shift in next bit
  }//for(i)
}//send24bit_msbFirst()

juanmol · November 12, 2013, 11:58am

Thanks!! but i really don't understand some thing. The:

color1 = c1[m-10];
color2 = c2[m-10]

why m? it's my variable or an array's property?

the last thing you say, in my code:

  if (color == 1){
    strip_colors = color1;
      for(byte color_bit = 23 ; color_bit != 24 ; color_bit--) {
      digitalWrite(CKI, LOW); //Only change data when clock is low
      long mask = 1L << color_bit;
      if(strip_colors & mask) 
        digitalWrite(SDI, HIGH);
      else
        digitalWrite(SDI, LOW);
  
      digitalWrite(CKI, HIGH);
    }
    color = 2;
  }
  else{
    strip_colors = color2;
      for(byte color_bit = 23 ; color_bit != 24 ; color_bit--) {
      digitalWrite(CKI, LOW);
      long mask = 1L << color_bit;
      if(strip_colors & mask) 
        digitalWrite(SDI, HIGH);
      else
        digitalWrite(SDI, LOW);
  
      digitalWrite(CKI, HIGH);
    }
    color = 1;
  }

how i can implement it? I had test and the IDE tell me a lot of errors.

alnath · November 12, 2013, 12:20pm

juanmol:
compiles for me
in fact it's working on an ATTiny45, but i need to put it on an ATTiny25.

Sure ?
with that ? :

[b]String yosoy = 100;[/b]    //!!
int todos = 254;
int led = 0;
//int colorrecibido = 0;
[b]String color = 1;[/b]   //!!

you should get the compiler errors (actually, the messages Nick has just posted) !
as Nick and dc42 say, don't use String objects. But if you really want to use it, have a look here :
http://arduino.cc/en/Tutorial/StringConstructors

nilton61 · November 12, 2013, 12:38pm

why m? it's my variable or an array's property?

You wrote this in your code:

          switch (m){
            case 10:
              color1 = 0x000000;
              color2 = 0x000000;
            break;
            case 11:
              color1 = 0xFFFFFF;
              color2 = 0xFFFFFF;
            break;

Since the first case is m==10 this translates to index 0. Also in the if-statement preceeding it you chech for m<100. What about the other cases???

The fuction i wrote compiles fine. You use it by calling it this way in your code:

if (color == 1){
  send24bit_msbFirst(color1);
  color = 2;
}else{
  send24bit_msbFirst(color2);
  color = 1;
}//if else

juanmol · November 12, 2013, 2:20pm

THANKS!!!! near a half of the original size ... AND WORKS!!!!

tylernt · November 12, 2013, 4:55pm

dc42:
4. It may be possible to reduce the size of the Manchester library, particularly if it includes functions that you are not using.

Correct me if I'm wrong, but I believe the compiler (or perhaps more accurately, the linker) will automatically omit functions that are never called?

juanmol · November 12, 2013, 5:10pm

i've remove all functions that i don't need, the program size it's the same, i think you are not wrong.

tylernt:

dc42:
4. It may be possible to reduce the size of the Manchester library, particularly if it includes functions that you are not using.

Correct me if I'm wrong, but I believe the compiler (or perhaps more accurately, the linker) will automatically omit functions that are never called?

dc42 · November 12, 2013, 5:39pm

juanmol:

dc42:
4. It may be possible to reduce the size of the Manchester library, particularly if it includes functions that you are not using.

Correct me if I'm wrong, but I believe the compiler (or perhaps more accurately, the linker) will automatically omit functions that are never called?

A standard linker will only omit whole compilation units that contain no referenced declarations, and only if that compilation unit is a module in a linker library file. Therefore, if the Manchester library is built as a single compilation unit (i.e. it has just one .cpp source file), then all of it will be included if any of it is included. Except that functions declared 'inline' that are either class members or are declared 'static' may have no code generated for them if they are not used.

Topic		Replies	Views
Can I reduce this further?? Programming	1	434	May 5, 2021
Coding Question - Please Send Help Programming	15	1814	May 6, 2021
cleaning up some code General Guidance	4	548	May 5, 2021
Need help to shorten my code for my led strip pls help Programming	11	269	February 10, 2023
Condense code into.... Programming	5	1920	May 5, 2021

reducing the code, need help!

Related topics