Odd Lib Behavior (C++ question) [Solved]

BulldogLowell · July 18, 2015, 6:05am

So, I am not sure if this is a Photon question… I made a little library for a non-blocking led fade.

Header File:

#ifndef Fade_h
#define Fade_h

#include "application.h"

class Fade
{
  public:
    Fade() {};
    Fade(int pin, uint32_t timeStep = 10, uint8_t min = 0, uint8_t max = 255);
    void write(int to);
    void update();
    void update(uint32_t time);
    uint8_t read();
    uint32_t readSpeed();
    uint32_t writeSpeed(uint32_t time);
  private:
    uint8_t _min;
    uint8_t _max;
    uint8_t _targetFade;
    uint8_t _pwmRate;
    uint32_t _time;
    uint32_t _last;
    uint32_t _pin;
};

#endif

and implementation file:

#include "application.h"

#include "Fade.h"


Fade::Fade ( uint32_t pin, uint32_t timeStep, uint8_t min, uint8_t max)
{
  _pin = pin;
  _time = timeStep;
  _min = min;
  _max = max;
  pinMode(_pin, OUTPUT);
  analogWrite(_pin, _min);
  _pwmRate = _min;
}

void Fade::write(int to)
{
  _targetFade = (uint8_t) constrain(to, _min, _max);

  this->update();
}

void Fade::update()
{
  this->update(millis());
}

void Fade::update(uint32_t time)
{
  if (time - _time > _last)
  {
    _last = time;
    if (_pwmRate > _targetFade) analogWrite(_pin, --_pwmRate);
    if (_pwmRate < _targetFade) analogWrite(_pin, ++_pwmRate);
  }
}

uint8_t Fade::read()
{
  return _pwmRate;
}

uint32_t Fade::readSpeed()
{
  return _time;
}

uint32_t Fade::writeSpeed(uint32_t time)
{
  _time = time;
}

Note that I set pinMode in the constructor.

but, I MUST set pinMode again in this example for the PWM to function:

#include "Fade.h"
#include <application.h>

int myPin = A4;

Fade led1(myPin, 5);// fader pin 11 speed 25 ms/ step
unsigned long startMillis;
bool toggle;

void setup()
{
  Serial.begin(115200);
  pinMode(myPin, OUTPUT);  //<<<<<<<<<<<< comment this out and it doesn't output
  led1.write(255); //fade to max
}

void loop()
{
  led1.update();
  if (millis() - startMillis > 5000UL)
  {
    toggle = !toggle;
    led1.write(toggle? 255 : 0);
    Serial.print("fading led1");
    Serial.println(toggle? "up" : "down");
    startMillis = millis();
  }
}

the only thing I can notice is that the (Particle Dev) editor doesn’t highlight the OUTPUT keyword in the Fade.cpp file:

but does in my ino file:

Can anyone see what I am doing wrong?

mdma · July 18, 2015, 6:19am

I’m puzzled. The photon does initialize all pins to input on startup, but this happens before the application setup()/loop() is executed.

I’ve added it to our backlog and will investigate as soon as I can, but since you have a workaround, please understand this will come after the high priority issues.

If anyone in the community with a JTAG debugger wants to look at this in the meantime, that would be rad!

Thanks
mat.

BulldogLowell · July 18, 2015, 12:40pm

thanks for looking at my issue, I appreciate that!

no problem, but It is good for folks to know about this one in the case where people are porting libraries with some difficulty.

thanks again, you guys rock!

Bspranger · July 18, 2015, 12:55pm

Shouldn’t it be:

If (time - _last > _time)

…

BulldogLowell · July 18, 2015, 1:46pm

that works too:

_last = 990
time = 994
__time = 10

if (time - _time > _last)
(994 - 10 > 990) evaluates false

or yours

if (time - _last > _time)
(994 - 990 > 10) evaluates false

short answer: the fader works fine, it is just the problem of initializing the pinMode to output in the library, but thanks for looking at it!!!

kennethlimcp · July 19, 2015, 8:41am

I do use pinMode() in my other libraries and they work fine. Your uint32_t pin should be uint16_t IMO.

See: https://github.com/spark/firmware/blob/latest/wiring/inc/spark_wiring.h#L66

BulldogLowell · July 19, 2015, 1:37pm

funny you mention that. I looked at the spark_wiring header and I agree because I could not get my file to compile using either uint16_t or uint32_t, that is how I landed on using int.

Therin lies the problem, I guess. I couldn’t see anything (it’s probably above my pay grade anyways ).

thanks for your looking into this!

kennethlimcp · July 19, 2015, 1:49pm

So it doesn’t work with that? Mine works fine but i used #define for the pins.


#define U1CKV (D5)
#define U1SPV (D4)
#define GMODE (D2)
#define BM_PGOOD (A5)
#define EINK_VDD (A1)

void mylibrary::pinInit(void){
  pinMode(BM_PGOOD,INPUT);
  pinMode(BM_CHG,INPUT);

  pinMode(U1CKV,OUTPUT);
  pinMode(U1SPV,OUTPUT);
  pinMode(GMODE,OUTPUT);
}

Muskie · July 19, 2015, 2:02pm

Keep in mind that with C/C++ the initialization order for global space variables is undefined. This includes constructor execution of global class instances. I can’t say if this is related to your problem but it is an important consideration. Be careful of interdependencies.

BulldogLowell · July 19, 2015, 2:27pm

@kennethlimcp

so I changed libraries and now it compiles just fine, thanks for the help!

#ifndef Fade_h
#define Fade_h

#include <application.h>

class Fade
{
  public:
    Fade() {};
    Fade(uint16_t pin, uint32_t timeStep = 10, uint8_t min = 0, uint8_t max = 255);
    void write(int to);
    void update();
    void update(uint32_t time);
    uint8_t read();
    uint32_t readSpeed();
    uint32_t writeSpeed(uint32_t time);
  private:
    uint8_t _min;
    uint8_t _max;
    uint8_t _targetFade;
    uint8_t _pwmRate;
    uint32_t _time;
    uint32_t _last;
    uint16_t _pin;
};

#endif

and implementation file:

#include <application.h>
#include "Fade.h"

Fade::Fade (uint16_t pin, uint32_t timeStep, uint8_t min, uint8_t max)
{
  _pin = pin;
  _time = timeStep;
  _min = min;
  _max = max;
  pinMode(_pin, OUTPUT);
  analogWrite(_pin, _min);
  _pwmRate = _min;
}

void Fade::write(int to)
{
  _targetFade = (uint8_t) constrain(to, _min, _max);

  this->update();
}

void Fade::update()
{
  this->update(millis());
}

void Fade::update(uint32_t time)
{
  if (time - _time > _last)
  {
    _last = time;
    if (_pwmRate > _targetFade) analogWrite(_pin, --_pwmRate);
    if (_pwmRate < _targetFade) analogWrite(_pin, ++_pwmRate);
  }
}

uint8_t Fade::read()
{
  return _pwmRate;
}

uint32_t Fade::readSpeed()
{
  return _time;
}

uint32_t Fade::writeSpeed(uint32_t time)
{
  _time = time;
}

and that now compiles, no issues there, but the solution to my problem is still elusive:

#include "Fade.h"
#include <application.h>

const int myPin = A4;  // i am guessing that the compiler is smart enough to demote the int to uint16_t??

Fade led1(myPin, 5);// faderPin speed 5 ms/ step
unsigned long startMillis;
bool toggle;

void setup()
{
  Serial.begin(115200);
  pinMode(myPin, OUTPUT);  //<<<<<<<<<<<< comment this out and it doesn't output
  led1.write(255); //fade to max
}

void loop()
{
  led1.update();
  if (millis() - startMillis > 5000UL)
  {
    toggle = !toggle;
    led1.write(toggle? 255 : 0);
    Serial.print("fading led1");
    Serial.println(toggle? "up" : "down");
    startMillis = millis();
  }
}

so there are monsters out there… is there a way to avoid the issue of the initialization order? I’m not sure that I could categorize what I am attempting to do in the constructor as creating an interdependency. Is there some kind of compiler directive I can add to push the treatment of my constructor to the end of the line?

Like @mdma mentioned, I have a workaround and that is simply setting the pinMode in setup() so for now I’m fine.

kennethlimcp · July 19, 2015, 2:28pm

Still one error here: in private–> int _pin;

BulldogLowell · July 19, 2015, 2:39pm

right, fixed it above, still have to use pinMode() but that is fine for now

thanks!

mdma · July 19, 2015, 4:50pm

On the Core this is definitely the case since the system and application are combined into one C++ module - initialization order between the system and application code can bite can cause unexpected bugs.

On the Photon this isn’t the case - the system globals are guaranteed to be fully initialized before the application globals are initialized since they are separate, dynamically loaded modules.

Muskie · July 19, 2015, 5:27pm

Thanks @mdma I suspected that with the Photon they would be separate. One must still be aware that within the process the ordering is not guaranteed. The OP showed an int initialization followed by a class instance initialization that depended on the int going first. While it may work (and probably will work) there are no guarantees.

BulldogLowell · July 19, 2015, 7:05pm

as @kennethlimcp mentioned (and better practice I’m sure) using the #define directive for the pin

#define LED_PIN A4

and the constructor:

Fade led1(LED_PIN, 5);

does the trick!

thanks guys for straightening me out!