mysensors / MySensors

MySensors library and examples
https://www.mysensors.org
1.31k stars 895 forks source link

Sending multiple messages in a row breaks the transmition #892

Open nufer opened 7 years ago

nufer commented 7 years ago

I have an mqtt gateway based on a esp6288 and a sensor node which sends 10 temperature and switch values in a row.

   // Relais Status publizieren
    send(msgRelais.setSensor(11).set(PUMPE_EIN==digitalRead(RELAIS_PUMPE)),true);
    send(msgRelais.setSensor(12).set(digitalRead(RELAIS_REGLER)),true);
    send(msgRelais.setSensor(13).set(rueckkuehlen),true);

    // PT1000
    if(offset!=DEFAULT_OFFSET && tDach>-50 && tDach<200){ // komisch messwerte ignorieren
      send(msg.setSensor(10).set(tDach,1),true);   
      send(msgU.setSensor(9).set(Umess,1));    
    }

    // Alle temperaturen
    send(msg.setSensor(SENSOR_SP).set(lastTemperature[SENSOR_SP],1),true);
    send(msg.setSensor(SENSOR_RUECKLAUF).set(lastTemperature[SENSOR_RUECKLAUF],1),true);
    send(msg.setSensor(SENSOR_VORLAUF).set(lastTemperature[SENSOR_VORLAUF],1),true);
    send(msg.setSensor(21).set(lastSpeicher[SP_UNTEN],1),true);
    send(msg.setSensor(22).set(lastSpeicher[SP_OBEN],1),true);

the values arrive, but not reliable (even with the ack expected). If i add a wait(500); statement after each send, all messages arrive.

mfalkvidd commented 7 years ago

See https://forum.mysensors.org/topic/4450/sensor-presentation-failure/ for more information.

nufer commented 7 years ago

This was not ment as Question, I have my solution up and running, adding a wait(); is in my opignion not a solution for a framework but a hack arround a bug. I would suggest to add some code to the send() function. In pseduocode:

var lastMessageSentInMills; function send(..){ if lastMessageSentInMills+500ms>currentMillis wait(rest); }

fallberg commented 7 years ago

Please submit a PR with your solution and we can see if it is suitable for incorporation in the library.

mfalkvidd commented 7 years ago

The suggested solution would force nodes that only send one value to stay awake for longer than necessary, which would have a negative effect on battery life. That is something MySensors users likely would not want.

nufer commented 7 years ago

But don't you think, it is worse for a user if he does not get the values? If the problem is, that the gateway only can store 3 messages, this problem should be adressed by the framework?

btw. the Wait would only take place if the last sent message was sent less than x ms ago.

untested solution:

diff --git a/core/MyTransport.cpp b/core/MyTransport.cpp
index 5432ae8..da9f806 100644
--- a/core/MyTransport.cpp
+++ b/core/MyTransport.cpp
@@ -53,6 +53,7 @@ static uint8_t _transportToken = AUTO;
 // global variables
 extern MyMessage _msg;         // incoming message
 extern MyMessage _msgTmp;      // outgoing message
+extern unsigned long lastMessageMs;    // current Milliseconds of the last message sent

 #if defined(MY_RAM_ROUTING_TABLE_ENABLED)
 static routingTable_t _transportRoutingTable;          //!< routing table
@@ -512,6 +513,14 @@ bool transportAssignNodeID(const uint8_t newNodeId)

 bool transportRouteMessage(MyMessage &message)
 {
+       // wait MY_TRANSPORT_MESSAGE_DELAY (500ms) between messages. This prevents the loss of messages if the message
+       // buffer on the gateway is full
+       unsigned long currentMs = millis();
+       if (lastMessageMs+MY_TRANSPORT_MESSAGE_DELAY>currentMs) {
+               wait(lastMessageMs+MY_TRANSPORT_MESSAGE_DELAY-currentMs)
+       }
+       lastMessageMs=currentMs;
+
        const uint8_t destination = message.destination;
        uint8_t route = _transportConfig.parentNodeId;  // by default, all traffic is routed via parent node
mfalkvidd commented 7 years ago

The problem is not by the gateway. As mentioned in the thread I linked, the 3 message buffer size is how the nrf24 hardware is designed.

It would be great if this issue could be handled by the MySensors library, yes. But personally I would not like it to be handled by the library at the expense of my nodes' battery life.

With the current implementation, sketch developers have the option of adding wait statements, while others, that don't need to wait, need to do nothing.

In the cases where this issue has been discussed in the forum, the sketch developers that have raised the question have been satisfied with the current solution. So we have a win-win: Users not affected don't get a penalty and users who are affected can make tests and/or a judgement call and implement appropriate code.

With that said, I would love to see a solution that solves this issue by default for everyone. I am not sure how such a solution would look like or how it could be implemented without sacrificing too much flash or ram and without introducing code that is hard to maintain or has complex bugs.

All that is needed is someone (or a group of people) with enough interest in solving this particular issue, with sufficient time and knowledge.

trlafleur commented 7 years ago

I have the same basic issue.... Its a bit more complex when your using a radio like the RFM95 with large spreading factors where the data rate is slow and half duplex radio.

In 2.2.x beta version, the automatic re-send on negative-ack, will help in most case, but its still a work in progress and not 100% guarantee to get the message through.

I use a wait(SendDelay); of 500ms with SF7, 2000ms with SF12.

Many of my sensor are battery operated, but the data is sent only a few time a day, so I prefer to get the data over battery life.

I've been using this compound send format for my sends... its still NOT 100% guarantee, but work well enough...

 // Send the sketch version information to the gateway and Controller
  sendSketchInfo(SKETCHNAME, SKETCHVERSION, AckFlag);   wait(SendDelay);

  // Register this device as Water Moisture Sensor
  present(CHILD_ID1, S_MOISTURE, "Water Moisture", AckFlag);   wait(SendDelay);

 send(TextMsg.set(txtBuffer), AckFlag);  wait(SendDelay);        

as related to this... when you TX a message, and the radio has finish sending, it goes back to receive mode, if you send another message immediately after the first message, the receive window may NOT be long enough to receive the ACK from the gateway. So adding a longer "receive window" with the wait(SendDelay); help.

moskovskiy82 commented 7 years ago

Why not just bring in this option and have something like #define NODE_BATTERY_POWERED which will disable this customization and some others for node's sleep sake?

kisse66 commented 7 years ago

I also like to see something like this added. The library does a lot of management already (sometimes too much) and this should really be handled unless user so desires (configurable). Making it compile time choice can avoid all the extra code as well.

Personally I have implemented a wrapper on top of send, that stores timestamp as send is done. Before the next send it checks if there has been enough time in between and calls wait(timeleft) if not. This way single send does not need any extra wait and if the code spends otherwise enough time between (like reading slow sensor) it is avoided as well. Timestamp takes 4 bytes and the code adds a bit, but in my case not a real issue. One thing not yet hidden in my case is that the timestamp needs to be separately cleared when going to sleep as time will not go forward. In one node I have an asynhronous event timer firing sends. It gets really easu to use this "smart" send as it does not add any delay if it's not needed. Well, a few lines of code yes.

I'd not bind this with battery use, i.e. name it BATTERY something. I have a battery operated rain gauge with multiple sensors and it takes a few hundred ms to read them all. The "delayed" send above mostly avoids the extra delay since some sensors are slow. It wakes up seldomly, sends 7 values and I want the messages get through. Now it looks they almost always do. USE_SEND_DELAY, SMART_SEND_WAIT or something?

kisse66 commented 7 years ago

oops, the suggested delay did exactly this...

flatsiedatsie commented 5 years ago

This issue popped up again with the RF-Nano boards. https://forum.mysensors.org/topic/10327/rf-nano-nano-nrf24-for-just-3-50-on-aliexpress/25

It lead to a discussion about a 'radio recouperation' feature, which this issue already seems to be about. https://forum.mysensors.org/topic/10378/nrf24-radio-recouperation-feature.

It would be great if adding a delay between radio actions was possible, even if that would only be popular with non-battery-powered nodes.

mfalkvidd commented 5 years ago

Not 100% related, but a delay feature could help users to stay within the legal send time limits (duty cycle) when using LoRa radios. If a user is using SF12 they could set the delay to ~200 seconds to avoid using too much air time.