Message is send twice

[esawyja]

Hi all,
I'm trying to detect when my generator runs, the external detection circuit works with no problems, but when the ProMini starts, it sends the message twice to the controller, although there was not a state change, see me code below

void loop() {
   bool tripped = digitalRead(GENERATOR_PIN) == HIGH;
  wait(1000);
  if (tripped == 0) {
    wait(5000);
    Serial.println("Generator RUNNING");
    send(msgGENERATOR.set(1));
    sleep(digitalPinToInterrupt(GENERATOR_PIN), CHANGE, SLEEP_TIME);
    //send(msgGENERATOR.set(tripped?"1":"0"));
  }
  if (tripped == 1) {
    Serial.println("Generator NOT running - Not sending");
    wait(5000);
      bool tripped = digitalRead(GENERATOR_PIN) == HIGH;
      if (tripped == 1) {
        Serial.println("Generator NOT running - SENDING....");
        send(msgGENERATOR.set(0));
        sleep(digitalPinToInterrupt(GENERATOR_PIN), CHANGE, SLEEP_TIME);
      }
  }  
}  

The serial port output demonstrate the problem, I'm checking twice if the generator is running, but it does not seem to break out of the loop, any help please!!

0 MCO:BGN:INIT NODE,CP=RRNNA--,VER=2.1.1
4 TSM:INIT
4 TSF:WUR:MS=0
8 TSM:INIT:TSP OK
10 TSM:INIT:STATID=11
12 TSF:SID:OK,ID=11
14 TSM:FPAR
145 TSF:MSG:SEND,11-11-255-255,s=255,c=3,t=7,pt=0,l=0,sg=0,ft=0,st=OK:
587 TSF:MSG:READ,0-0-11,s=255,c=3,t=8,pt=1,l=1,sg=0:0
593 TSF:MSG:FPAR OK,ID=0,D=1
2152 TSM:FPAR:OK
2152 TSM:ID
2154 TSM:ID:OK
2156 TSM:UPL
2164 TSF:MSG:SEND,11-11-0-0,s=255,c=3,t=24,pt=1,l=1,sg=0,ft=0,st=OK:1
2215 TSF:MSG:READ,0-0-11,s=255,c=3,t=25,pt=1,l=1,sg=0:1
2222 TSF:MSG:PONG RECV,HP=1
2224 TSM:UPL:OK
2226 TSM:READY:ID=11,PAR=0,DIS=1
2238 TSF:MSG:SEND,11-11-0-0,s=255,c=3,t=15,pt=6,l=2,sg=0,ft=0,st=OK:0100
2289 TSF:MSG:READ,0-0-11,s=255,c=3,t=15,pt=6,l=2,sg=0:0100
2304 TSF:MSG:SEND,11-11-0-0,s=255,c=0,t=17,pt=0,l=5,sg=0,ft=0,st=OK:2.1.1
2320 TSF:MSG:SEND,11-11-0-0,s=255,c=3,t=6,pt=1,l=1,sg=0,ft=0,st=OK:0
4335 TSF:MSG:SEND,11-11-0-0,s=6,c=0,t=3,pt=0,l=0,sg=0,ft=0,st=OK:
4349 TSF:MSG:SEND,11-11-0-0,s=7,c=0,t=30,pt=0,l=0,sg=0,ft=0,st=OK:
4358 MCO:REG:REQ
4368 TSF:MSG:SEND,11-11-0-0,s=255,c=3,t=26,pt=1,l=1,sg=0,ft=0,st=OK:2
4417 TSF:MSG:READ,0-0-11,s=255,c=3,t=27,pt=1,l=1,sg=0:1
4423 MCO:PIM:NODE REG=1
4427 MCO:BGN:STP
5427 MCO:BGN:INIT OK,TSP=1
Generator NOT running - Not sending
Generator NOT running - SENDING....
11440 TSF:MSG:SEND,11-11-0-0,s=6,c=1,t=2,pt=2,l=2,sg=0,ft=0,st=OK:0
11446 MCO:SLP:MS=3600000,SMS=0,I1=1,M1=2,I2=255,M2=255
11452 MCO:SLP:TPD
11454 MCO:SLP:WUP=1
Generator NOT running - Not sending
Generator NOT running - SENDING....
17469 TSF:MSG:SEND,11-11-0-0,s=6,c=1,t=2,pt=2,l=2,sg=0,ft=0,st=OK:0
17475 MCO:SLP:MS=3600000,SMS=0,I1=1,M1=2,I2=255,M2=255
17481 MCO:SLP:TPD

[gohan]

Try adding some more debug prints of variables on serial an try to understand why it loops twice

[core_c]

From your log i can see that your node goes to sleep, and immediately wakes up again.
I don't know the value of your SLEEP_TIME, but i guess it's not a value close to 0.
It looks like your sleep() cycle is interrupted by a change on your GENERATOR_PIN.
What if you inserted a short pause just before going to sleep, just after send(msgGENERATOR)?

Another way to handle interrupts:
Your code uses an interrupt to detect changes of the generator status, but you handle possible changes outside the interrupt-handler (the code that is executed whenever there was a change on the pin). Your sleep function can exit in two ways: by interrupt OR by the SLEEP_TIME expiring. Your code does the same in both cases. Even if there was no change on the pin, but the timer expired, a message will be send.
If you only want to send a message when there was actually a change of signal on the pin, i would use an interrupt-handler that detects any change, and let the loop() code know about it.

volatile bool generator_pin_changed = false;
volatile int generator_pin_value;
void generator_pin_interrupt_handler() {
  generator_pin_changed = true;
  generator_pin_value = digitalRead(GENERATOR_PIN);
}

void setup() {
  attachInterrupt(digitalPinToInterrupt(GENERATOR_PIN), generator_pin_interrupt_handler, CHANGE);
}

void loop() {
  if (generator_pin_changed) {
    generator_pin_changed = false;  /* reset for next detection */
    if (generator_pin_value == LOW) {
      Serial.println("Generator RUNNING");
      send(msgGENERATOR.set(1));
    } else {
      Serial.println("Generator NOT running - SENDING....");
      send(msgGENERATOR.set(0));
    }
  }
  wait(200);
  /* sleep again if the timer expired. stop sleeping if woken up by an interrupt. */
  while (sleep(digitalPinToInterrupt(GENERATOR_PIN), CHANGE, SLEEP_TIME) < 0);
}

[esawyja]

@core_c Thanks so much, this did the trick, thanks again!!!

[esawyja]

@core_c I spoke to soon, looks like it detects the change, but does not do anything, I made the unsigned long SLEEP_TIME = 0;

0 MCO:BGN:INIT NODE,CP=RRNNA--,VER=2.1.1
4 TSM:INIT
4 TSF:WUR:MS=0
8 TSM:INIT:TSP OK
10 TSM:INIT:STATID=11
12 TSF:SID:OK,ID=11
14 TSM:FPAR
145 TSF:MSG:SEND,11-11-255-255,s=255,c=3,t=7,pt=0,l=0,sg=0,ft=0,st=OK:
405 TSF:MSG:READ,0-0-11,s=255,c=3,t=8,pt=1,l=1,sg=0:0
411 TSF:MSG:FPAR OK,ID=0,D=1
2152 TSM:FPAR:OK
2152 TSM:ID
2154 TSM:ID:OK
2156 TSM:UPL
2164 TSF:MSG:SEND,11-11-0-0,s=255,c=3,t=24,pt=1,l=1,sg=0,ft=0,st=OK:1
2215 TSF:MSG:READ,0-0-11,s=255,c=3,t=25,pt=1,l=1,sg=0:1
2222 TSF:MSG:PONG RECV,HP=1
2224 TSM:UPL:OK
2226 TSM:READY:ID=11,PAR=0,DIS=1
2238 TSF:MSG:SEND,11-11-0-0,s=255,c=3,t=15,pt=6,l=2,sg=0,ft=0,st=OK:0100
2289 TSF:MSG:READ,0-0-11,s=255,c=3,t=15,pt=6,l=2,sg=0:0100
2304 TSF:MSG:SEND,11-11-0-0,s=255,c=0,t=17,pt=0,l=5,sg=0,ft=0,st=OK:2.1.1
2320 TSF:MSG:SEND,11-11-0-0,s=255,c=3,t=6,pt=1,l=1,sg=0,ft=0,st=OK:0
4335 TSF:MSG:SEND,11-11-0-0,s=6,c=0,t=3,pt=0,l=0,sg=0,ft=0,st=OK:
4349 TSF:MSG:SEND,11-11-0-0,s=7,c=0,t=30,pt=0,l=0,sg=0,ft=0,st=OK:
4358 MCO:REG:REQ
4368 TSF:MSG:SEND,11-11-0-0,s=255,c=3,t=26,pt=1,l=1,sg=0,ft=0,st=OK:2
4417 TSF:MSG:READ,0-0-11,s=255,c=3,t=27,pt=1,l=1,sg=0:1
4423 MCO:PIM:NODE REG=1
4427 MCO:BGN:STP
interupt
1
5429 MCO:BGN:INIT OK,TSP=1
1
Generator NOT running - SENDING....
5441 TSF:MSG:SEND,11-11-0-0,s=6,c=1,t=2,pt=2,l=2,sg=0,ft=0,st=OK:0
5648 MCO:SLP:MS=0,SMS=0,I1=1,M1=1,I2=255,M2=255
5652 MCO:SLP:TPD
5654 MCO:SLP:WUP=1
5857 MCO:SLP:MS=0,SMS=0,I1=1,M1=1,I2=255,M2=255
5861 MCO:SLP:TPD
5863 MCO:SLP:WUP=1
6066 MCO:SLP:MS=0,SMS=0,I1=1,M1=1,I2=255,M2=255
6070 MCO:SLP:TPD

[gohan]

Why sleep time = 0?

[core_c]

@esawyja I don't really know what your goal is, but is it correct to send a 1 if the generator is running, and to send a 0 when it's not running?
It seems more logical to send a 1 to enable a device, and 0 to disable it.

Could it be that you disable the generator, and it simply is not responding anymore?

[esawyja]

@core_c Hi

Does it really matter, it's just the way the external circuitry is set up, it pulls to a low when the generator is running, I get get it working with this below, but I'm afraid my coding skills are not that good

void loop() {
   bool tripped = digitalRead(GENERATOR_PIN) == HIGH;
   Serial.print("value of tripped = ");
   Serial.println(tripped);
   wait(1000);
   if ((tripped == 0) && ( tripped != oldValue)) {
      Serial.println("Generator RUNNING - Not sending");
      wait(1000);
      bool tripped = digitalRead(GENERATOR_PIN) == HIGH;
      if (tripped == 0) {
        Serial.println("Generator RUNNING - SENDING");
        send(msgGENERATOR.set(1));
        wait(10000);
        send(msgGENERATOR.set(1));
        oldValue = tripped;
        read_batt();
      }
    }
  
  if ((tripped == 1) && ( tripped != oldValue)) {
    Serial.println("Generator NOT running - Not sending");
    wait(1000);
    bool tripped = digitalRead(GENERATOR_PIN) == HIGH;
    if (tripped == 1) {
        Serial.println("Generator NOT running - SENDING....");
        send(msgGENERATOR.set(0));
        wait(10000);
        send(msgGENERATOR.set(0));
        oldValue = tripped;
        read_batt();
    }
  }  
sleep(digitalPinToInterrupt(GENERATOR_PIN), CHANGE, SLEEP_TIME);    
}  
  
void read_batt() {
  wait(15000);
  int sensorValue = analogRead(BATTERY_SENSE_PIN);
  float batteryV = sensorValue * volt_value;
  send(batt_msg.set(batteryV,2));
  return;  
}

[core_c]

@esawyja No, it does not matter if it's active low or active high. I was only thinking that it could be an oversight.. something wrong in code that someone (possibly) keeps missing because they are assuming that that part of the code is correct.

About what you said previously: "looks like it detects the change, but does not do anything":
Correct me if i'm wrong, but this looks like it detects, and does something:

interupt
1
5429 MCO:BGN:INIT OK,TSP=1
1
Generator NOT running - SENDING....
5441 TSF:MSG:SEND,11-11-0-0,s=6,c=1,t=2,pt=2,l=2,sg=0,ft=0,st=OK:0

After that, when the generator did not change state anymore, you will see a stream of only sleep/wake-up messages (a stream, because sleeptime is 0). I see that stream start at 5648.

In your current program, i'm curious if that second send() is really needed (the last send in the "send,wait(10000),send" sequence).
But anyway, your skills got it working.. :heavy_check_mark:

[esawyja]

@core_c thanks, just a bit worried about my programming skills, yes the reason for the double send is that sometimes I get st=NACK and I'm worried about missing the start , stop message, hence I send the message twice, I don't know how to detect the st=NACK, thanks for the help

[Boots33]

@esawyja send will return a boolean result which you can use to determine if the message was successful. For an example have a look at this post

[esawyja]

@Boots33 Thanks so much, I'll have a look

[core_c]

@esawyja In your last program, your battery level is only send when the generator changed state.
Probably you want to let the battery report it's level at regular intervals, independent of the generator state.
Boots33 pointed out how to know if a message has been sent succesfully.
Here is a version of your sketch that does both things:

#define RETRY_TIMES 3
#define RETRY_WAIT 10000
#define BATTERY_REPORT_INTERVAL 60000

int oldValue;
uint32_t battery_report_time;

volatile bool generator_pin_changed = false;
volatile int generator_pin_value;
void generator_pin_interrupt_handler() {
  generator_pin_changed = true;
  generator_pin_value = digitalRead(GENERATOR_PIN);
}

void setup() {
  oldValue = digitalRead(GENERATOR_PIN);
  sendGeneratorMsg(oldValue==HIGH?0:1);
  battery_report_time = millis() + BATTERY_REPORT_INTERVAL;
  attachInterrupt(digitalPinToInterrupt(GENERATOR_PIN), generator_pin_interrupt_handler, CHANGE);
}

void loop() {
  int value = digitalRead(GENERATOR_PIN);
  if (value != oldValue) {
    oldValue = value;
    if (value == LOW) {
      Serial.println("Generator RUNNING - SENDING");
      sendGeneratorMsg(1);
    } else {
      Serial.println("Generator NOT running - SENDING....");
      sendGeneratorMsg(0);
    }
  }
  long sleep_time_left = battery_report_time - millis();
  if (sleep_time_left < 0) {
    battery_report_time = millis() + BATTERY_REPORT_INTERVAL;
    sleep_time_left = BATTERY_REPORT_INTERVAL;
    read_batt();
  }
  sleep(digitalPinToInterrupt(GENERATOR_PIN), CHANGE, sleep_time_left);    
}  

void sendGeneratorMsg(int state) {
  for (int i=0; i<RETRY_TIMES; i++) {
    if (send(msgGENERATOR.set(state))) {
      Serial.println("Generator message sent");
      return;
    }
    wait(RETRY_WAIT);
  }
  Serial.println("Failed to send generator message");
}

void read_batt() {
  wait(15000);
  int sensorValue = analogRead(BATTERY_SENSE_PIN);
  float batteryV = sensorValue * volt_value;
  send(batt_msg.set(batteryV,2));
}

EDIT: I have changed ^^the code^^ with the suggestions i made in my next post.

[esawyja]

@core_c thanks so much, I will try it ASAP and provide feedback, thanks again!

[esawyja]

@core_c This works a treat, but I don't get the battery readings, if I look at the debug log, the MS seems to become quite large

71362 MCO:SLP:MS=9681,SMS=0,I1=1,M1=1,I2=255,M2=255
71366 MCO:SLP:TPD
71370 MCO:SLP:WUP=-1
71372 MCO:SLP:MS=9671,SMS=0,I1=1,M1=1,I2=255,M2=255
71378 MCO:SLP:TPD
71380 MCO:SLP:WUP=1
71383 MCO:SLP:MS=9660,SMS=0,I1=1,M1=1,I2=255,M2=255
71389 MCO:SLP:TPD
71391 MCO:SLP:WUP=1
Generator RUNNING - SENDING 
71401 TSF:MSG:SEND,11-11-0-0,s=6,c=1,t=2,pt=2,l=2,sg=0,ft=0,st=OK:1
Generator message sent
81424 MCO:SLP:MS=4294966915,SMS=0,I1=1,M1=1,I2=255,M2=255
81430 MCO:SLP:TPD
81432 MCO:SLP:WUP=1
81434 MCO:SLP:MS=4294966905,SMS=0,I1=1,M1=1,I2=255,M2=255
81440 MCO:SLP:TPD
81444 MCO:SLP:WUP=1
Generator NOT running - SENDING...
81498 TSF:MSG:SEND,11-11-0-0,s=6,c=1,t=2,pt=2,l=2,sg=0,ft=0,st=OK:0
Generator message sent
91521 MCO:SLP:MS=4294956818,SMS=0,I1=1,M1=1,I2=255,M2=255
91527 MCO:SLP:TPD

Any suggestions please?

[esawyja]

@core_c Sorry me again, here I made
#define BATTERY_REPORT_INTERVAL 2000
and printed the value

battery_report_time = millis() + BATTERY_REPORT_INTERVAL;
Serial.print("Battery report time is = ");
Serial.println(battery_report_time);

It looks like the sleep uses the time left over, as it prints faster and faster on the serial port and then the MS value just increases 10 fold

21035 TSF:MSG:SEND,11-11-0-0,s=6,c=1,t=2,pt=2,l=2,sg=0,ft=0,st=OK:0
Battery report time is = 23043
21043 MCO:BGN:INIT OK,TSP=1
21049 MCO:SLP:MS=1994,SMS=0,I1=1,M1=1,I2=255,M2=255
21053 MCO:SLP:TPD
21057 MCO:SLP:WUP=1
21059 MCO:SLP:MS=1984,SMS=0,I1=1,M1=1,I2=255,M2=255
21065 MCO:SLP:TPD
21067 MCO:SLP:WUP=-1
21069 MCO:SLP:MS=1974,SMS=0,I1=1,M1=1,I2=255,M2=255
21075 MCO:SLP:TPD
21078 MCO:SLP:WUP=-1
21080 MCO:SLP:MS=1963,SMS=0,I1=1,M1=1,I2=255,M2=255
21086 MCO:SLP:TPD
21088 MCO:SLP:WUP=-1
21092 MCO:SLP:MS=1951,SMS=0,I1=1,M1=1,I2=255,M2=255
21096 MCO:SLP:TPD
21100 MCO:SLP:WUP=-1
......
......
22878 MCO:SLP:MS=165,SMS=0,I1=1,M1=1,I2=255,M2=255
22882 MCO:SLP:TPD
22884 MCO:SLP:WUP=-1
22888 MCO:SLP:MS=155,SMS=0,I1=1,M1=1,I2=255,M2=255
22892 MCO:SLP:TPD
22896 MCO:SLP:WUP=-1
22898 MCO:SLP:MS=145,SMS=0,I1=1,M1=1,I2=255,M2=255
22904 MCO:SLP:TPD
22906 MCO:SLP:WUP=-1
22908 MCO:SLP:MS=135,SMS=0,I1=1,M1=1,I2=255,M2=255
22915 MCO:SLP:TPD
22917 MCO:SLP:WUP=-1
22919 MCO:SLP:MS=124,SMS=0,I1=1,M1=1,I2=255,M2=255
22925 MCO:SLP:TPD
22927 MCO:SLP:WUP=-1
22931 MCO:SLP:MS=112,SMS=0,I1=1,M1=1,I2=255,M2=255
22935 MCO:SLP:TPD
22937 MCO:SLP:WUP=-1
22941 MCO:SLP:MS=102,SMS=0,I1=1,M1=1,I2=255,M2=255
22945 MCO:SLP:TPD
22947 MCO:SLP:WUP=-1
22951 MCO:SLP:MS=92,SMS=0,I1=1,M1=1,I2=255,M2=255
22958 MCO:SLP:TPD
22960 MCO:SLP:WUP=-1
22962 MCO:SLP:MS=81,SMS=0,I1=1,M1=1,I2=255,M2=255
22968 MCO:SLP:TPD
22970 MCO:SLP:WUP=-1
22972 MCO:SLP:MS=71,SMS=0,I1=1,M1=1,I2=255,M2=255
22978 MCO:SLP:TPD
22980 MCO:SLP:WUP=-1
22982 MCO:SLP:MS=61,SMS=0,I1=1,M1=1,I2=255,M2=255
22988 MCO:SLP:TPD
22990 MCO:SLP:WUP=-1
22992 MCO:SLP:MS=51,SMS=0,I1=1,M1=1,I2=255,M2=255
22999 MCO:SLP:TPD
23001 MCO:SLP:WUP=-1
23005 MCO:SLP:MS=38,SMS=0,I1=1,M1=1,I2=255,M2=255
23009 MCO:SLP:TPD
23011 MCO:SLP:WUP=-1
23015 MCO:SLP:MS=28,SMS=0,I1=1,M1=1,I2=255,M2=255
23019 MCO:SLP:TPD
23021 MCO:SLP:WUP=-1
23025 MCO:SLP:MS=18,SMS=0,I1=1,M1=1,I2=255,M2=255
23029 MCO:SLP:TPD
23031 MCO:SLP:WUP=-1
23035 MCO:SLP:MS=8,SMS=0,I1=1,M1=1,I2=255,M2=255
23040 MCO:SLP:TPD
23044 MCO:SLP:WUP=-1
23046 MCO:SLP:MS=4294967293,SMS=0,I1=1,M1=1,I2=255,M2=255
23052 MCO:SLP:TPD

After this, there is nothing more on the serial port

[core_c]

@esawyja You are right. I made a mistake. This should fix it:
Change this line:

uint32_t sleep_time_left = battery_report_time - millis();

into this:

long sleep_time_left = battery_report_time - millis();

A variable of type long can hold a negative number, but an unsigned long (same as uint32_t) can never hold a negative number (because it is unsigned, no sign, no minus sign, no negative).
So, if (sleep_time_left < 0) will never be true.. That was the mistake.

I was also wondering if the generator would produce interrupts without using attachInterrupt().
Without that, you can still poll the generator pin, and see if the value changed (what is happening now), but the sleep() cycle would not be interrupted when the pin changed.
To be on the safe side, i would also add the following to your code:

void setup() {
  attachInterrupt(digitalPinToInterrupt(GENERATOR_PIN), generator_pin_interrupt_handler, CHANGE);
}

Hopefully that'll make it work as intended.

[esawyja]

@core_c Thanks for that
I had to change the battery reading routine to this below, else when it gets to the routine, it stays there and never get back to the loop, I hope it makes sense

void read_batt() {
  wait(15000);
  int sensorValue = analogRead(BATTERY_SENSE_PIN);
  float batteryV = sensorValue * volt_value;
  send(batt_msg.set(batteryV,2));
  long sleep_time_left = battery_report_time - millis();
  battery_report_time = millis() + BATTERY_REPORT_INTERVAL;
  sleep_time_left = BATTERY_REPORT_INTERVAL;
  return;
}

If I add this to setup, I get the following error

  battery_report_time = millis() + BATTERY_REPORT_INTERVAL;
  Serial.print("Battery report time is = ");
  Serial.println(battery_report_time);
 attachInterrupt(digitalPinToInterrupt(GENERATOR_PIN), generator_pin_interrupt_handler, CHANGE);  
}

gen_run_sketch:54: error: 'generator_pin_interrupt_handler' was not declared in this scope

  attachInterrupt(digitalPinToInterrupt(GENERATOR_PIN), generator_pin_interrupt_handler, CHANGE);  

                                                        ^

exit status 1
'generator_pin_interrupt_handler' was not declared in this scope

[core_c]

@esawyja attachInterrupt() should only be done once, in the setup() function.

[esawyja]

@core_c I'm sorry I do not understand, must I replace this in loop

sleep(digitalPinToInterrupt(GENERATOR_PIN), CHANGE, sleep_time_left);  

With this in setup?

attachInterrupt(digitalPinToInterrupt(GENERATOR_PIN), generator_pin_interrupt_handler, CHANGE);

Sorry for all the questions!! This is sooo confusing to me, it also seems that the battery level is not sending, I left it running for 12 hours plus and never got a reading, the line
#define BATTERY_REPORT_INTERVAL 60000
Does this define that the battery level must be send ever 60000 milli seconds?