Message is send twice



  • 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
    
    

  • Mod

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



  • 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);
    }
    


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



  • @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
    

  • Mod

    Why sleep time = 0?



  • @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?



  • @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;  
    }
    


  • @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:



  • @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


  • Hero Member

    @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



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



  • @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.



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



  • @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?



  • @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



  • @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.



  • @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
    


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



  • @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?


Log in to reply
 

Looks like your connection to MySensors Forum was lost, please wait while we try to reconnect.