Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sending multiple messages in a row breaks the transmition #892

Open
nufer opened this issue Jul 14, 2017 · 12 comments

Comments

Projects
7 participants
@nufer
Copy link

commented Jul 14, 2017

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 mfalkvidd added the question label Jul 14, 2017

@mfalkvidd

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2017

@nufer

This comment has been minimized.

Copy link
Author

commented Jul 14, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2017

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

@mfalkvidd

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2017

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

This comment has been minimized.

Copy link
Author

commented Jul 14, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2017

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

This comment has been minimized.

Copy link

commented Jul 17, 2017

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

This comment has been minimized.

Copy link

commented Aug 9, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2017

oops, the suggested delay did exactly this...

@fallberg fallberg added this to To do in 2.4.0 via automation Feb 25, 2018

@flatsiedatsie

This comment has been minimized.

Copy link

commented May 9, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

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.

@mfalkvidd mfalkvidd removed the question label May 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.