Photon publishing when it shouldn't?

I’m publishing distance data (from a HC-SR04 ultrasonic sensor) every 5 minutes which sometimes works, and other times publishes when the value is out of range, which should be prevented by an if clause, but it isn’t. I’m not getting a random value, but one that is either 172091 or 172092 mm (the correct range should be around 320 mm). I thought maybe this was a problem with IFTTT or with Google sheets, but I see the same incorrect values on the Particle Dashboard. Here is the code I’m using:

int trigPin = D0;  
int echoRisingPin = D2;
int echoFallingPin = D4;
unsigned long startTime;
unsigned long endTime;
unsigned long distance;
String  waterLevel;
bool publishValue; // if true, pusblish the event, "tankMonitor", to IFTTT which adds the waterLevel value to a Google sheet
bool initiatePulse; //  if true, causes an ultrasonic pulse to be sent out by the sensor (HC-SR04)


void setup() {
    publishValue = false;
    initiatePulse = true;
    pinMode(trigPin, OUTPUT);
    digitalWrite(trigPin, LOW);
    attachInterrupt(echoRisingPin, risingDetect, RISING);
    attachInterrupt(echoFallingPin, fallingDetect, FALLING);
}


void loop() {
    if (publishValue) {
        Particle.publish("tankMonitor", waterLevel);
        delay(1000);
        System.sleep(SLEEP_MODE_DEEP,300);
    }
    
    if (initiatePulse) {
        initiatePulse = false; // set to false so another trigger pulse is not sent on the next turn of the loop
        triggerSensor(); // trigger the sensor to send out an ultrasonic pulse
    }
}


void risingDetect() {
    startTime = micros();
}


void fallingDetect() {
    endTime = micros();
    distance = (endTime - startTime)/5.82; // distance in millimeters;
    waterLevel = String::format("%lu mm", distance);
    if (distance > 310 && distance < 400) {
        publishValue = true;
    }else{
        initiatePulse = true;
    }
}


void triggerSensor() {
    delay(1000);
    digitalWrite(trigPin, HIGH);
    delayMicroseconds(15);
    digitalWrite(trigPin, LOW); 
}

The data I’m publishing is the String waterLevel, but my if clause tests the unsigned long, distance. Could there be something wrong with the conversion from the unsigned long to the string, or am I just missing something obviously wrong in my code?

void fallingDetect() {
    endTime = micros();
    distance = (endTime - startTime)/5.82; // distance in millimeters;
    waterLevel = String::format("%lu mm", distance);
    if (distance &gt; 310 && distance &lt; 400) {
        publishValue = true;
    }else{
        initiatePulse = true;
    }
}

I wonder if you are trying to accomplish too much in this ISR… Have you tried moving the waterlevel = String::format("%lu mm", distance); function into loop?

Also, any variable that you manipulate in an ISR ought to be declared volatile :

...
volatile unsigned long startTime;
volatile unsigned long endTime;
volatile unsigned long distance;
...

finally, using String class can cause issues… if you can use null terminated array of chars (C string) you may find it more stable over the long term.

What do you mean by “using String class can cause issues”? Is the String class usable or not? Does it have known issues that are being worked on? I’ll try implementing your other suggestions and see if they fix the issue. As far as “doing too much in an ISR”, what is too much? I know you’re not supposed to have anything long running in one, but really, if this is too much, what can you do in one?

In addition to the issues pointed out above that should definitely be fixed, it looks like you’re never clearing the publishValue flag so as soon as one value is within the range, all future values are forever set to be published. You do this for initiatePulse, so it was probably just an oversight.

@indraastra, it is cleared because when the Photon wakes up from deep sleep, setup() is run again.

Ah, sorry @Ric ! I missed that part.

Have you already tried @BulldogLowell’s suggestions? I think the String class issue he is probably alluding to is that it covertly uses heap memory, which could lead to eventual heap fragmentation. Rather than using String::format, you can try snprintf’ing to a fixed-length buffer to sidestep such issues entirely.

I have a hunch that because you didn’t declare the flags you’re setting as retained variables, you can’t count on them being set to a non-arbitrary value upon waking up.

Edit: Nevermind, I see that those are being initialized in setup as well. Out of ideas for now :stuck_out_tongue:

@indraastra, I haven’t tried sprintf’ing yet, but I’ve made the other changes. I declared the variables as volatile, and moved the String::format line into loop(). I’m still having the same problem, so I’ll get rid of the format function, and see if the fixes it. I want to take this a step at a time, so I can understand what could cause this problem.

    attachInterrupt(echoRisingPin, risingDetect, RISING);
    attachInterrupt(echoFallingPin, fallingDetect, FALLING);

Just to be on the safe side of things you might also want to add pinMode(xxx, INPUT) for your interrupt pins.
I’ve seen an open issue in the open sources about attachInterrupt() not setting the pinMode() implicitly (which I personally don’t see as an issue but rather desirable, to give the considered programmer the freedom of choice of the best suited mode ;-)).

I’d also add a sanity check as startTime != 0 and make sure that startTime gets reset after (or inside) fallingDetect().
Also move the String building out of the ISR into a place where timing is non-critical - as @BulldogLowell already suggested.

I’d also add a publishValue = false into the else block of your range check, since you might actually have another trigger to one of your ISRs before your loop() gets round to acting upon publishValue == true causing the subsequent distance to be published - for that reason @indraastra’s advice is valid and relying on the auto-reset via deep-sleep might not actually work in such a case.
Keeping track of asynchronous tasks is not always that easy :sunglasses:

1 Like

trying to incorporate all of what was mentioned by @ScruffR & @indraastra , you could try something like the code below.

smallest is best. Even an empty ISR has overhead, so try to get as much done outside the ISR as possible, and in your case, you can.

const int trigPin = D0;
const int echoRisingPin = D2;
const int echoFallingPin = D4;
volatile unsigned long startTime;
volatile unsigned long endTime;
volatile bool pulseComplete;
unsigned long distance;
//String  waterLevel;
bool publishedValue; // if true, pusblish the event, "tankMonitor", to IFTTT which adds the waterLevel value to a Google sheet
bool initiatePulse; //  if true, causes an ultrasonic pulse to be sent out by the sensor (HC-SR04)

void setup() {
  publishedValue = false;
  initiatePulse = true;
  pinMode(trigPin, OUTPUT);
  digitalWrite(trigPin, LOW);
  pinMode(echoRisingPin, INPUT);  // EDIT: added
  pinMode(echoFallingPin, INPUT);  // EDIT: added
  attachInterrupt(echoRisingPin, risingDetect, RISING);
  attachInterrupt(echoFallingPin, fallingDetect, FALLING);
}

void loop() 
{
  if (pulseComplete)
  {
    pulseComplete = false;
    distance = (endTime - startTime) / 5.82; // distance in millimeters;
    if (distance > 310 && distance < 400) 
    {
      char myReading[32] = "";  //gets created and destroyed each time
      //waterLevel = String::format("%lu mm", distance); // may lead to heap fragmentation
      sprintf(myReading, "%lu mm", distance);
      Particle.publish("tankMonitor", myReading);
      publishedValue = true;
    } 
    else 
    {
      delay(1000); // put time between re-triggers
      triggerSensor();
    }
  }
  if (publishedValue)
  {
    publishValue = false;
    System.sleep(SLEEP_MODE_DEEP,300);
  }
}

void risingDetect()
{
  startTime = micros();
}

void fallingDetect()
{
  endTime = micros();
  pulseComplete = true;
}

void triggerSensor() 
{
  delay(1000);
  digitalWrite(trigPin, HIGH);
  delayMicroseconds(15);
  digitalWrite(trigPin, LOW);
}

just a guideline, I couldn’t test it :wink:

1 Like

@BulldogLowell, not quite all of the suggestions :wink:

And even loop could be stream lined by putting all the string building into the true branch of the range check.

1 Like

whoops! you are right… I edited above (yes that’s a big no-no) :smile:

1 Like

@BulldogLowell and @ScruffR, thanks for all the suggested changes. As far as I can tell, BulldogLowell’s code, with a minor addition, is working well. I had to add pulseComplete = true (and set startTime and endTime equal to 0 to be sure they had defined values the first time through loop()) to setup(), otherwise triggerSensor() is never called.

2 Likes

Super! you can say that you had a mixed bag of things to consider when using an ISR!