next topic »

[Reply #30 on:]

Well, ok, but is this an actual problem causing it to not work at this point?

It isn't that I don't mind changing it, but just now I am trying to get my head around what is wrong FUNCTIONALLY with the code.  Not so much the cosmetics of it.

I am trying to see the structure of the code.  Putting specific names confuses me with how I see the patterns.

[Reply #31 on:]

But don't you see that what you call "cosmetics" is actually obfuscation, making it harder for you and others to discern the functionality?

If someone makes a local copy of a parameter within a function, my immediate thought is "OK, they're going to change the value of the original, but need to save it for some reason for later", and when I see that that DOESN'T happen, I wonder about the intent, and whether there's some deeper thing I'm missing, which I then waste time looking for.

Besides, it makes your code longer than it needs to be, and longer code allows dusty corners to accumulate, and bugs love dusty corners.

[Reply #32 on:]

So how is this for the "latest" code version:

Code:

/*
    Automamtic plant pot watering system.
   
    Concept:
    Daily or weekly it waters plant pots.
    Sensors detect if the saucer under the pot is
    full or not, and stop the pump when it detects
    the water depth.
    There is also a "timeout" period per pot incase
    there is a problem with filling the pot.
   
*/

#include <arduino.h>
#include <Wire.h>
#include <stdio.h>
#include "DS1307_1.h"
#include "alarm_clock.h"


//  Array for each pot's fill time
int sol_run_time[5];
//  Array for the values for the system to look for to indicate "full"
int level[6];
//  Solenoid array
int solenoid[5];
//  Sensor array
int sensor[6];


//  These are the sensors for each pot and the main reserve
const byte sensorPins [6] = {14, 15, 16, 17, 18, 19};


/*
//  These are the sensors for each pot and the main reserve
#define sensorPins[0] 14
#define sensorPins[1] 15
#define sensorPins[2] 16
#define sensorPins[3] 17
#define sensorPins[4] 18
#define sensorPins[5] 19
*/


//  These are the pins to drive each solenoid for each pot.
//  They are OR'd together to turn on the motor/pump.
//int solenoid[5];
#define solenoidPins[1] 0
#define solenoidPins[2] 1
#define solenoidPins[3] 2
#define solenoidPins[4] 3
#define solenoidPins[5] 4



#define LED 5
#define btn_1 6

//  Set time for daily run.
//int run_time_dow 2;
int run_time_hr = 12;
int run_time_mn = 00;
//int run_time_sec = 00;

// RTC stuff
int rtc[7];
DS1307 RTC=DS1307();             // Create RTC object


//#define setRTC
#ifdef setRTC
// Set/init RTC
  RTC.stop();
  RTC.set(DS1307_SEC,0);
  RTC.set(DS1307_MIN,50);
  RTC.set(DS1307_HR,0);
  RTC.set(DS1307_DOW,3);           // value from 1 to 7. User define whether 1 is sun or mon.
  RTC.set(DS1307_DATE,10);
  RTC.set(DS1307_MTH,06);
  RTC.set(DS1307_YR,12);
  RTC.start();
#endif


void setup()
{
    //
    pinMode(sensorPins[0],INPUT);
    pinMode(sensorPins[1],INPUT);
    pinMode(sensor[2],INPUT);
    pinMode(sensor[3],INPUT);
    pinMode(sensor[4],INPUT);
    pinMode(sensor[5],INPUT);

    pinMode(btn_1,INPUT);   
   
    pinMode(solenoid[1],OUTPUT);
    pinMode(solenoid[2],OUTPUT);
    pinMode(solenoid[3],OUTPUT);
    pinMode(solenoid[4],OUTPUT);
    pinMode(solenoid[5],OUTPUT);

    pinMode(LED,OUTPUT);
   
    digitalWrite(LED,LOW);
    digitalWrite(solenoid[1],LOW);
    digitalWrite(solenoid[2],LOW);
    digitalWrite(solenoid[3],LOW);
    digitalWrite(solenoid[4],LOW);
    digitalWrite(solenoid[5],LOW);

    // Time in seconds each pot takes to be "filled".
    //  All values to be set.
    sol_run_time[1] = 20;
    sol_run_time[2] = 20;
    sol_run_time[3] = 20;
    sol_run_time[4] = 20;
    sol_run_time[5] = 20;
   
    //  All values to be set.
    level[1] = 20;
    level[2] = 20;
    level[3] = 20;
    level[4] = 20;
    level[5] = 20;
    level[6] = 20;      //  This will be the main reserve, and when it is empty rather than full.

}

void loop()
{
    //  Get time from RTC.
    int rtc[7];
    RTC.get(rtc,true);
    int flag;
    //  Have time, check what time it is.
    if (btn_1 == HIGH)
    {
        Solenoid();
    }
    //
    if (rtc[2] == run_time_hr)
    {
        if (rtc[1] == run_time_mn)
        {
            if (flag == 0)
            {
                //
                Solenoid();
                flag = 1;
            }
        }
    }
   
}

//*************************************************************************************************

void Solenoid()
{
    int solenoid_selector;
    digitalWrite(0,HIGH);
    delay(20000);


    int solenoid_selector;
    for (solenoid_selector=0;i<5;solenoid_selector++)
    {
        //  Get time value for i
        //  Turn on soldenoid i
        //  Wait the specified time
        //  Check sensor for input
        //  Abort if active
        //  Turn off solenoid i
        digitalWrite(solenoid[solenoid_selector],HIGH);
        sensor_delay(solenoid_selector);
    }


}

//*************************************************************************************************

int sensor_delay(int x)
{
    //  Read threashold values for all sensors.
    //  i is current one, but Main gets read every time anyway.
    int active_solenoid_number = x;
    int reading;
    int TTG;
    int time_up;
    TTG = sol_run_time[i];
    time_up = timer(TTG);
    if (time_up > 0)
    {
        reading = digitalRead(sensor[0]);
        if (reading > level[6])      //  Main reserve is good.
        {
            reading = digitalRead(sensor[active_solenoid_number]);
            if (reading > level[active_solenoid_number])
            {
                //  Water level reached.  Stop pumping and go to next plant.
                time_up = 0;
                digitalWrite(solenoid[active_solenoid_number],LOW);
            }
            //  This is if the plant's water is already good
        }
        //  This is if the main level is not good.
    }
    // time_up = 0
}

   
//*************************************************************************************************
   
int timer(int foo)
{
    static int counter;
    counter = foo;
    int rtc[7];
    int min_flag;
    RTC.get(rtc,true);
        if (counter >0)
        {
           if (min_flag != rtc[1])
           {
               min_flag = rtc[1];
               counter = counter - 1;
           }
        }
    return counter;
}

//*************************************************************************************************

[Reply #33 on:]

So how is this for the "latest" code version:

Code:

#define sensorPins[0] 14

You tell us.

Code:

pinMode(sensorPins[0],INPUT);
    pinMode(sensorPins[1],INPUT);
    pinMode(sensor[2],INPUT);
    pinMode(sensor[3],INPUT);
    pinMode(sensor[4],INPUT);
    pinMode(sensor[5],INPUT);

and

Code:

pinMode(solenoid[5],OUTPUT);

Enthusiasm is great.
I really like enthusiasm.

But when it comes to programming, I prefer attention to detail

[Reply #34 on:]

So how is this for the "latest" code version:

Did you try compiling it?

[Reply #35 on:]

No, I don't have the time.

[Reply #36 on:]

I was referring to the OP.

Code:

void Solenoid()
{
    int solenoid_selector;
...
    int solenoid_selector;
    for (solenoid_selector=0;i<5;solenoid_selector++)
  ...
    }


}

[Reply #37 on:]

I found a few places where I goofed and missed a couple of "i" for the new name.

It now complies.

But I still don't really know if it does what I want it to do.

Now it complies without error.

Though again I still don't know if it is really doing what I want.

[Reply #38 on:]

I am really not taking this well.   Alas I seem to be floundering in mistakes.

It would be easier I think if someone was here with me and I could "talk to them".

Seems that is the way I learn better than what I am doing.

[Reply #39 on:]

Code:

int timer(int foo)
{
    static int counter;
    counter = foo;

Kinda defeats the point of making it static, don't you think?

[Reply #40 on:]

Code:

int timer(int foo)
{
    static int counter;
    counter = foo;

Kinda defeats the point of making it static, don't you think?

Why?

The routine is called.  "counter" is static - as in it keeps its value.
It gets the RTC minutes, and counts down in minutes until zero.
If it returns a greater value the calling routine other things happen, but when the counter gets to zero, it signals to the calling routine/function to abort.

[Reply #41 on:]

Code:

#define sensorPins[0] 14

I shudder every time I see you leaving those macros in. Please go in there right now and just delete every line that starts with "#define" and has square brackets in it. You absolutely don't want any of those.

Please decide whether you're going to use sensor or sensorPins to hold the sensor pin numbers. Delete whichever one you aren't going to use, and correct all references to use the one you're sticking with. Then do the same with solenoid and solenoidPins.

Anywhere you find yourself repeating code to do the same thing to all the entries in an array, such as your calls to pinMode(), replace that with a loop through the array.

I recommend that you add either a #define or a const int to record the size of each array and use that in your FOR loops that walk through the array, instead of hard-coding the size of the arrays everywhere.

[Reply #42 on:]

The routine is called.  "counter" is static - as in it keeps its value.

No, it doesn't - you've just assigned it the value of parameter you supplied, so the "static" qualifier is pointless.
What does "min_flag" do, and where is it initialised (hint)?

Code:

level[6] = 20;

What I said earlier about attention to detail - I really meant it.

Playing 20 Questions Programming isn't the way to learn - try some of the supplied examples in the IDE, then try modifying them, adding functions, or extra hardware.

[Reply #43 on:]

I shudder every time I see you leaving those macros in.

To be fair, those particular ones are now inside a comment (but should be removed from the sketch), and the solenoidPins ones aren't used (hint, OP)

[Reply #44 on:]

Code:

int timer(int foo)
{
    static int counter;
    counter = foo;

Kinda defeats the point of making it static, don't you think?

Why?

The routine is called.  "counter" is static - as in it keeps its value.

Since you're declaring it within the function, it is only visible to the code within that function. And every time you call that function, you immediately overwrite that static variable with the argument foo, overwriting whatever the previous value was. So you are never actually making use of the previous value, hence there's no point in it being static. If you had a reason for making it static then this code is probably not doing what you intended.