Arduino C, Led > binary alternative all lights

Question

I'm trying to alternatively have a random int generate a binary number with 4 lights.

This is my current code, However, it seems that all the lights(leds) go on, I don't think the minus option works. As in, it won't take a value from the currently randomized digit. (randombinary = randombinary - x)

I also tried just "randombinary - x"

---

const int button = 2;
const int redone = 13;
const int redtwo = 8;
const int greenone = 12;
const int greentwo = 7;
int randomnumber = 0;
int randombinary = 0;


void setup() {

  pinMode(13, OUTPUT);
  pinMode(12, OUTPUT);
  pinMode(7, OUTPUT);
  pinMode(8, OUTPUT);
  pinMode(2, INPUT);

}
  void loop() {

delay(1000);

  int randomnumber = (1, 15);
  randombinary = randomnumber;

  if (randombinary >=8){
    digitalWrite(redone, HIGH);
   randombinary = randombinary - 8;
  }
 else {
   digitalWrite(redone, LOW);
 }


  if (randombinary >=4){
    digitalWrite(greenone, HIGH);
   randombinary = randombinary - 4;
  }
  else {
    digitalWrite(greenone, LOW);
  }

  if (randombinary >=2){
    digitalWrite(redtwo, HIGH);
  randombinary =  randombinary - 2; 
  }
  else  {
    digitalWrite(greentwo, LOW);
  }

  if (randombinary >=1){
    digitalWrite(greentwo, HIGH);
  randombinary =  randombinary - 1;
  }
  else  {
    digitalWrite(greentwo, LOW);
  }


  }

Would appreciate any help!

Answer 1

Code review (one possible solution can be found below)

• Absolute numbers in the code, like 15 as the limit for the random numbers are generally considered harmful. It would be better to give all absolute numbers a constantsname and use these throughout. The OP code did not use the constants for the pins, for instance.
• Writing similar pieces of code in a row makes maintenance hard. Therefore, group similar items in any kind of list, like an array, and iterate over them.
• Functions should have expressive names clearly stating what is going on inside.
• Functions should be short and concise, ideally having only a few lines.

Try something along these lines:

// setting the number of bits to analyze, corresponding pins and corresponding powers of two.
// the latter is here for instructional purposes although it is redundant. 
const int numberOfBits = 4;
const int randLimit = 15;   // 2^(1+numberOfBits)-1 actually;
int ledPins[numberOfBits] = {13, 8, 12, 7};
int bitValue[numberOfBits] = {8, 4, 2, 1}; // the powers of two

void turnOffAllLeds ()
{
  Serial.println ("turning all off");
  for (int i = 0; i < numberOfBits; i++)
  {
    digitalWrite (ledPins[i], LOW);
  }
}

void selectivelyTurnOnLeds(int value)
{
  turnOffAllLeds ();

  // now for each bit, check if it is set in the value, if yes: turn on corresponding led
  for (int i = 0; i < numberOfBits; i++)
  {
    if ((value & bitValue[i]) > 0)
    {
      digitalWrite (ledPins[i], HIGH);
    }
  }
}

void setup() 
{
  for (int i = 0; i < numberOfBits; i++)
  {
    pinMode (ledPins[i], OUTPUT);
  }
}

void loop() 
{
  selectivelyTurnOnLeds (random (randLimit));
  delay (1000);
}                                                                            

P.S. When posting a question, please also post what your code is supposed to do. If it were not for a lazy prechristmas workday, I would not have taken the time to reverse engineer your code.

Answer 2

int randomnumber = (1, 15);

Guess you have missed to call random. That statement actually always returns 15. C is a strange language.

int randomnumber = random(1, 15);

Cheers!

Answer 3

So here's the new version, very strange, when I set the number on random, it doesn't work. However, if I enter a number myself, it does.

const int button = 2;
const int redone = 13;
const int redtwo = 8;
const int greenone = 12;
const int greentwo = 7;
int randomnumber = 0;
int randombinary = 0;


void setup() {

  pinMode(13, OUTPUT);
  pinMode(12, OUTPUT);
  pinMode(7, OUTPUT);
  pinMode(8, OUTPUT);
  pinMode(2, INPUT);




      maarten();

    }
      void loop() {

      }

      void maarten(){



      int randomnumber = random(1, 15);
      randombinary = randomnumber;

    if (randombinary >=8){

      digitalWrite(redone, HIGH);
      randombinary = randombinary - 8;
    }
    else {
      digitalWrite(redone, LOW);
 }


    if (randombinary >=4){

      digitalWrite(greenone, HIGH);
      randombinary = randombinary - 4;
    }
    else {
      digitalWrite(greenone, LOW);
    }

    if (randombinary >=2){

      digitalWrite(redtwo, HIGH);
      randombinary =  randombinary - 2; 
    }
    else  {
      digitalWrite(greentwo, LOW);
    }

   if (randombinary >=1){

     digitalWrite(greentwo, HIGH);
     randombinary =  randombinary - 1;
   }
   else  {
     digitalWrite(greentwo, LOW);
  }

  }