1

i make a 3 pump for output and im using millis for it, i want to stop if rach the intervalm now in my code its still looping not realy stop for it thx

int pump1 = 5;//pin D5
int pump2 = 6;//pin D6
int pump3 = 7;//pin D7
float pompa1;
float pompa2;
float pompa3;
unsigned long prev_pump1    = 0;
unsigned long prev_pump2    = 0;
unsigned long prev_pump3    = 0;
unsigned long initiate_start = 0;
int pump_state1 = false;
int pump_state2 = false;
int pump_state3 = false;

and after this this is my loop program

  //-----------PUMP ALGORITHM--------------------
  long int pompa01 = pompa1*1000;//interval pump1
  long int pompa02 = pompa2*1000;//interval pump2
  long int pompa03 = pompa3*1000;//interval pump2

  unsigned long initiate_start = millis();

  //pompa1
  if(intiate_start-prev_pump1 > pompa01)
  {
    pump_state1 = !pump_state1;
    
    if(pump_state1)
    {
      digitalWrite(pump1, HIGH);
    }
    else
    {
      digitalWrite(pump1, LOW);
    }
    prev_pump1 = initiate_start;
  }
  
    //pompa2
  if(intiate_start-prev_pump2 > pompa02)
  {
    pump_state2 = !pump_state2;
    
    if(pump_state2)
    {
      digitalWrite(pump2, HIGH);
    }
    else
    {
      digitalWrite(pump2, LOW);
    }
    prev_pump2 = initiate_start;
  }

    //pompa3
  if(intiate_start-prev_pump3 > pompa03)
  {
    pump_state3 = !pump_state3;
    
    if(pump_state3)
    {
      digitalWrite(pump3, HIGH);
    }
    else
    {
      digitalWrite(pump3, LOW);
    }
    prev_pump3 = initiate_start;
  }
2
  • 1
    Please, copy-edit your question: make complete sentences, and pay attention to grammar and punctuation. As it stands, it is really hard to decipher. Dec 8 '20 at 9:11
  • 1
    Side note: The variable names initiate_start and intiate_start are too similar, which makes the code confusing. Dec 8 '20 at 9:18
1

This is meant as a complement to Michel Keijzers' answer. I fully agree with his assessment: code duplication is something to be avoided, as it greatly hurts maintainability. And I sympathise with him feeling the urge to implement this as a proper class. ;-)

I would argue, however, that between the simplistic version of the OP's code, and the fully object-oriented one of that answer, there is an intermediate step that is worth targeting as the next thing to learn. I mean: arrays and loops.

Many cases of code duplication follow a pattern roughly like this:

// At the top of the sketch:
some_type data0 = some_value;
some_type data1 = some_other_value;
// etc.

// Within a function:
do_lots_of_stuff(data0);
do_lots_of_stuff(data1);
// etc.

where do_lots_of_stuff() may be a lot of code, rather than a single function call (and it would indeed not be that bad if it was just a function call).

The simple solution to this kind of code duplication is to put the variables in an array, and process them by looping through the array:

// At the top of the sketch:
const int data_count = ...;
some_type[data_count] = {some_value, some_other_value, ...};

// Within a function:
for (int i = 0; i < data_count; i++)
    do_lots_of_stuff(data[i]);

Here is my attempt at applying this technique to the OP's code. Note that, since there are multiple variables for each pump, we need multiple arrays:

const int pump_count = 3;
const uint8_t pump_pins[pump_count] = {5, 6, 7};
const uint32_t pump_half_periods[pump_count] = {1000, 2500, 3200};

uint8_t pump_states[pump_count];  // LOW or HIGH
uint32_t pump_last_actuations[pump_count];  // time in ms

void setup() {
    for (int i = 0; i < pump_count; i++) {
        pinMode(pump_pins[i], OUTPUT);
    }
}

void loop() {
    unsigned long now = millis();
    for (int i = 0; i < pump_count; i++) {
        if (now - pump_last_actuations[i] >= pump_half_periods[i]) {
            pump_states[i] = (pump_states[i] == HIGH) ? LOW : HIGH;
            digitalWrite(pump_pins[i], pump_states[i]);
            pump_last_actuations[i] = now;
        }
    }
}

This removes all the duplication, and should be easier to maintain and debug.

Once one is comfortable with this concept, I would argue the next step in terms of building abstractions would be to bundle together all the data relative to one pump into a struct. Then we would have a single array (the elements of which are structs) instead of a collection of arrays:

// Data structure describing a pump.
struct Pump {
    const uint8_t pin;
    const uint32_t half_period;  // half period in ms
    uint8_t state;               // LOW or HIGH
    uint32_t last_actuation;     // time in ms
};

const int pump_count = 3;

Pump pumps[pump_count] = {
// pin, half-T, state, last_actuation
    {5, 1000, LOW, 0},
    {6, 2500, LOW, 0},
    {7, 3200, LOW, 0}
};

The body of setup() and loop() would be very similar to the previous version, but with pump_pins[i] replaced by pumps[i].pin and so on.

The third step in the abstraction ladder would be to embed the code that handles the data structure with the data itself in a class, as shown in Michel Keijzers' answer. With the proper methods defined, the main sketch could be reduced to something like this:

#include "Pump.h"

Pump pumps[] = {
    Pump(5, 1000),
    Pump(6, 2500),
    Pump(7, 3200)
};

void setup() {
    for (Pump &pump : pumps)
        pump.begin();
}

void loop() {
    for (Pump &pump : pumps)
        pump.process();
}

I tried all these methods, and it turns out the first one (with a collection of arrays) is the one that uses the least lines of code, and the last one (with a class) uses the most. One could wonder then why use more advanced abstractions, if they increase the amount of code one has to type. The reason is, these abstractions become useful when the program grows in complexity. On can put together all the code that deals with the pumps in one place (maybe in a separate file), all the code that deals with sensors in another place... and the main sketch that puts all the pieces together remains simple.

7
  • what is a "half period in ms" ? is it a state time or what
    – daffa faiz
    Dec 13 '20 at 17:26
  • @daffafaiz: The “period” is the time it takes the signal to repeat itself. The “half period” is half the period. “ms” is the symbol for “milliseconds”, a unit of time. Dec 13 '20 at 17:45
  • thx for that sir
    – daffa faiz
    Dec 13 '20 at 21:00
  • and sir how if i erase the const it is able to edit isnt ?
    – daffa faiz
    Dec 13 '20 at 23:58
  • @daffafaiz: Excuse me? Can you make a full sentence? Dec 14 '20 at 7:56
0

Note: this is not an answer (my previous version of the answer contained a problem, thanks Edgar Bonet for notifying and a fix for the remainder of the answer). However, I don't want to remove this answer as it may help in removing a lot of duplication. I took the liberty to convert it into a class.

It compiles, but I can't check the functionality but hopefully it helps you to continue. If there are problems, use write commands to send text to the serial to check the times and state.

Sketch:

#include  "Pump.h"

const int NR_OF_PUMPS = 3;

Pump pumps[NR_OF_PUMPS] = 
{
  Pump(5, 1.0),
  Pump(6, 2.5),
  Pump(7, 3.2) 
};

void setup() 
{
}

void loop() 
{
  process();
}

void process()
{
  for (int pumpIndex = 0; pumpIndex < NR_OF_PUMPS; pumpIndex++)
  {
    pumps[pumpIndex].Process(millis());
  }
}

Pump.h:

class Pump
{
public:
  Pump::Pump(int pin, float interval);
  void Process(uint32_t currentTime);

private:
  int _pin;
  float _interval;
  uint32_t _prevTime;
  bool _state;
};

Pump.cpp:

#include "Arduino.h"
#include "Pump.h"

Pump::Pump(int pin, float interval)
:
  _pin(pin),
  _interval(interval * 1000.0),
  _prevTime(0),
  _state(false)
{
}
    
void Pump::Process(uint32_t currentTime)
{
  if (currentTime - _prevTime > _interval)
  {
    _state = !_state;
    digitalWrite(_pin, _state ? HIGH : LOW);
    _prevTime = currentTime;
  }
}
9
  • 1
    Re “seems that the current time is always put into the prev variables”: the update of the prev variables is within the if condition that controls the actuation of the pump, which is fine. BTW, you forgot to update _prevTime in the example you provide. Dec 8 '20 at 10:49
  • @EdgarBonet Good catch, thanks for the improvements. Dec 8 '20 at 10:55
  • The prev_pump3 = initiate_star make the loop forever ? Or what ?
    – daffa faiz
    Dec 8 '20 at 12:02
  • 1
    Re “_prevTime(millis())”: global constructors are called before the initialization of the Arduino core. I would avoid calling core functions at this time, even though millis() may be harmless. It is safe to initialize _prevTime to zero, which is the starting value of millis() anyway. Dec 8 '20 at 13:58
  • 1
    Okaaay let me add some println for this thx for helpin me
    – daffa faiz
    Dec 8 '20 at 15:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.