Solenoid action at max,min diff

Hello,

I am taking readings from a pressure sensor and writing it to sd card. As the sensor readings are being read by the micro controller, I am calculating the max and min values and their difference.If their difference is above 20, am controlling a solenoid by asking it to go HIGH for 0.5 sec and if it is less than 20, it should be low.
The micro controller am using is promini 3.3V,8 Mhz.
The solenoid can open/close within 100ms.
Data is being taken from the sensor at 55ms .(time between 2 data points is 55ms)

The max and min values, and their difference are being calculated correctly. The solenoid acts correctly (opens at diff>20 for 0.5 sec) sometimes. But sometimes, it does not open even when the diff>20 and does not close even when diff<20.

arduinoforum-pic.PNG

I see you are having the common problem of mixing floating and integer numbers in your computations.

I see this: temperature = (temperature_data * 0.0977) - 50; Try this: temperature = (temperature_data * 0.0977) - 50.0;.

All these:

float pressure, temperature;
float pressure_mmhg;

float Pdelta;
static bool foundMax = false;
static bool foundMin = false;
static bool solstate = true;
static float max=0;
static float min=0;

const long interval = 500;

Need to go before "setup".

Paul

It's pretty unlikely, but I do note that you continue to increment even after you have taken that first count. This means that it will eventually roll over to 0 again … but I'm not sure how this might be causing your bug.

I'm a little stumped. My next step would be to look at the actual numbers and to track the internal logic of the program, because what's probably going on is that something weird happens when the data goes up - up - the same - down - the same - up again, or something like that.

AH HA!!!!!

What might be causing this is when there's a maximum or minimum in the data whose 'peak' is more than one sample long at the same detected value. Your code simply won't "see" it, and the normal business that you always see maxima and minima in turn won't apply.

Consider this sequence of values:

100
99
100 // minimum of 99 gets locked in
100
5 // code does not see this maximum, because sendata[0] == sendata[1]
100 // code does not see the minumum of 5, because the minimum of 99 is locked in
90 // code sees the maximum of 100, but it is within tolerance of the previous minimum 99

I suggest you throw away samples that are identical to the previous sample. That means that you don't shift the senval values down. Careful! You still need to close the solenoid on time, so the code that does that has to be outside the 'ignore duplicates' if().

  static boolean no_data_yet = true;

  if (no_data_yet)
  {
    senval[2] = pressure_mmhg; // changed to 2 because reasons
    no_data_yet = false;
  }
  else
  {
    if(pressure_mmhg != senval[2]) {
      senval[0] = senval[1];
      senval[1] = senval[2];
      senval[2] = pressure_mmhg;

      if (!foundMax  && (senval[1] > senval[0]) && (senval[1] > senval[2]))
      {
        foundMax = true;
        max = senval[1];
        Serial.print("max  ");
        Serial.println(max);
      }
      else if (!foundMin && (senval[1] < senval[0]) && (senval[1] < senval[2]))
      {
        foundMin = true;
        min = senval[1];
        Serial.print("min  ");
        Serial.println(min);
      }

      if (foundMax && foundMin)
      {
        foundMax = false;//enable finding of next max
        foundMin = false;   

        Pdelta = max - min;
        Serial.print("Pdelta  ");
        Serial.println(Pdelta);
        if ( Pdelta >= 20)
        {
          digitalWrite(4, HIGH);
          startTime = millis();
          solstate = true;
        }
      }
    }
  }

  //keep solenoid triggered for period of time 500ms
  if (solstate == true && millis() - startTime >= interval)
  {
    digitalWrite(4, LOW);
    solstate = false;
  }

Oh - there's another problem

I'd also suggest that if you find a min, THEN fnd a max, throw away the min value but don't throw away the max. And likewise the other way around. I suggest this because weird-shaped oscillations might cause things that should be caught to be dropped.

Consider this sequence of values

1
5
4 // max of 5 detected
5 // min of 4 detected - solenoid not triggered. The max and min values are thrown away
100
99 // max of 100 detected, but the jump between min of 4 and max of 100 is not

To fix, keep a boolean variable named "the_most_recent_event_i_detected_was_a_max" and after the solenoid decision throw away the other value.

It's a tricky old thing, algorithms.

I think that @PaulMurrayCbr has come up with a good point, in that the max min finding algorithm only works when the middle of the three values is the max or min, and there may be anomalies with repeats or jittery, alternating data points.

He also suggested detailed print outs of the numerical which might pin point the problem. I agree with that, as the graphical data is confusing and you can't really associate specific max/min pairs with the solenoid anomalies.

Because the code is not blocking and you still look at data during the 500ms solenoid timing period, you might find another min max pair which resets the start time and the solenoid period is extended.

In an earlier thread the possibility of data smoothing was brought up. Why don't you try applying the three point max min algorithm to data which is derived from more than one reading.

I suggest you throw away samples that are identical to the previous sample. That means that you don't shift the senval values down. Careful! You still need to close the solenoid on time, so the code that does that has to be outside the 'ignore duplicates' if().

Thank you for the suggestion and all the help. I implemented the code as you suggested(without the count) and have attached the data and pictures.

I'd also suggest that if you find a min, THEN fnd a max, throw away the min value but don't throw away the max. And likewise the other way around. I suggest this because weird-shaped oscillations might cause things that should be caught to be dropped.

Sorry, I did not understand this.Could you please explain this.

He also suggested detailed print outs of the numerical which might pin point the problem. I agree with that, as the graphical data is confusing and you can't really associate specific max/min pairs with the solenoid anomalies.

I have attached the data and the picture for it. I zoomed the portion in black circles and highlighted it on the other picture.

Because the code is not blocking and you still look at data during the 500ms solenoid timing period, you might find another min max pair which resets the start time and the solenoid period is extended.

How do I close the solenoid as soon as the 500ms period is over.I want the solenoid to be open for 500 ms and the close(even if more peaks are detected during that interval,the solenoid can be closed)
I want to close the solenoid after 500 ms of staying open.
After the 500 ms, the solenoid can look for the next peak and then open for another 500ms.
As you mentioned, it is staying open for an extended period of time because an other max.min pair is found.

In an earlier thread the possibility of data smoothing was brought up. Why don't you try applying the three point max min algorithm to data which is derived from more than one reading.

Sorry, I did not understand what you meant by" data which is derived from more than one reading"

sol value,pmax,pmin.txt (10.5 KB)

arduinoforum-3.PNG

arduinoforum-4.PNG

How do I close the solenoid as soon as the 500ms period is over.I want the solenoid to be open for 500 ms and the close(even if more peaks are detected during that interval,the solenoid can be closed)
I want to close the solenoid after 500 ms of staying open.
After the 500 ms, the solenoid can look for the next peak and then open for another 500ms.
As you mentioned, it is staying open for an extended period of time because an other max.min pair is found.

I think the control logic change for this is very simple. Change the initialization of solState to false, and try this modification

//if (foundMax == true && foundMin == true)
if (solState == false && foundMax == true && foundMin == true)

If this does not do what you want, the control can be moved up into the min/max testing routines.

Test this, and I'll think about a smoothing routine.

I would tend not to use cattledog's suggestion - it does violence to your peak-detecting logic, which is already working just fine. Instead, I would deal directly with the fact that you want your solenoid now to have three states, not simply a 'on/off', and it is no longer a boolean.

I deal with this by separating the solenoid timing stuff into a separate object, or at least a separate set of functions. It works pretty much the same as what you already have, there's just a clearer demarcation of responsibilities.

class SolenoidDriver {
    // when I need one pin inside a class, I just name it 'pin'.
    // because its encapsulated in a class, I don't need to worry about name collisions

    const byte pin;

    // I always suffix variables that hold a value measured in some unit
    // this helps stop my Mars probes crashing
    const unsigned long activeInterval_ms;
    const unsigned long restInterval_ms;
    unsigned long startInterval_ms;

    enum State { IDLE, ACTIVE, RESTING } state;

  public:
    SolenoidDriver(const byte pin,
                   const unsigned long activeInterval_ms,
                   const unsigned long restInterval_ms) :
      pin(pin), // initialising the constants
      activeInterval_ms(activeInterval_ms),
      restInterval_ms(restInterval_ms)
    {
      // this could also be done in the initializer section above, but I prefer to 
     // initialize things that aren't constant in the body of the constructor.

      state = IDLE;
    }

    void setup() {
      // this cannot be done in the constructor, because the constructor gets
      // executed before the chip calls setup(), and things might be in a state 
      // not ready for pinMode calls yet
      pinMode(pin, OUTPUT);
    }

    void loop() {
      switch (state) {
        case IDLE: break;

        case ACTIVE:
          if (millis() - startInterval_ms >= activeInterval_ms) {
            digitalWrite(pin, LOW);
            state = RESTING;
          }
          break;

        case RESTING:
          if (millis() - startInterval_ms >= activeInterval_ms + restInterval_ms) {
            state = IDLE;
          }
          break;

      }
    }

    void activate() {
      // this is the line that does what you actually want
      if (state != IDLE) return;

      digitalWrite(pin, HIGH);
      startInterval_ms = millis();
      state = ACTIVE;
    }
};


// pin 4, active for 500ms, resting for 500 ms
SolenoidDriver solenoid(4, 500, 500);


void setup() {
  solenoid.setup();
}

void loop() {
  solenoid.loop();

 //////////// the rest of the stuff in your existing loop goes here /////////

  if (we_need_to_activate_the_solenoid_now /* SAMPLE CODE */ ) {
    solenoid.activate();
  }
}

Classes are just a away of reorganising code that you already have. You would

  1. take the SolenoidDriver class and the solenoid instance from the code above and add it to your sketch
  2. make a call to the setup and loop methods of that instace in your main loop. Note that the call to solenoid.loop() is made unconditionally. I usually put those things right at the top of my loop().
  3. remove the solenoid timing code from your loop, and just make a call to solenoid.activate() when you detect a big jump.

Doing this sot of untangles things. As with all "coding style" type things, it can be overdone.

This clasas cold be coded up differently - it could be coded up to count the calls to activate() and queue them up. But that's not what you are after as far as I understand it.

-- EDIT --

The business of calling the loop() unconditionally kinda-sorta spins off the job of managing the solenoid into a subtask that's almost like having a separate thread of execution, if you care to think of it that way. Of course, we don't have real threads on the arduino so it's actually rather simpler to code. But mentally, when coding the main loop logic you can think of the solenoid on-rest-ready sequence as this thing that happens in the backgound that you no longer need to worry about. Classes help manage complexity.

See the link in my sig for a longer example of using classes on Arduino.

dpoornima:
Sorry, I did not understand this.Could you please explain this.

     // ** this is a LOCAL variable **
     boolean foundMinmaxNow = false;

     if (!foundMax  && (senval[1] > senval[0]) && (senval[1] > senval[2]))
      {
        foundMax = true;
        foundMinmaxNow = foundMin;
        foundMin = false;
        max = senval[1];
        Serial.print("max  ");
        Serial.println(max);
      }
      else if (!foundMin && (senval[1] < senval[0]) && (senval[1] < senval[2]))
      {
        foundMin = true;
        foundMinmaxNow = foundMax;
        foundMax = false;
        min = senval[1];
        Serial.print("min  ");
        Serial.println(min);
      }

      if (foundMinmaxNow)
      {
        Pdelta = max - min;
        Serial.print("Pdelta  ");
        Serial.println(Pdelta);
        if ( Pdelta >= 20)
        {
          digitalWrite(4, HIGH);
          startTime = millis();
          solstate = true;
        }
      }

Note that foundMax and foundMin are always equal to one-another's negation. So you don't need two variables.

// ** this is a GLOBAL variable **
boolean lookingForMax;

// …

     // ** this is a LOCAL variable **
     boolean foundMinmaxNow = false;

     if (lookingForMax  && (senval[1] > senval[0]) && (senval[1] > senval[2]))
      {
        foundMinmaxNow = true;
        lookingForMax = false;
        max = senval[1];
        Serial.print("max  ");
        Serial.println(max);
      }
      else if (!lookingForMax && (senval[1] < senval[0]) && (senval[1] < senval[2]))
      {
        foundMinmaxNow = true;
        lookingForMax = true;
        min = senval[1];
        Serial.print("min  ");
        Serial.println(min);
      }

      if (foundMinmaxNow)
      {
        Pdelta = max - min;
        Serial.print("Pdelta  ");
        Serial.println(Pdelta);
        if ( Pdelta >= 20)
        {
          digitalWrite(4, HIGH);
          startTime = millis();
          solstate = true;
        }
      }

Here's my take at smoothing the readings by making each reading be the average of two readings. I have functionalized the reading of the sensor and call it twice. There are probably other ways to do this, but I tried to have minimal impact of the existing program.

Temperature is now a global variable. The value for the program will come from the second reading. I'm not sure of the exact timing of the two readings, and given the existing delay() of 20ms in the routine you may or may not want to add some additional delay between the readings. I commented out the continuing increase of count since it will overflow and is not used.

#include <SPI.h>
#include <SD.h>
#define FILE_BASE_NAME "Data"
#include <Wire.h>
#include <RTClib.h>
#include <float.h>
#define DS3231_Address 0x68
#define HSCDRRN400MD2A3_I2C 0x28
#define OUTPUT_MIN 1638.4
#define OUTPUT_MAX 14745.6
#define PRESSURE_MIN -400
#define PRESSURE_MAX +400
unsigned long time;
RTC_DS3231 rtc;
int chipSelect = 10;
File file;
int BASE_NAME_SIZE = sizeof(FILE_BASE_NAME) - 1;
char filename[] = FILE_BASE_NAME "00.csv";

//added button library, variables and button function
#include <Bounce2.h>
Bounce button = Bounce(); //instantiate a Bounce object

const byte LedPin = 9;
const byte ButtonPin = 7;
bool logFlag = true;//default status to enable SD writes in Loop

int solenoidPin = 4;
int count = 0;
float senval[3] = {0, 0, 0};
float temperature;

void buttonCheck()//debounced function to check if button pressed
{
  button.update();
  if (button.fell())
  {
    if (logFlag == true)
    {
      logFlag = false;
      Serial.println();
      Serial.println("Data Logging in Loop Disabled");
      Serial.println("Press Reset to Enable");
      Serial.println();
    }
  }
}

float getPressure()
{
  float pressure; 
  float pressure_mmhg;

  //send a request
  Wire.beginTransmission(HSCDRRN400MD2A3_I2C);
  Wire.write(1);
  Wire.endTransmission();
  // now get the data from the sensor
  delay (20);

  Wire.requestFrom(HSCDRRN400MD2A3_I2C, 4);

  byte a     = Wire.read();
  byte b     = Wire.read();
  byte c     = Wire.read();
  byte d     = Wire.read();
  byte status1 = (a & 0xc0) >> 6;
  int bridge_data = ((a & 0x3f) << 8) + b;
  int temperature_data = ((c << 8) + (d & 0xe0)) >> 5;
  pressure = 1.0 * (bridge_data - OUTPUT_MIN) * (PRESSURE_MAX - PRESSURE_MIN) / (OUTPUT_MAX - OUTPUT_MIN) + PRESSURE_MIN;
  pressure_mmhg = 0.750062 * pressure;
  temperature = (temperature_data * 0.0977) - 50;
  return pressure_mmhg;
}

void setup()
{
  Wire.begin(); // wake up I2C bus
  delay (500);
  Serial.begin(9600);
  Serial.println ("Sketch has started");

  pinMode (LedPin, OUTPUT);
  digitalWrite(LedPin, LOW);

  pinMode(ButtonPin, INPUT_PULLUP);
  button.attach(ButtonPin);
  button.interval(20);//20 ms debounce interval
 
  pinMode(4, OUTPUT);

  if (rtc.lostPower())
  {
    Serial.println("RTC lost power,set the time");
    //rtc.adjust(DateTime(2016,8,16,14,16,00));
  }
  delay(10);

  if (!SD.begin(10))
  {
    Serial.println(F("begin failed"));
    return;
  }

  while (SD.exists(filename)) {
    if (filename[BASE_NAME_SIZE + 1] != '9') {
      filename[BASE_NAME_SIZE + 1]++;
    }
    else if (filename[BASE_NAME_SIZE] != '9') {
      filename[BASE_NAME_SIZE + 1] = '0';
      filename[BASE_NAME_SIZE]++;
    }
    else {
      Serial.println(F("Can't create file name"));
      return;
    }
  }
  File file = SD.open(filename, FILE_WRITE);
  digitalWrite(LedPin, HIGH);//indicate file being written
  DateTime now = rtc.now();
  file.print(now.year(), DEC);
  file.print('/');
  file.print(now.month(), DEC);
  file.print('/');
  file.print(now.day(), DEC);
  file.print(',');
  file.print(now.hour(), DEC);
  file.print(':');
  file.print(now.minute(), DEC);
  file.print(':');
  file.print(now.second(), DEC);
  file.println();
  file.print("Time( ms)");
  file.print(',');
  file.print("Presuure (mmhg)");
  file.print(',');
  file.print("Valve status");
  file.println();
  file.close();
  digitalWrite(LedPin, LOW);//file write finished

  Serial.print(now.year(), DEC);
  Serial.print('/');
  Serial.print(now.month(), DEC);
  Serial.print('/');
  Serial.print(now.day(), DEC);
  Serial.print(',');
  Serial.print(now.hour(), DEC);
  Serial.print(':');
  Serial.print(now.minute(), DEC);
  Serial.print(':');
  Serial.print(now.second(), DEC);
  Serial.println();
}
void loop()
{
  // float pressure, temperature;
  // float pressure_mmhg;
  float Pdelta;
  static bool foundMax = false;
  static bool foundMin = false;
  static bool solstate = false;
  static float max = 0;
  static float min = 0;

  const long interval = 500;
  static unsigned long int startTime = millis(); 
  time = millis();
  if (count == 0)
  {
    senval[1] = getPressure();//pressure_mmhg;
    //delay(20);//??
    senval[1] = (senval[1] + getPressure()) / 2;
    ++count;
  }

  else
  {
    senval[2] = getPressure();//pressure_mmhg;
    //delay(20);//?
    senval[2] = (senval[2] + getPressure()) / 2;
    //++count;//never used but will overflow

    if (foundMax == false && (senval[1] > senval[0]) && (senval[1] > senval[2]))
    {
      foundMax = true;
      max = senval[1];
      Serial.print("max  ");
      Serial.println(max);
    }
    else if (foundMin == false && (senval[1] < senval[0]) && (senval[1] < senval[2]))
    {
      foundMin = true;
      min = senval[1];
      Serial.print("min  ");
      Serial.println(min);
    }

    if (solState == false && foundMax == true && foundMin == true)
    {
      foundMax = false;//enable finding of next max
      foundMin = false;

      Pdelta = max - min;
      Serial.print("Pdelta  ");
      Serial.println(Pdelta);
      if ( Pdelta >= 20)
      {
        digitalWrite(4, HIGH);
        startTime = millis();
        solstate = true;
      }
    }
    //keep solenoid triggered for period of time 500ms
    if (solstate == true && millis() - startTime >= interval)
    {
      digitalWrite(4, LOW);
      solstate = false;
    }

    senval[0] = senval[1];
    senval[1] = senval[2];
    senval[2] = 0;

  }

  int solenoidvalue = digitalRead(4);
  
  buttonCheck();//update logFlag

  if (logFlag)//logFlag determines write enable
  {
    File file = SD.open(filename, FILE_WRITE);
    if (file)
    {
      digitalWrite(LedPin, HIGH);//indicate file being written
      Serial.print(time);
      Serial.print(',');
      Serial.println(pressure_mmhg);
      Serial.print(',');

      file.print(time);
      file.print(',');
      file.print(pressure_mmhg);
      file.print(',');
      file.print(solenoidvalue);
      file.println();
      file.close();
      digitalWrite(LedPin, LOW);//file write finished
    }
  }
}

I checked it near the pdelta condition so that max min values are read continously.

if ( Pdelta >= 20 && solstate==false)

This looks good, and it is a better place for the control than where I placed it.

Syntax error here with = instead of == for comparison
//if(peakcounter = 5)
if(peakcounter ==5)

I think there were some logical errors in this section, and I think this is correct.

if (foundMax == true && foundMin == true) //move solState control to Pdelta>20 section 
{
  foundMax = false;//enable finding of next max
  foundMin = false;

  Pdelta = max - min;
  Serial.print("Pdelta  ");
  Serial.println(Pdelta);

  if (solState == false && Pdelta >= 20)
  {
    peakcounter = peakcounter + 1 ;
    Serial.print("peakcounter");
    Serial.print(peakcounter);
  }
  if (peakcounter == 5)
  {
    digitalWrite(4, HIGH);
    startTime = millis();
    solstate = true;
    peakcounter = 0;
  }
  //only reset peak counter when ==5
  // else{
  //   peakcounter =0;
  // }
}
//keep solenoid triggered for period of time 500ms
if (solstate == true && millis() - startTime >= interval)
{
  digitalWrite(4, LOW);
  solstate = false;
}

Syntax error here with = instead of == for comparison

I corrected it . But I still get only garbage values in the peakcounter.

Did you fix the else statement where peakcounter was reset?

But I still get only garbage values in the peakcounter.

What does this mean? What serial output do you see? There is no new line statement after Serial.print(peakcounter). Are you sure you are not seeing the next value to be printed appended to the peakcounter value.

Do you have serial print debug statements every time the peakcounter variable appears in the code? What do they show? Is it garbage after intitialization to 0? Is it garbage before the first time it is incremented? You should be able to pinpoint where it turns from 0 to "garbage".

But it stays open for more than 0.5 sec !
Is this not the right approach to do this?

When solState == true, peakcounter is still ==5 because it hasn't been reset or incrmented, and there is constant reentry into the bloc of code which resets startTime and solState.

I wold place the reset of peakcounter to 0 in the code block where it ==5 which is where it was a few post ago. I think it could even be in both places.

if ( foundMax == true && foundMin == true)
    {
      foundMax = false;//enable finding of next max
      foundMin = false;

      Pdelta = max - min;
      Serial.print("Pdelta  ");
      Serial.println(Pdelta);
      
      if (solstate==false && Pdelta >= 20)
      {
        ++peakcounter ;
        Serial.print("peakcounter");
        Serial.print(peakcounter);
      }
       if(peakcounter == 5)
        {
        digitalWrite(4, HIGH);
        startTime = millis();
        solstate = true;
        peakcounter = 0;//reset here
       
      }
     
    }
    //keep solenoid triggered for period of time 500ms
    if (solstate == true && millis() - startTime >= interval)
    {
      digitalWrite(4, LOW);
      solstate = false;
      //peakcounter=0;
    }

I don't know why you have put a bunch of stuff not related to timing a solenoid (so that when it goes on, it stays on for 500ms and when it goes off your can't turn it on for another 500ms) in the solenoid driver class. Why on earth is a class whose job - stated in the class name - is to drive a solenoid opening up SD cards? Why is it talking to pin 4? It has a class variable containing the pin number, named 'pin'. It should be using that. Why is it fooling about with ledPin and buttonPin when those variables are not used in the class loop() and obviously aren't part of it's operation?

A class should do one, or a small number of closely related things.

In answer to your question

Can we use a if loop and check if the solenoid is resting and then set the counter to zero?
Can we make the states public and then use it to set counter to zero?

Yes, but a better might be is to provide a public boolean 'isResting' method on the solenoid class. The functions 'actiavte', 'loop', and 'isResting' for the public interface of the class.

I wold place the reset of peakcounter to 0 in the code block where it ==5 which is where it was a few post ago. I think it could even be in both places

I placed it in both the places and it works fine now!
Thank you :slight_smile:

Why is it fooling about with ledPin and buttonPin when those variables are not used in the class loop() and obviously aren't part of it's operation?

Sorry i messed it up! I went through your signature Arduino the Object Way and was able to understand quite few things about classes! I was able to correct the code and make it work !

Thank you :slight_smile:

I placed it in both the places and it works fine now!

was able to understand quite few things about classes! I was able to correct the code and make it work !

I'm glad you got it sorted out. I would appreciate if you posted the two working equivalent code versions. I'm personally interested in seeing if the OO code simplifies the conditional statements and logical nesting of the bloc structured code.