Creating two object with same class type - strange results.

I'm having issues with two objects of the same class type. I have a LED class, which has functions to fade in and out, a LED. In my code, I create two objects of this class, and on the board, I fade them in and out.

Code is in my FadingLED.h file, which I #include.

I instantiate these classes - and then in my setup(), I call an Init function on the classes to initialise them and pass all the parameters I need.

And then in the loop, I just keep calling an Update method, which internally, uses Millis() to handle updating the brightness of the LED.

But I think I am having cross contamination of the objects. Because if I trigger them both to fadein and out at the same time, only the first LED fades. If I stagger them, the 2nd LED works, but it's timing seems to be out. It seems like maybe I'm somehow mixing member variables within the class. I'm new to this, so it's probably an obvious fault?

My calling code:

// This #include statement was automatically added by the Particle IDE.
#include "Buzzer.h"
#include "FadingLED.h"

// Pins being used by devices.
int LED1_PIN = A4;
int LED2_PIN = A5;
int PIR_PIN = 3;
int BUZZER_PIN = 0;

// Instantiate the objects
FadingLED LED_1;
FadingLED LED_2;
Buzzer buzzer;

// Setup global variables.
bool AllStill = true;

// Setup.
void setup() {
// Initialise the two LEDs
LED_1.Init(LED1_PIN, 500, 0, true);
LED_2.Init(LED2_PIN, 500, 250, true);
// Set the PIR pinmode.
pinMode(PIR_PIN, INPUT);
Particle.publish("Device","Board initialised", PUBLIC);
buzzer.Beep(100, 2200);
}

// Function to check if movement has been detected.
bool CheckMovement() {
// Read the pin value.
int val = digitalRead(PIR_PIN);

// Has a new movement been detected?
if(val==true && AllStill==true) {
buzzer.Beep(200, 2200);
AllStill = false;
Particle.publish("Device", "Movement Detected", PUBLIC);
}
AllStill = !val; // All still is false if we have movement.

return val != 0;
}

void loop() {
// Check for moment.
bool val = CheckMovement();
// Update connected devices.
LED_1.Update(val);
LED_2.Update(val);
buzzer.Update();
}

Below is the class it's self. It's pretty basic... Just runs up and down the brightness for the given period that the LED must be on for. So in my code, I'm just telling it how long it must complete one fade cycle (Fade up, and down in that time). It's a bit messy (The way to works out the brightness) but it works for now. The issue seems to be - my classes are mixing variables, I think.

/*

  • A class that manages a fading LED. This needs to be on PWM pin.
    */
    class FadingLED {
    int delayedStart = 0; // Period to wait before starting.
    int ledPin = -1; // The pin to control - Note, as we're fading, we must use an ANALOG pin.
    long fadePeriod = 0; // The time to do a full light and dim cycle.
    unsigned long previousMillis = 0; // The previous millis value.
    bool debug = false; // Should we output stuff?
    float fadeChangeGap = 0; // Calculated gap between brightness changes.
    unsigned long firstMillis = -1;

public:
void Init(int LedPin, int FadePeriod, int DelayedStart, bool Debug) {
ledPin = LedPin;
fadePeriod = FadePeriod;
delayedStart = DelayedStart;
debug = Debug;
pinMode(ledPin, OUTPUT);
Particle.publish("Debug", "LED Initialised on " + String(ledPin) + ", Fade Period is " + String(fadePeriod) + ", Start Delay is " + String(delayedStart), PUBLIC);
}

void Update(bool isOn) {

// Get the current millis since the board was started.
unsigned long currentMillis = millis();

// Is this the first time this code is running?
if(firstMillis == -1)
firstMillis = currentMillis; // Set the first time this class was run.

// Adjust the current time to be based on the current time since the object was created.
currentMillis = currentMillis - firstMillis;

// Calculate the difference bwteen the last update and now.
unsigned long diff = currentMillis - previousMillis;

int brightness = 0;

// Check if we have a delay set. If so, don't light up yet.
if(delayedStart > 0 && currentMillis < delayedStart) {
Particle.publish("LED", "In Delay Start...", PUBLIC);
previousMillis = currentMillis;
return;
}

// Map the difference which is betweem 0 and the cycle time, to a value between 0 and 512.
// Map the difference, which is between 0 and the fade period, to a value between 0 at 512.
int b = map(diff, 0, fadePeriod, 0, 512); // 512, as we'll brighten to 256, then then fade from 256 to 512.

// Check if we should be bringtning, or fading...
if(b > 255) {
brightness = 255 - (b-255);
}
else
{
brightness = b;
}

if(isOn == false) {
brightness = 0;
}
// Set the brightness.
analogWrite(ledPin, brightness);

// And then save the timestamp.
if(currentMillis - previousMillis >= fadePeriod) {
previousMillis = currentMillis;
}
}
};

Any help would be great. Am I creating objects in the wrong way?
If I create just one object... it works fine.
I've tried:
FadingLED LED_1 = new FadingLED();
But get:
"conversion from 'FadingLED*' to non-scalar type 'FadingLED' requested"

This is problematic:

unsigned long   firstMillis = -1;

How can an unsigned long EVER be -1?

Regards,
Ray L.

Excellent point, Ray! I'll fix that.
Not sure it's the cause of my symptoms though. I'll fix that.

RayLivingston:
This is problematic:

unsigned long   firstMillis = -1;

How can an unsigned long EVER be -1?

This is perfectly valid. I doesn't mean that firstMillis will hold the value of -1, but it guaranteed by the language that firstMillis will receive the maximum value of unsigned long type. After that you can compare firstMillis == -1 and it is guaranteed to compare equal.

This is actually idiomatic usage, used quite widely in the actual code. You don't have to use it yourself, but you have to be able recognize and understand it.

And it is used correctly in the OP's code. Nothing to correct here.

Cralis:
// Pins being used by devices.
int LED1_PIN = A4;
int LED2_PIN = A5;

Why are you using A-pins for PWM output purpose?

Note, that "The analogWrite function has nothing to do with the analog pins" (!) (see here).

Pins that support analogWrite are different from board to board. Are you sure that both A4 and A5 on your board support it? The link above lists, which pins can be used for analogWrite on each board.

Thanks Montmorency - I'll look into that. I've just assumed that A0-A5 were Analog 0 to Analog 5. And then D0 to Dx were al Digital - some supporting PWM. I'm very new to this so go easy on me. :slight_smile: But I thought digital was On and Off. But as I am fading the LED... I thought I'd need to send values to modify the brightness.

Seems I have misunderstood that. Let me read up.

I'm switching between my Arduino Uno, and a Particle Photon. At the moment, I'm using the photon. Pins guide states:

A0~A7
12-bit Analog-to-Digital (A/D) inputs (0-4095), and also digital GPIOs. A6 and A7 are code convenience mappings, which means pins are not actually labeled as such but you may use code like analogRead(A7). A6 maps to the DAC pin and A7 maps to the WKP pin. A4,A5,A7 may also be used as a PWM[2] output.

D0~D7 Digital only GPIO pins. D0~D3 may also be used as a PWM[2] output.

The last bit about the "A4,A5 ,A7" use as PWM. As I'm using A4 and 5, that should be OK to use AnalogWrite?

The code itself is a bit disorganized, but from the functionality perspective it looks perfectly fine to me. In fact, if you are "very new to this", this is very impressive.

Have you tied running it on UNO, using one of the PWM-enabled pins?

Thanks Montmorency.

I'm actually a C# and SQL Server developer - but this is my first attempts with C++, and single threading. So it looks highly messy to me too (haven't worked out a good naming convention as well), but .. not sure how to make it better. Any tips would be much appreciated.

I'll transplant the components onto my Arduino UNO board and see if it helps. But any tips and tricks would be great.

I still think I have a code issue though. If I disable the Update() on LED1, then LED2 workds perfectly. Fades in and out as expected. As soon as I put the Update() for LED1 back .. LED2 behaves strangely. (Not fading... seems to flash, and too fast). Also, if I initialise them to startat the same time, and last the same amoutn of time ... the LED2 stays off all together. Only LED1 fades in and out.

So this inicates to me that it's a code issue, and not really a pin out issue. But I'll transplant it now to be sure.

Cralis:
I still think I have a code issue though. If I disable the Update() on LED1, then LED2 workds perfectly. Fades in and out as expected. As soon as I put the Update() for LED1 back .. LED2 behaves strangely. (Not fading... seems to flash, and too fast). Also, if I initialise them to startat the same time, and last the same amoutn of time ... the LED2 stays off all together. Only LED1 fades in and out.

So this inicates to me that it's a code issue, and not really a pin out issue. But I'll transplant it now to be sure.

Maybe I'm missing something, but I see absolutely no way how the two objects of your class can interfere with each other at software level. This is what makes me believe that this is most likely a hardware interference.

Thanks Montmorency - doing transplant now.
In .Net, I'd need to 'new up' an object. So, would expect to use the 'new' keyword somewhere. In my code, I only seem to allocate space to my LED objects (FadingLED LED_1;)... I never use 'new'. Is that normal?

Cralis:
So, would expect to use the 'new' keyword somewhere. In my code, I only seem to allocate space to my LED objects (FadingLED LED_1;)... I never use 'new'. Is that normal?

It is perfectly normal.

You don't need new here. In C++ plain new is only used to create objects in dynamic memory. There's absolutely no need to involve dynamic memory in this example.

Your LED_1 and LED_2 are global/static objects that are created by namespace-level definitions (as in your code). No need for any new.

Cralis:
I'm having issues with two objects of the same class type. I have a LED class, which has functions to fade in and out, a LED. In my code, I create two objects of this class, and on the board, I fade them in and out.

.
.
.

There's a couple of things in your code that give me pause but I thought I'd try a slightly more formal re-writing of your fade class for shits and giggles before mentioning them. The sketch below and attached cpp and h files compiles for my 2560 and, to the degree I can test it without a full setup, seems to work...

I don't have your buzzer class so that code is commented out, as is the "particle" stuff (which I also don't have.) You can re-enable that stuff as you please.

Note that pin 0 (you buzzer output) is also the serial RX pin; not sure if that will have any effect on your project's performance...

The class name is preserved but the file names of the cpp and h files are different to avoid conflict on your side.

//#include "Buzzer.h"
#include "LEDFade.h"

// Pins being used by devices.
int LED1_PIN = A4;
int LED2_PIN = 13;//A5;
int PIR_PIN = 3;
int BUZZER_PIN = 0;

// Instantiate the objects
FadingLED LED_1;
FadingLED LED_2;

//Buzzer buzzer;

// Setup global variables.
bool AllStill = true;

// Setup.
void setup() 
{
    // Initialise the two LEDs
    LED_1.Init(LED1_PIN, 500, 0, true);
    LED_2.Init(LED2_PIN, 500, 250, true);
    // Set the PIR pinmode.
    pinMode(PIR_PIN, INPUT);
    //Particle.publish("Device","Board initialised", PUBLIC);
//    buzzer.Beep(100, 2200);
}

// Function to check if movement has been detected.
bool CheckMovement() 
{
    // Read the pin value.
    int val = digitalRead(PIR_PIN);

    // Has a new movement been detected?
    if(val==true && AllStill==true) {
//        buzzer.Beep(200, 2200);
        AllStill = false;
        //Particle.publish("Device", "Movement Detected", PUBLIC);
    } 
    
    AllStill = !val; // All still is false if we have movement.

    return val != 0;    
}


void loop() 
{
    // Check for moment.
    bool val = CheckMovement();
    // Update connected devices.
    LED_1.Update(val);
    LED_2.Update(val);
    
//    buzzer.Update();
}

LEDFade.cpp (2.23 KB)

LEDFade.h (830 Bytes)

Thanks Blackfin! I never knew you could use the cpp files. I thought the lot was in the .h file!
Thanks! Just trying to understand the new concept now.

Is the .h file more like an interface type file, and the cpp has the concrete implementation? This has got quite foreign - but really worth learning. Thanks.

When we call:
FadingLED LED_1;

How does it find the class implementation?
At the top of the main file, we have:

#include "FadingLED.h"

So the code will find the fadingLED.h file, which has a class declaration, but it only has properties.

Will the compiler then hunt for a fadingLED.cpp (automatically) by name convention, to get the methods for the class? The cpp seems to have a reference to the .h file as well, which seems:

Main file -> .h file -> .cpp file -> .h file.

Oh wait...
The cpp file has -> #include "FadingLED.h"

So... the class declaration will be copied into the cpp file....
So the cpp file will have the class definition at the top (where the include happens).
But the class methods will be outside of the class. That confuses me.

Blackfin:
There's a couple of things in your code that give me pause but I thought I'd try a slightly more formal re-writing of your fade class for shits and giggles before mentioning them. The sketch below and attached cpp and h files compiles for my 2560 and, to the degree I can test it without a full setup, seems to work...

Some remarks about the rewritten code:

  1. The original code used C++11-style in-class initializers for all fields, which is equivalent to using a constructor initializer list. This is a proper thing to do and a very good programming practice when writing C++ classes.

The rewritten code removed these initializers, which in general case will make the default-constructed object contain garbage values. This is a major downgrade from the original code.

  1. Variables should be declared as locally as possible. The best scenario is when a variable can be declared with a meaningful initializer. Piling up variable declarations at the top of the function is a major no-no, especially when they can't be supplied with a meaningful initializer there.

And this is what I see in the rewritten version. It is true that the original code was a bit disorganized with variable declarations: some variables were declared earlier that necessary. But at least the original code was making an effort to declare variables with initializers (!). In this respect the rewritten version is a major downgrade from the original code.

  1. Explicitly declaring (and defining!) the default constructor in this class is completely unnecessary - the compiler will do it for you. And doing it after you deliberately destroyed its functionality in point 1 (!)... it looks like a mockery of the poor default constructor.

  2. if( isOn == true )

Um... No. This problem is present in both versions. Please, simply use if (isOn). With non-bool values an explicit comparison is quite welcome. But with bool values it is just... inappropriate, to put it mildly.

  1. (void) function declarations in C++ code? Hmm... Weren't they planning to deprecate it?

Cralis:
Will the compiler then hunt for a fadingLED.cpp (automatically) by name convention, to get the methods for the class?

No, the compiler will never hunt for the .cpp file unless you tell it to do so. C and C++ compiler consume only what is explicitly fed into them.

There's no "naming convention" of any kind. There's no connection between header file and .cpp file, even if they have the same name. You can give your .cpp file a completely different name - it doesn't matter.

In traditional C++ build environment it would be your responsibility to gather all .cpp files needed for your program and feed them into the compiler. The compiler would translate each .cpp file into a separate object file(.o file for GCC compiler). And then the compiler (or you) would have to feed all these object files into the linker, which would produce the final executable.

This process is usually batched/automated by using scripts, makefiles and various other build environments, like IDE. Arduino IDE does that for you too. It simply feeds all files stored in your sketch directory into the compiler and then linker (maybe it excludes some extensions).

Thanks Montmorency.

I'd just rewritten it all to use that new format. :slight_smile: You live, and you learn.

if(isOn) <--- Yup, will fix. I did that a couple of times. I kept looking at it with the "Wait, I can simplify that!", but kept forgetting. :slight_smile:

With regards you comments, is it therefore OK to have all the code for each class, in a single file? It looks like we're splitting the declaration, and then implementation now. Having it all in on .h (or .cpp?) file would seem better and easier to maintain.

And when you mention the newer c++ style - does that mean things like:
void Buzzer::Beep(int ToneTime, int Pitch) {

Would just be:

void Beep(int ToneTime, int Pitch) {

within the class declaration?

And then we don't need the delcarations:

public:
Buzzer( void );
void Init( int );
void Beep( int, int );
void Update( void );

which are currently in the .h file?

FYI, before I break up my project to try on the Arduino (My kids play with the project and wanted me to be able to rebuild it with the Photon later - as it's small and compact - so I attempted to contribute:

http://fritzing.org/projects/motion-detector-with-alarm-and-leds

Cralis:
And when you mention the newer c++ style - does that mean things like:
void Buzzer::Beep(int ToneTime, int Pitch) {

Would just be:

void Beep(int ToneTime, int Pitch) {

within the class declaration?

And then we don't need the delcarations:

public:
Buzzer( void );
void Init( int );
void Beep( int, int );
void Update( void );

which are currently in the .h file?

No, method declarations have to be present inside the class definition. And if you make method definitions separately, you will have to repeat the full name of the method (and parameter list) in the definition. There's no way around it.

When it comes to declaring/defining class methods you basically have 3 choices in C++:

  1. In-class implementation
class MyClass
{
  int my_method(int a, int b, int c)
    { return a + b + c; }
};

The above method is implicitly inline. You don't need to write anything for it in .cpp file. (You might not even need a .cpp file for your class)

  1. Out-of-class inline implementation
class MyClass
{
  int my_method(int a, int b, int c);
};

inline int MyClass::my_method(int a, int b, int c) 
{ 
  return a + b + c; 
}

All this goes into the header file. Again, you don't need to write anything for it in .cpp file. (You might not even need a .cpp file for your class)

  1. Non-inline implementation (uses a .cpp file)

In header file you have

class MyClass
{
  int my_method(int a, int b, int c);
};

In .cpp file you have

int MyClass::my_method(int a, int b, int c) 
{ 
  return a + b + c; 
}

That's the "classic" two-file approach.


If your class's methods are lightweight, you can implement everything in the header file using either method 1 or 2. In that case you will not need a .cpp file for that class at all. In fact, with modern compilers you can use these approaches even for "heavy" methods. I.e. "ditch the .cpp files entirely and implement everything in the headers" philosophy. It will make the compiler work harder when compiling your program, but the resultant code might end up better optimized. Some people love this header-only approach, some people hate it...

That is brilliant. Thanks for the help, Montmorency.
Just because of my .Net/c# background, and the simplicity of what I am trying to do, I'll go with option 1 for now, which is really what I had. And then remove the poor coding issues you pointed out. Well, that you started pointing out. I'm sure there's a load more.
I'll get that working again on the Photon, and then transplant it across to an Arduino.

Here's the latest code. I've tried to do everything mentioned.
Still have the same initial issue. But I'm close to ready to move the components across to my Arduino.

Buzzer.h (738 Bytes)

FadingLED.h (2.83 KB)

motiondetector.ino (1.17 KB)