0

I am trying to create a library called "LED" for LED Sequencer in Arduino IDE,where the user has to configure the LED pins using an array and pass it to LED constructor in program and then will be able to use BlinkAll(),LeftToRight() and RightToLeft() Functions which lights the LED from Left to Right or Right to Left based on the sequence mentioned in the array. The problem is when I initiate the array and pass it to the constructor Pins 4,5,7 are not working at all , Please help me find what is the possible issue the code in LED.cpp file is shown below

#include <Arduino.h>
#include "LED.h"

LED::LED(int channel, int* a){
  _channel = channel;
  for(int i = 0 ; i < channel; i ++)
  {
    _pins[i] = a[i] ;
    pinMode(_pins[i],OUTPUT);
  }
}
void LED :: BlinkAll(int delay_ms)
{
   for (int i = 0 ; i<_channel;i++)
   {
    digitalWrite(_pins[i],HIGH);
   }
   delay(delay_ms);
   for (int i = 0 ; i<_channel;i++)
   {
    digitalWrite(_pins[i],LOW);
   }
   delay(delay_ms);
}
void LED :: LeftToRight(int numberofLEDs, int delay_ms)
{
  for(int i =0 ;i<_channel; i++)
  {
    digitalWrite(_pins[i], LOW);
  }
  for(int i =0 ;i<_channel; i++)
  {
//    if (i > 0)
//    {
//     digitalWrite(_pins[i-1], LOW);
//    }
  for(int i =0 ;i<_channel; i++)
  {
    digitalWrite(_pins[i], LOW);
  }
    digitalWrite(_pins[i], HIGH);
    delay(delay_ms);
  }
}
void LED :: RightToLeft(int numberofLEDs,int delay_ms)
{
    for(int i =_channel ;i>0; i--)
  {
    digitalWrite(_pins[i], LOW);
  }
  for(int i = _channel ;i>0; i--)
  {
    if (i < _channel)
    {
     digitalWrite(_pins[i+1], LOW);
    }
    digitalWrite(_pins[i], HIGH);
    delay(delay_ms);
  }
}

LCD.h

#ifndef LED_h
#define LED_h

#include <Arduino.h>

class LED 
{
public:
  LED(int channel, int* pins);
  void BlinkAll(int delay_ms);
  void LeftToRight(int numberofLEDs, int delay_ms);
  void RightToLeft(int numberofLEDs, int delay_ms);
  void BlinkAlternate(int delay_ms);
private:
  int _channel;
  int _pins[16];
};

#endif

Sample Code

#include "LED.h"

int const LED_pins[8]={5,6,7,8,9,10,11,12};
int const No_of_LED = 8;

LED led(8,LED_pins);

void setup()
{
  Serial.begin(9600);
}
void loop()
{
  led.LeftToRight(1,500);
  led.RightToLeft(1,500);
}
  • Please show us a full code. Meaning also the LED.h and a small test sketch, which uses your library and exhibits the problem. – chrisl Oct 20 at 20:05
  • Directly I think you have unballanced curly brackets in the LeftToRight() function and you use an array index out of bounds in the RightToLeft() function (by effectively doing _pins[_channel] though the greatest index should be _channel-1) – chrisl Oct 20 at 20:07
  • if you correct the indentation levels in your code, then you may be able to see the problem area yourself – jsotola Oct 20 at 21:52
  • pinMode shouldn’t be in the constructor. You need a begin method that can be called from setup for that. The constructor May get run before setup and before the hardware is ready for pinMode. – Delta_G Oct 21 at 15:35
0

It seems in the LeftToRight method, you didn't comment out enough (or too much), because you use a for loop inside another for loop which is probably unintended.

In the RightToLeft' method I will try to fix the problem, so you can change theLeftToRight` method similarly.

void LED :: RightToLeft(int numberofLEDs,int delay_ms)
{
  for(int i =_channel ;i>0; i--)
  {
    digitalWrite(_pins[i], LOW);
  }
  for(int i = _channel ;i>0; i--)
  {
    if (i < _channel)
    {
     digitalWrite(_pins[i+1], LOW);
    }
    digitalWrite(_pins[i], HIGH);
    delay(delay_ms);
  }
}

The first for loop is to switch off the LEDs, I would make this a separate method, so it can be reused in both the LeftToRight and RightToLeft method, so you get:

void LED::RightToLeft(int numberofLEDs, int delay_ms)
{
  switchAllLedsOff();

  for(int i = _channel; i > 0; i--)
  {
    if (i < _channel)
    {
     digitalWrite(_pins[i + 1], LOW);
    }
    digitalWrite(_pins[i], HIGH);
    delay(delay_ms);
  }
}

void LED::SwitchAllLedsOff()
{
  for(int i = _channel; i > 0; i--)
  {
    digitalWrite(_pins[i], LOW);
  }
}

The problem is also, that you always write HIGH to a pin, since it overwrite the possible LOW assignment within the if statement. To correct this you get:

void LED::RightToLeft(int numberofLEDs, int delay_ms)
{
  switchAllLedsOff();

  for(int i = _channel; i > 0; i--)
  {
    digitalWrite(_pins[i + 1], i >= _channel ? HIGH : LOW);
    delay(delay_ms);
  }
}

You also can use an if statement, but in this case the ternary (? :) statement is more clear since the assignment is for a single destination (_pins[i + 1]). So in case i >= channel, it is set to HIGH, otherwise set to FALSE. I also inverted the operator, but you can keep it inverted too.

Third, _channel seems to be the number of elements in the array, if you use it in an index it should start with _channel - 1, so you get:

void LED::RightToLeft(int numberofLEDs, int delay_ms)
{
  switchAllLedsOff();

  for(int i = _channel - 1; i >= 0; i--)
  {
    digitalWrite(_pins[i + 1], i >= _channel - 1 ? HIGH : LOW);
    delay(delay_ms);
  }
}

There might be more problems, but the above are at lest some of them.

Also I changed your way of using spaces, although it's a matter of personal style, yours I never saw before (which doesn't mean it's incorrect of course).

(note I have not tested the program).

  • Thanks Mr.Michel Keijers. Will work on it and let you know – Mr.B Oct 21 at 6:21
  • Mr.Michel I tried the RightToLeft() Code of yours but nothing is lit so I changed i>=_channel to i <= _Channel then it is lit but the LED keeps glowing without turning off like a bargraph instead of a LED scroll – Mr.B Oct 21 at 17:41
  • I thought you wanted a graph, not a single LED, in that case you can remove the for loop and use set one pin only to high. – Michel Keijzers Oct 21 at 18:15
  • Thats usefull, Thanks Mr.Michel Keijzers. But I also figured out some hardware loose connections which made the led in 5th pin not glow . Still I can't figure out why 6th pin is not glowing. other than that It works . – Mr.B Oct 21 at 20:12
  • Check if the wire is good, by measuring it with a multi meter, or replace the led, and the wire ( so 4 combinations), until you find out if at least the component and the wire is ok. – Michel Keijzers Oct 21 at 20:18

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.