# Pulse Proportional Thermostat - Strange behaviour

Guys, need some advice, especially as the person who was tutoring me is now no longer with us so i’m a little stuck.

Over the past year or so I’ve been developing a reptile environment controller using a Mega2560. The code may be very unstructured as I basically duplicated the single output section for the remaining 7 channels once we had the basic single channel working.

The code for the output stage should be self explanatory, but notes have been included. Basically if the temperature read from the DS18B20 is within a degree of the target temperature the pulse duration is adjusted each time the cycle loops round. If it is more than a degree it turns the pin low, if it less than degree its high. There are also some error checking to make the output low and sound a buzzer if the sensor can’t be read, or the temperature is outside the safe range.

``````void newoutput1 ()  {

frequency=5000;

error1 = ((setpoint1+0.5)-temperature1);                                        // Obtain the value for the difference between desired tempertaure and actual temperature
if (error1>1.0)                                                                 // if the difference is more than a degree
dutyCycle1=1.0;                                                               // then set the value of the duty cycle (pulse) to 100%
else if (error1<0.0)                                                            // or if the value is negative and below zero
dutyCycle1=0.0;                                                               // then set the value of the duty cycle to 0%
else                                                                            // or if the value is between 1.0 and 0.0
dutyCycle1=error1;                                                              // set the value of the duty cycle to match that value

if (error1 > 1.0) {                                                             // if the dutycycle value is 0
digitalWrite(output1, HIGH);                                                // set output 1 high
var1A = 1;                                                                  // and set var1A to 1

}
else {
digitalWrite(output1, LOW);                                                 // set output 1 low
var1A = 0;                                                                  // and set var1A to 0 and continue through this routine
}

if (temperature1 == -127.00)                                                    // if aa misread or incorrect response from DS18B20 produces a value of -127.00
{
dutyCycle1=0;                                                                   // set control to 0
digitalWrite(alarm, HIGH);                                                     // sound the buzzer to notify there is a problem
digitalWrite(output1, LOW);                                                    // turn off the heater
}

else if ((temperature1 >= AlarmHIGH1) || (temperature1 <= AlarmLOW1))           // if the read tempertaure is outside the high and low thresholds
{
dutyCycle1=0;                                                                 // set control to zero
digitalWrite(alarm, HIGH);                                                    // and sound the buzzer to notify the user of a problem
digitalWrite(output1, LOW);                                                   // turn off power to heater
}

heatpulse1 = (millis() + (frequency * dutyCycle1));                               // current  millis() time  + On time( freq*duty) in Milli seconds

do    {                                                                           // The DO  / WHILE CONDITIONAL LOOP

if (heatpulse1 > millis() && var1A == 0 ) {                                    // if Var1A is low, ie not running Full heating, then pulse ON, if needed.
digitalWrite(output1, HIGH);                                                  // if Var1A is High, full heating is ON , so - Skip
var1 = 1;
}
else if (var1A == 0) {                                                        // if Var1A is low, ie not running Full heating, then pulse can be turned OFF
digitalWrite(output1,LOW);                                                     // if Var1A is High  full heating is ON, so - Skip
var1 = 0;
}

if (var1 == 0)  {                                                              //  TEST IF ALL THE PULSES HAVE FINISHED, WHEN THEY HAVE VAR5 BECOMES ZERO
var9 = 0;
}
else var9 = 1 ;

}
while  ( var9 >0 );                                                               //  THE WHILE BOOL CONDITION   IF VAR9 IS GREATER THAN 0 CONTINUE WITH THE LOOP

void loop();
}
``````

The problem I’m experiencing is that when first booted it works fine. If the temperature exceeds the set point the monitoring LED goes out and remains off (and the same is for each of the eight outputs). But if left running for a few days, it then starts misbehaving, in that it never actually makes the output low if the temperature exceeds the set point, and the LED gives a very short but noticeable pulse.

As mentioned, the chap who was supporting me and helping me migrate form PICs and PicBASIC Pro is no longer here, so I’m a tad stuck as to why it does this.

My guess… is it something to do with millis timer that somehow isn’t being reset and after so many days overflows ??

Please always do an Auto Format (Tools > Auto Format in the Arduino IDE or Ctrl + B in the Arduino Web Editor) on your code before posting it. This will make it easier for you to spot bugs and make it easier for us to read.

Malc-C:

`````` void loop();
``````

You should never call loop(). The function will return to the loop when it exits.

Also, don’t call functions like this:

`````` void loop();
``````

you call functions like this:

`````` loop();
``````

Malc-C:

``````while  ( var9 >0 );                                                               //  THE WHILE BOOL CONDITION   IF VAR9 IS GREATER THAN 0 CONTINUE WITH THE LOOP
``````

Unless var9 is updated in an interrupt function, that will cause an endless loop when var9 > 0.

Hi,
If it is too big, then post it as an .ino attachment.

Thanks.. Tom...

why do you call loop() from within a sub-function? doesn't loop() call the sub-function, newoutput1()?

while ( var9 >0 );

does not call loop() because the ";" is the body of the while. loop() is always called.

if handling time is the problem, those variables should be unsigned long. can't tell given the partial code listing

TomGeorge:
Hi,
If it is too big, then post it as an .ino attachment.

Thanks… Tom…

I’m always concerned when posting a full listing in the public domain

Thanks for the offer of assistance.

With regards to the other questions as to why the loop commands were used - I don’t know. As mentioned I was being mentored and the chap is no longer here so can’t ask the questions

I’ve since removed them from each section and it still runs but still pulses an output (only slightly) when it should be off after the program has been running for less than a day.

Oh what the hell… I need help so here’s the file

Hopefully the comments help explain what is supposed to be happening. It all works as it should, ie the serial data gets sent to a PC application, which can then update the arduino Mega, the lights come on when they should, and the temperatures drop at night if set… its just this strange rouge pulse that starts after around 18 hours run time.

So for argument sake, I set the NOVIU variable to 4 for four outputs and upload the code. I can then set the target temperature for channel 1 to 15c. As this is below room temperature the monitoring LED on channel on goes out permanently as it should. But then at some point after running (around 18 hours in on this occasion) it will start to flicker with a very short pulse at the start of each cycle…

Now the code may well be full of poor code structure, or could be condensed in many ways so I accept that as a program it may not be the traditional way of structuring Arduino code… but please don’t slate me for that… sometimes duplicating a function over and over is easier to understand than having complex for next loops etc. - It all depends on how our brains are wired

8_Channel_Stat_v4_26_1_20.ino (302 KB)

loop() is called by the arduino main(). your loop() invokes newoutput1() which in turn invoked loop() which in turn ...

each time a function is called, at least a return address is put on the stack consuming memory. eventually memory runs out and the code "starts misbehaving".

Hi,
Thanks for the 8 channel code.
Have you got a copy of the original single channel code that worked satisfactorily?

Tom...

gcjr:
loop() is called by the arduino main(). your loop() invokes newoutput1() which in turn invoked loop() which in turn ...

each time a function is called, at least a return address is put on the stack consuming memory. eventually memory runs out and the code "starts misbehaving".

OK, thanks for the explanation... I sort of understand. I guess that's a throwback from the picBASIC, logic where you could step out of the main loop, gosub and then return....

My mentor suggested that things are more modular with Arduino and you basically bold together self contained subroutines... maybe that was where I was going wrong.

TomGeorge:
Hi,
Thanks for the 8 channel code.
Have you got a copy of the original single channel code that worked satisfactorily?

Tom…

Regretfully I don’t. When I said it worked, I was more referring to the fact the output pulsed at the frequency I was expecting. When we extrapolated this to 8 channels the code would do each channel sequentially which was no good as meant that it could be 10 seconds or more between each channel being pulsed. I wanted all outputs to come on, then each channel to remain on for the duration set by the maths, then when all channels had been processed loop back again.

It was then suggested that by using a variable the number of channels can be set to give more flexibility, so rather than have the unit hard coded for three vivariums, and then have to re-code for four, all that needed to be changed was a single variable. This is when I think the problem kicked in…

You declare this array

``````int dataIn[74];
``````

But then march past the end of the array...

``````  HR = (dataIn[72]);
MN = (dataIn[73]);
SC = (dataIn[74]);

rtc.setTime(SC, MN, HR, 5, 22, 2, 18);

NOVIU = (dataIn[75]);
//...
``````

An array of 74 elements can only be indexed from 0..73.

You also don't check properly in this code

``````void processNumber (const long n)
{
dataIn[i]  = n;                                           // dataIn is an array to set the data received from the PC application against the correct variables
i++;                                                      // increase i by one on each pass
if ( i >= 76)                                             // until i = 75 - which is the last variable / element in the array
{
serialFlag = 0;                                         // when i =76 set flag back to zero
i = 0;                                                  // and reset i back to zero
j = 1;
loopcount = 3222;                                       // stops the Serial While Loop, though better replaced by a timer
}
}
``````

You should never compare floating point numbers for equality.. They may not be equal since a computer can not represent every possible floating point number. It may be better to change all those to integers and keep track of temperature in tenths or hundredths of a degree.

``````if (temperature1 == -127.00)                                // check to see if sensor is readable
{
Serial.print("000");                                      // if not then send 000 to the PC for first channel
}
``````

As a bonus, you could probably remove 90% of your code if you learn how to use arrays and remove all the duplication.

Thanks for pointing out some of the problems with the code.

The array elements issue is my fault as I added a few more elements and obviously screwed up !

The floating point comparison seems to work, in that if I remove the sensor so -127 is returned the alarm sounds, the output goes low and the screen displays ERROR. So it seemed the logical way to do the comparison.

Oh I'm sure the code could be optimised considerably, but I'm still at the bottom of the learning curve, hence why the code is so heavily commented.

Thanks for the contribution and support. Have you any idea though as to how to resolve the issue with what I currently have to work with

in such cases there are often multiple problems. more minor problems often make it harder to find the more significant one. So it make sense to fix all the minor problems.

blh64:
As a bonus, you could probably remove 90% of your code if you learn how to use arrays and remove all the duplication.

So could I replace

``````float dutyCycle1;                                     // Used to obtain percentage of pulse
float dutyCycle2;
float dutyCycle3;
float dutyCycle4;
float dutyCycle5;
float dutyCycle6;
float dutyCycle7;
float dutyCycle8;
``````

With

[/code]
float dutyCycle = {1,2,3,4,5,6,7,8};
[/code]

And replace

``````byte var1 = 0;
byte var2 = 0;
byte var3 = 0;
byte var4 = 0;
byte var5 = 0;
byte var6 = 0;
byte var7 = 0;
byte var8 = 0;
byte var9 = 0;
``````

with

``````int var[9]=0
``````

Basically I'm looking at re-writing a bulk of the code, and trying to follow your suggestions

OK This compiles

I changed the code as follows:

``````float dutyCycle[8] = {1,2,3,4,5,6,7,8};

unsigned long heatpulse[8]= {1,2,3,4,5,6,7,8};
``````

``````for (int i; i<8; i++)
heatpulse[i] = (millis() + (frequency * dutyCycle[i]));    // current  millis() time  + On time in Milli seconds
``````

And the code compiled without errors. So I’ll run through the the rest of the code and repeat the changes in each section for now to test. I now I should be able to simplify the code more by using a for statement and the NOVIU variable so that it can be a single routine… but that will come later

OK I've been editing the code and I've run into a problem

I've set the array thus

``````byte setpoint[8] = {1,2,3,4,5,6,7,8};
``````

But I'm having issues setting the initial value I want for the 8 elements of the array. I want to set the value for each set point as 20.

``````byte setpoint[1] = 20;
``````

it results in an error message ' conflicting declaration 'byte setpoint [1]'

If I change it to

``````setpoint[1] = 20;
``````

I get this error message ''setpoint' does not name a type'

Sorted the error messages - moved the variables to the setup routine and that worked. The code compiles and I’ve uploaded the code to the mega… but I get no LEDs lighting up even though the temperatures are a few degrees less than the set points.

Main variables are now in arrays

``````//-----------------------------------------------------------------
int frequency;                                          // Frequency in milliseconds. Used to set the full pulse duration in milliseconds
float error[8] = {1, 2, 3, 4, 5, 6, 7, 8};
byte var[9] = {1, 2, 3, 4, 5, 6, 7, 8, 9};
byte varA[8] = {1, 2, 3, 4, 5, 6, 7, 8};
unsigned long heatpulse[8] = {1, 2, 3, 4, 5, 6, 7, 8};  // used to calculate dutycycle and control the pulse duration
float temperature[8] = {1, 2, 3, 4, 5, 6, 7, 8};;       // Used to store the real tempertaure based on the DS18B20
float daytemp[8] = {1, 2, 3, 4, 5, 6, 7, 8};
float AlarmHIGH[8] = {1, 2, 3, 4, 5, 6, 7, 8};
float AlarmLOW[8] = {1, 2, 3, 4, 5, 6, 7, 8};
float nighttemp[8] = {1, 2, 3, 4, 5, 6, 7, 8};
float dutyCycle[8] = {1, 2, 3, 4, 5, 6, 7, 8};
byte setpoint[8] = {1, 2, 3, 4, 5, 6, 7, 8} ;
//-----------------------------------------------------------------
``````

The set points are entered as

``````  setpoint[1] = 20;                                        // Used to display the target temperature
setpoint[2] = 20;
setpoint[3] = 20;
setpoint[4] = 20;
setpoint[5] = 20;
setpoint[6] = 20;
setpoint[7] = 20;
setpoint[8] = 20;

daytemp[1] = 25;                                  // Daytime temperature initial set temperature of 25c
daytemp[2] = 25;
daytemp[3] = 25;
daytemp[4] = 25;
daytemp[5] = 25;
daytemp[6] = 25;
daytemp[7] = 25;
daytemp[8] = 25;

etc etc....
``````

Pins are set to output

``````// Pin Dassignments (Mega2560)

int output[8] = {8, 9, 10, 11, 41, 40, 39, 38};      // Physical pins

pinMode(output[1], OUTPUT);                             // sets the digital pin as output
pinMode(output[2], OUTPUT);                             // sets the digital pin as output
pinMode(output[3], OUTPUT);                             // sets the digital pin as output
pinMode(output[4], OUTPUT);                             // sets the digital pin as output
pinMode(output[5], OUTPUT);                             // sets the digital pin as output
pinMode(output[6], OUTPUT);                             // sets the digital pin as output
pinMode(output[7], OUTPUT);                             // sets the digital pin as output
pinMode(output[8], OUTPUT);                             // sets the digital pin as output
``````

In the main loop , for now I’ve left the read sensor routine as is, followed by the calls to update the screen etc

``````  if (NOVIU == 1) {                                           // check to see how many channels have been set in the configuration
}

Screen();                                                   // Display data from sensors and RTC on the screen
tempdrop ();                                                // check for any night time temp drop

test ();

Lighting();                                                 // Test to see if the lights should be on
``````

The key one is test() which is the new routine to control the outputs

``````void test () {

for (int i; i < NOVIU; i++)

frequency = 5000;

error[i] = ((setpoint[i] + 0.5) - temperature[i]);                                // Obtain the value for the difference between desired tempertaure and actual temperature
if (error[i] > 1.0)                                                               // if the difference is more than a degree
dutyCycle[i] = 1.0;                                                             // then set the value of the duty cycle (pulse) to 100%
else if (error[i] < 0.0)                                                          // or if the value is negative and below zero
dutyCycle[i] = 0.0;                                                             // then set the value of the duty cycle to 0%
else                                                                              // or if the value is between 1.0 and 0.0
dutyCycle[i] = error[i];                                                        // set the value of the duty cycle to match that value

if (error[i] > 1.0) {                                                             // if the dutycycle value is 0
digitalWrite(output[i], HIGH);                                                  // set output 1 high
varA[i] = 1;                                                                    // and set var1A to 1

}
else {
digitalWrite(output[i], LOW);                                                   // set output 1 low
varA[i] = 0;                                                                    // and set var1A to 0 and continue through this routine
}

if (temperature[i] == -127.00)                                                    // if aa misread or incorrect response from DS18B20 produces a value of -127.00
{
dutyCycle[1] = 0;                                                               // set control to 0
digitalWrite(alarm, HIGH);                                                      // sound the buzzer to notify there is a problem
digitalWrite(output[i], LOW);                                                   // turn off the heater
}

else if ((temperature[i] >= AlarmHIGH[i]) || (temperature[i] <= AlarmLOW[i]))     // if the read tempertaure is outside the high and low thresholds
{
dutyCycle[1] = 0;                                                               // set control to zero
digitalWrite(alarm, HIGH);                                                      // and sound the buzzer to notify the user of a problem
digitalWrite(output[i], LOW);                                                   // turn off power to heater
}

heatpulse[i] = (millis() + (frequency * dutyCycle[i]));                           // current  millis() time  + On time( freq*duty) in Milli seconds

do    {                                                                           // The DO  / WHILE CONDITIONAL LOOP

if (heatpulse[i] > millis() && varA[i] == 0 ) {                                 // if Var1A is low, ie not running Full heating, then pulse ON, if needed.
digitalWrite(output[i], HIGH);                                                // if Var1A is High, full heating is ON , so - Skip
var[i] = 1;
}
else if (varA[i] == 0) {                                                        // if Var1A is low, ie not running Full heating, then pulse can be turned OFF
digitalWrite(output[i], LOW);                                                 // if Var1A is High  full heating is ON, so - Skip
var[i] = 0;
}

if (var[i] == 0)  {                                                             //  TEST IF ALL THE PULSES HAVE FINISHED, WHEN THEY HAVE VAR5 BECOMES ZERO
var[9] = 0;
}
else var[9] = 1 ;

}
while  ( var[9] > 0 );                                                            //  THE WHILE BOOL CONDITION   IF VAR9 IS GREATER THAN 0 CONTINUE WITH THE LOOP

}
``````

The screen is correctly displaying the set temperatures and the sensor temperatures so it would seem that the arrays are storing the right values. But there is no output…

Also with the for loop this would sequentially deal with each output which is not what I want. I need all outputs to go high at the same time, but then depending on the closeness each channel’s sensor is to the set point, will extinguish - thus the “pattern” of how the LEDs go out can be random as each channel will have a different differential between the set point and actual temperature each time the cycle is run

To get a better understanding of using arrays I’ve started with a fresh sketch… but something strange is happening with one of the variables.

``````#include "SPI.h"
#include <SparkFunDS1307RTC.h>
#include <OneWire.h>
#include <Wire.h>
#include <DallasTemperature.h>
#include <EEPROM.h>

#define TFT_RST 48                            // As defined below
#define TFT_DC 46
#define TFT_CS 49

byte setpoint[2] = {1, 2};
float temperature[2] = {1, 2};
float daytemp[2] = {1, 2,};
float dutyCycle[2] = {1, 2};
int output[8] = {8, 9, 10, 11, 41, 40, 39, 38};
float error[2] = {1, 2};
byte var[3] = {1, 2, 3};
byte varA[2] = {1, 2};
unsigned long heatpulse[2] = {1, 2};

byte  NOVIU = 2;
int frequency;
byte ind1 = 0;

int DSviv1 = 44;
int DSviv2 = 31;

void setup() {
for (int i; i < 8; i++)
{
pinMode(output[i], OUTPUT);
}
setpoint[1] = 25;
setpoint[2] = 25;

ind1 = 0;
tft.begin();
tft.fillScreen(ILI9341_BLACK);
uint8_t rotation = 1;
tft.setRotation(rotation = 1);

}

{
{
#define ONE_WIRE_BUS (DSviv1)
OneWire oneWire1(ONE_WIRE_BUS);
DallasTemperature sensors1(&oneWire1);
sensors1.begin();
sensors1.setResolution(11);
sensors1.requestTemperatures();
temperature[1] = sensors1.getTempCByIndex(0);
}
{
#define ONE_WIRE_BUS (DSviv2)
OneWire oneWire2(ONE_WIRE_BUS);
DallasTemperature sensors2(&oneWire2);
sensors2.begin();
sensors2.setResolution(11);
sensors2.requestTemperatures();
temperature[2] = sensors2.getTempCByIndex(0);
}
}

void Screen()
{
uint8_t rotation = 1;
tft.setRotation(rotation = 1);
tft.setCursor(0, 0);
tft.setTextColor(ILI9341_MAGENTA, ILI9341_BLACK); tft.setTextSize(2);
tft.println("      Octi-Therm V5.0");
tft.println();
tft.println();
tft.setTextColor(ILI9341_YELLOW, ILI9341_BLACK);    tft.setTextSize(2);
tft.println("  TEMP   SET     TEMP  SET");
tft.println();
if (NOVIU == 2 )
{
tft.setTextColor(ILI9341_MAGENTA, ILI9341_BLACK);
tft.print("1 " );
tft.setTextColor(ILI9341_GREEN, ILI9341_BLACK);
tft.print(temperature[1]);
tft.setTextColor(ILI9341_CYAN, ILI9341_BLACK);
tft.print("  " );
tft.print(setpoint[1]);
}

{ tft.setTextColor(ILI9341_MAGENTA, ILI9341_BLACK);
tft.print("    2 ");
tft.setTextColor(ILI9341_GREEN, ILI9341_BLACK);
tft.print(temperature[2]);
tft.setTextColor(ILI9341_CYAN, ILI9341_BLACK);
tft.print("  " );
tft.print(setpoint[2]);
}

tft.println();
tft.println();
tft.println(error[1]);
}

void loop() {
Screen();
//test ();
}

void test () {
frequency = 5000;
for (int i; i < NOVIU; i++)
{
error[i] = ((setpoint[i] + 0.5) - temperature[i]);                                // Obtain the value for the difference between desired tempertaure and actual temperature
if (error[i] > 1.0)                                                               // if the difference is more than a degree
dutyCycle[i] = 1.0;                                                             // then set the value of the duty cycle (pulse) to 100%
else if (error[i] < 0.0)                                                          // or if the value is negative and below zero
dutyCycle[i] = 0.0;                                                             // then set the value of the duty cycle to 0%
else                                                                              // or if the value is between 1.0 and 0.0
dutyCycle[i] = error[i];                                                        // set the value of the duty cycle to match that value

if (error[i] > 1.0) {                                                             // if the dutycycle value is 0
digitalWrite(output[i], HIGH);                                                  // set output 1 high
varA[i] = 1;                                                                    // and set var1A to 1

}
else {
digitalWrite(output[i], LOW);                                                   // set output 1 low
varA[i] = 0;                                                                    // and set var1A to 0 and continue through this routine
}

heatpulse[i] = (millis() + (frequency * dutyCycle[i]));                           // current  millis() time  + On time( freq*duty) in Milli seconds

do    {                                                                           // The DO  / WHILE CONDITIONAL LOOP

if (heatpulse[i] > millis() && varA[i] == 0 ) {                                 // if Var1A is low, ie not running Full heating, then pulse ON, if needed.
digitalWrite(output[i], HIGH);                                                // if Var1A is High, full heating is ON , so - Skip
var[i] = 1;
}
else if (varA[i] == 0) {                                                        // if Var1A is low, ie not running Full heating, then pulse can be turned OFF
digitalWrite(output[i], LOW);                                                 // if Var1A is High  full heating is ON, so - Skip
var[i] = 0;
}

if (var[i] == 0)  {                                                             //  TEST IF ALL THE PULSES HAVE FINISHED, WHEN THEY HAVE VAR5 BECOMES ZERO
var[9] = 0;
}
else var[9] = 1 ;

}
while  ( var[9] > 0 );                                                            //  THE WHILE BOOL CONDITION   IF VAR9 IS GREATER THAN 0 CONTINUE WITH THE LOOP
}
}
``````

If I comment out the test routine the set point of 25 is correctly displayed for both setpoint[1] and setpoint[2]

However if I uncomment the line in the loop the value for setpoint[1] is shown as zero and 191 for setpoint[2]

Can’t see why the values are changed so drastically…

You index into arrays based on 0 so your setpoint[2] means you can access index 0 and index 1. Index 2 is beyond the end of the array and stopping on memory is should not.

``````byte setpoint[2];

void setup() {
setpoint[0] = 42;
setpoint[1] = 12;
// ...
}
``````

Malc-C:
Sorted the error messages - moved the variables to the setup routine and that worked.

If you declare your variables BEFORE setup, they will be global variables, accessible from anywhere in your code.
Tom...