As an exercise I'm trying to develop my own simple class librray to handle an LED, using millis() for timing.
I think I'm having problems testing it because the sketch and class both use millis() for timing. Here is the sketch and library code.
I'd be very grateful for any advice on improving the code - I'm here to learn.
led.h
#ifndef MY_LED_H
#define MY_LED_H
#include <Arduino.h>
class Led {
private: // leading underscore is a convention to show this is a member variable for the class
byte _pin;
boolean _currentState; //records whether led is on or off
const bool _ON=LOW, _OFF=HIGH; //LED is connected to +Vcc
int _tOn, _tOff; //
byte _flashing, _nTimes; //
unsigned long _timeWas = 0; // will store time LED was last updated
unsigned long _timeNow;
public:
Led(byte pin);
void init();
void on();
void off();
bool change();
// flash() is overloaded;
// bool flash(int t);
bool flash(int tOn,int tOff);
//bool flash(int tOn,int tOff, byte nTimes);
byte flashing;
};
#endif
led.cpp
#include "Led.h"
Led::Led(byte pin) {
// private variable _pin takes value from value passed to constructor
_pin = pin;
init();
}
void Led::init() {
pinMode(_pin, OUTPUT);
_timeNow = millis();
_timeWas = _timeNow;
off();
}
void Led::on() {
_currentState = _ON;
digitalWrite(_pin, _currentState);
}
void Led::off() {
_currentState = _OFF;
digitalWrite(_pin, _currentState);
}
bool Led::change() { //whatever it was, change it
//digitalWrite(_pin, !digitalRead(_pin)); //doesnt work!
//_currentState = (_currentState == LOW) ? HIGH : LOW; //this works but why so complicated?
_currentState = !_currentState;
digitalWrite(_pin, _currentState);
_timeWas = _timeNow; //record time of last change
return (_currentState);
}
bool Led::flash(int tOn, int tOff) {
_tOn = tOn;
_tOff = tOff;
_flashing = HIGH;
//flash LED for time t
_timeNow = millis();
if (_flashing == HIGH) {
if ((_currentState == _OFF) && (_timeNow - _timeWas >= _tOn)) {
change(); //turn LED on
}
else if ((_currentState == _ON) && (_timeNow - _timeWas >= _tOff)) {
change(); //turn LED off
_flashing = LOW; //flash completed
Serial.println("flash completed");
}
}
flashing=_flashing;
return(flashing);
}
not too sure about how to return a value to the sketch?
and I think my button management timing is interfereing but not too sure how to fix it.
OOPExample.ino
//https://roboticsbackend.com/arduino-object-oriented-programming-oop/
#include "led.h"
#define LED_1_PIN 9
#define LED_2_PIN 6
#define BUTTON_PIN 4
bool oldState, newState = LOW;
unsigned long tMillis, tNext = 0;
int flashing = 1;
// Create your objects in the global scope so you can
// get access to them in the setup() and loop() functions
Led led1(LED_1_PIN); //constructor initialises led pin & OFF
Led led2(LED_2_PIN);
void setup() {
Serial.begin(9600);
pinMode(BUTTON_PIN, INPUT_PULLUP);
newState = digitalRead(BUTTON_PIN);
oldState = newState;
}
void loop() {
newState = digitalRead(BUTTON_PIN);
if ((newState != oldState) && (newState == LOW) )
{
flashing = 1;
if (flashing) {
flashing = led1.flash(100, 200);
}
led2.change();
tNext = millis();
}
tMillis = millis();
if (tMillis - tNext >= 1000) {
Serial.print("Button state is ");
Serial.println(newState);
oldState = newState;
tNext = millis();
}
}
No time to look deep into into your code.
You should not call your current init() method from within the constructor as there is no guarantee that all 328P registers are already initialised.
Call the init() method from setup().
//digitalWrite(pin, !digitalRead(_pin)); //doesnt work!
//_currentState = (_currentState == LOW) ? HIGH : LOW; //this works but why so complicated?
This looked very familiar
Why did you start a second topic on the same subject, particularly when you had marked the original one [solved] ?
https://forum.arduino.cc/?topic=703267
Hi UKHeliBob thanks for responding; the previous problem was resolved - code blocking the millis() timing - so I felt it was right to start a new thread.
I've further developed the code and the timing is working fine, but I'm now trying to further my knowledge (all easy stuff to you) such as returning a value from the library to the sketch, and overloading the flash() method.
Terminology is new to me also - objects, methods, public & private etc. so sorry if my explanations arent clear.
I studied your example pinToggle and I see you have an update() method that takes care of timing.
I THINK the timing in the sketch is interfering with the timing in the library and causing issues. So I'm not sure how to handle two different things that need seperate timing.
To get flash() to do just a single flash I had to return a value to the sketch which I feel is clunky.
The behaviour I am currently seeing is that I do get a single flash if I momentarily press the button. If I hold it closed I get repetitive flashes - probably due to the logic around the button press;
However a momentary press SHOULD (ie is intended to) turn the LED on for 100ms and OFF for 200;
so the end state should always be off. However I'm seeing it sometimes end in the ON state.
I studied your example pinToggle and I see you have an update() method that takes care of timing.
An update() (or equivalent) function is quite commonly used and I used it in my library as I had seen it used elsewhere. Incidentally I wrote the library to teach myself more about libraries so I know where you are coming from
As to the timing in yours, there is no reason why the timing in the main sketch should have any effect on that used with objects of the library as long as different variables are used. I don't think that you have described what is going wrong with your current sketch or, indeed, exactly what it should do.
Did you note and correct the possible problem with the init() function noted in reply #1 ?
The purpose of the sketch is just to test the operation of the various led() functions.
I've tried many different ways to do that (starting with delay() )
and the on(), off() and change() all work fine -
but although I have seen it work as described above, the flash() doesnt.
I've simplified the sketch as follows and added serial.prints.
void loop() {
newState = digitalRead(BUTTON_PIN);
if (newState != oldState) {
if (newState == LOW) { //button has just been pressed
Serial.println("button has been pressed");
flashing = 1;
if (flashing) {
flashing = led1.flash(100, 200);
}
led2.change();
oldState = newState;
}
else { //button has been released
Serial.println("button has been released");
oldState = newState;
}
}
}
This is what happens on the serial monitor:
button has been pressed LED comes on & stays on
button has been released
button has been pressed
flash completed LED goes off & stays off
button has been released
What I expect: after the first press the LED should go on, stay on for 100ms, then go off and stay off for at leats 200ms. (that will only be relevant when the flash repeats)
Did you note and correct the possible problem with the init() function noted in reply #1 ?
I didnt understand - but i tried it with no change. It seems neater calling it from the constructor.
Is the constructor called before setup()?
What if the init() code had been written in the constructor?
Led led1(LED_1_PIN); //constructor initialises led pin & OFF
Led led2(LED_2_PIN);
void setup() {
Serial.begin(9600);
led1.init();
led2.init();
pinMode(BUTTON_PIN, INPUT_PULLUP);
newState = digitalRead(BUTTON_PIN);
oldState = newState;
}
}
Is the constructor called before setup()?
Yes, and that is what can cause a problem
Every C/C++ program has a main() function but the Arduino environment hides it from the user. In essence it is like this
int main(void)
{
initHardware();
setup();
for (;;) {
loop();
}
return 0;
}
So it is quite possible for the init() function of the class to be called before the hardware is initialised if it is called from the constructor. Calling the class init() function from setup() ensures that the system hardware initialisation is complete before doing things like pinMode()
I will try your sketch later if I have time
I have not had time to look in detail but why do this
flashing = 1;
if (flashing)
and
_flashing = HIGH;
//flash LED for time t
_timeNow = millis();
if (_flashing == HIGH)
In both cases you set a variable to a specific value then test it
My head has been in the wrong place; I've been thinking of an object method as being an independent entity like an ISR. Which it isnt.
Your comment about the update() process got me thinking and I've realised that while I can START a method like flash() I cant start it repeatedly in loop()
so I think I'll need an update() method to service it once its started.
Thats a bit annoying in a way because the non-time dependent mthods - on(), off(), change() will then be different to flash().
Its a bit like the issue with init() which would be nice to be implicit in the constructor.
Its also made me think I need to develop flash(t1, t2, n) first, then set n=1 for a single flash(t1,t2) and t2=t1 for flash(t).
I'll try in the morning.
Thats a bit annoying in a way because the non-time dependent mthods - on(), off(), change() will then be different to flash().
Different in what way ?
The update() function will be called from loop() (or by a hardware timer) and will check whether timed actions are required and if so, do what is necessary to service them. It may well call on(), off() or change() to do what is needed but if you choose to make those functions public they could also be called explicitly by the program at any time
Its working!
And I forced it to be symmetrical by moving this line to the update() method.
digitalWrite(_pin, _currentState);
I've attached my files for completeness, but here is the interesting bit.
bool Led::flash(int tOn, int tOff, byte nTimes) {
_tOn = tOn;
_tOff = tOff;
_nTimes = nTimes;
_currentState = on();
return (_currentState);
}
bool Led::flash(int tOn, int tOff) {
_tOn = tOn;
_tOff = tOff;
_nTimes = 1;
_currentState = on();
return (_currentState);
}
bool Led::flash(int tOn) {
_tOn = tOn;
_tOff = tOn;
_nTimes = 1;
_currentState = on();
return (_currentState);
}
bool Led::update() {
digitalWrite(_pin, _currentState);
if (_nTimes > 0) { // we need to flash
if (_currentState == _ON) {
_timeNow = millis();
if (_timeNow - _timeWas >= _tOn) { //if _tOn expired turn led off
off();
}
}
else {
_timeNow = millis();
if (_timeNow - _timeWas >= _tOff) {
_nTimes--;
if (_nTimes > 0) {
on();
}
}
}
}
}
OOP_example1.ino (1.05 KB)
led.cpp (1.99 KB)
led.h (753 Bytes)
Good news that it is working, which is the important thing
The update() function looks a little clumsy with some repeated code. It could be made neater/shorter if you put the on/off periods in an array (levels 0 and 1) and use the current LED state to determine the current period
So the update function basically does this
if this object is not subject to timing
return
get time now
if time now - start time >= current period from the array indexed using current state
change LED state (current state = !current state)
write the state to the LED
save the start time
end if
Yes I'm not happy with the structure - I find it a bit hard to understand.
I'll think about your idea, thanks, I see where its going.
One last question (for now) - going back a way - is there a reason to prefer 1: to 2: ?
1: _currentState = (_currentState == LOW) ? HIGH : LOW;
2:: _currentState = !_currentState;
Oh - and another - why in the ide are init and update shown in red?
Many thanks for your help
Your init method should not be called from the constructor. The correct place to touch the hardware for the first time is in setup.
is there a reason to prefer 1: to 2: ?
Personal preference
Ease of understanding
why in the ide are init and update shown in red?
Because somewhere in some library the names are used and have been put into the keywords.txt file for that library. The keyword colouring system is dumb in that it does not only reference keywords.txt for libraries used in the sketch but all instances of keywords.txt
Thanks - I wondered if this was academically valid -but it seems to work.
_currentState = !_currentState;
Because somewhere in some library the names are used and have been put into the keywords.txt file for that library. The keyword colouring system is dumb in that it does not only reference keywords.txt for libraries used in the sketch but all instances of keywords.txt
thanks again
Just an update - I followed up your suggestion UKHeliBob and here is the result:
a LOT simpler and I like the symmetry.
I've also learnt that you CAN index an array with a boolean!
bool Led::flash(int tOn, int tOff, byte nTimes) {
_t[0] = tOn;
_t[1] = tOff;
_nTimes = nTimes << 1;
_currentState = on();
return (_currentState);
}
bool Led::update() {
digitalWrite(_pin, _currentState);
if (_nTimes > 0) { // we need to flash
_timeNow = millis();
if (_timeNow - _timeWas >= _t[_currentState]) { //if _tOn expired turn led off
_nTimes--; //_nTimes is set to the number of phases ie twice the number of flashes.
change();
}
}
else off(); //if no further flashes are required LED should be in the off state
return (_currentState);
}
digitalWrite(_pin, _currentState);
Why do this ?
Have you fixed the potential problem with the init() function being called from the constructor ?