monstrenyatko / ArduinoMqtt

MQTT client for Arduino
MIT License
72 stars 13 forks source link

Correct use of mqtt->yield #13

Closed gjt211 closed 7 years ago

gjt211 commented 7 years ago

I have been testing this library for a few days (was using PubSubClient) and so far I really like it but don't understand the correct usage of mqtt->yield(30000L)

My problem is it introduces an unacceptable delay in my main loop.

Questions

  1. Is there a minimum time that should be specified?
  2. Is there a need to specify a time at all?
  3. Can I just call mqtt->yield(); and it will automatically yield for the required time?
  4. How often does this need to be called? Can I call it say once every 200mS?

Sorry for the many questions, I am trying to understand how it works so I can best write my code to accommodate the library.

Is there a way to manually send the keep alive ping?

FYI: I have temporarily commented out the debug output for the mqtt->yield() function in 'MqttClient.h' as I am logging my serial output trying to find rare & random reconnects that have been causing me headaches.

Thank you in advance.

monstrenyatko commented 7 years ago

Hi @gjt211 The yield is used to provide execution context to the library. While yield is executed the library processes incoming messages (subscriptions) and sends keep alive when required.

You can specify any value you like. The value in milliseconds. The default value is 1000 when you call it without parameter. The value 30000 is a just example value.

You must call the function regularly or when you are expecting the new incoming message and/or keep alive must be processed. Please take a look on getIdleInterval function. It could be helpful to estimate the interval until you need to call the yield next time.

You can't send keep alive manually right now. Do you think this functionality might be useful? The yield call sends it automatically when required.

Answers:

  1. No, if the default value (1000) is good for you
  2. No, see 1
  3. Yes, see 1
  4. Yes, you can call it once every few seconds or when your application has nothing to do.

I am working on the improvement of the yield call to make it optimized for very short values (See #12).

Plese, give me more info about your application loop desired behavior and timings. I'll try to help you to configure the library correctly.

monstrenyatko commented 7 years ago

@gjt211 If you don't need the library debug output just do not define the MQTT_LOG_ENABLED to 1. Debug is disabled by default.

gjt211 commented 7 years ago

Hi @monstrenyatko Thank you for your reply. I am sorry for asking questions here, I know this is not meant for support but for bugs etc but thank you for helping.

I noticed the default value of 1000mS in the yield() call inside your library later after I posted my original question. I was aware the 30000 value was just an example and I used it here in my question as such. I initially ran the demo code modified with a publish to my broker with yield(30000L) for 24 hours to see if I would get any reconnects which I did not.

My code does many things in the background also. Here is a list.

  1. Reads a temperature sensor every 6 seconds (DS18B20)
  2. Sends the (averaged) temperature value to broker every 60 seconds
  3. Sends the WiFi RSSI to the broker every 45 seconds
  4. Performs a httpupdate (OTA) check every 15 minutes
  5. Constantly monitors and debounces a switch input (every millisecond)
  6. Subscribes to a single topic that receives commands from the broker
  7. Received commands are used to control relay outputs and for setting slope * offset for temperature sensor and other things.

I actually want the debug output right now so I can monitor what is going on when the reconnect occurs. I commented out the Yield debug output as it was filling up my log with info I don't really need. I have implemented a timer that checks if 150 milliseconds have passed and yields for 50mS each time it happens. (In the last 12 hours I had reconnects between 1 and 20 from various units).

My main loop currently is this;

void loop() {
  // Check connection status
  if (!mqtt->isConnected()) {
    ts.disable(TASK_send_RSSI);      //Disable sendRSSI task
    ts.disable(TASK_send_TEMP);
    reconnect_mqtt();
  } else {
    //We are connected
    // MQTT yield call once every 150 mS
    if (millis() > next_mqtt_loop){
      mqtt->yield(50L);
      digitalWrite(__DATALED, _DATALEDON);
      delay(1);
      digitalWrite(__DATALED, _DATALEDOFF);
      next_mqtt_loop = millis() + 150;
    }

  }
  readSWIN();     //Check the switch input and perform debounce
  ts.update();          // Update the task scheduler
  updateRelay();      // Update the relay status if we received an 'timed-on' command

}

My readSWIN() function (debounce) is currently based on the number of times it is called (from previous code using other mqtt library) and I really need to change it to a time based function rather that a number of times function. That should reduce my dependance on delays in the main loop, but it won't solve them completely. It is possible that a switch gets turned on and off again within a second. I also have a function that has a counter input. It increments a count and needs to be able to read up to 100 pulses per second. I would need to alter this to use interrupts for it to work correctly.

Question, when a publish is sent, does the keep alive timeout get reset?

Here is a paste from the debug output of my code where a reconnect occurred. Note that my keep alive is 45 seconds and I reduced the 0.8 keep alive to 0.7 in your library to see if that would help with my reconnects.

MQTT - Keepalive, ts: 13004475
MQTT - Process message, type: 13
MQTT - Keepalive ack received, ts: 13004630
MQTT - Keepalive, ts: 13035726
MQTT - Publish, to: RSSI/5CCF7FACB85511319381, size: 4
---> [MQTT] RSSI/5CCF7FACB85511319381 => -39
MQTT Connecting
MQTT - Connect, clean-session: 1, ts: 13036296
MQTT - Wait for message, type: 2, tm: 9999 ms
MQTT - Process message, type: 2
MQTT - Connect ack received
MQTT - Connect ack, code: 0
MQTT - Keepalive interval: 31 sec
MQTT - Session is not present => reset subscription
 Success
MQTT - Subscribe, to: CMD/5CCF7FACB85511319381, qos: 0
MQTT - Wait for message, type: 9, tm: 9994 ms
MQTT - Process message, type: 9
MQTT - Subscribe ack received
MQTT - Publish, to: FWVER/5CCF7FACB85511319381, size: 18
---> [MQTT] FWVER/5CCF7FACB85511319381 => 2202017082403.bin
TASK send RSSI enabled
MQTT - Publish, to: OUT/5CCF7FACB85511319381/SW1, size: 2
---> [MQTT] OUT/5CCF7FACB85511319381/SW1 => 0
MQTT - Keepalive, ts: 13067489
MQTT - Process message, type: 13
MQTT - Keepalive ack received, ts: 13067644
MQTT - Publish, to: RSSI/5CCF7FACB85511319381, size: 4
---> [MQTT] RSSI/5CCF7FACB85511319381 => -39

The FWVER publish is used by my server for two things. It is only sent (published) when the device connects to the broker. One is so I know what version of firmware is running in each device, and secondly each time the server receives the firmware version it knows the device has reconnected and increments a counter so I can see how frequently there are reconnects.

FYI: My server is a dedicated (not VPS or shared) CentOS system located in my countries largest data centre with WHM & cPanel, Apache, php, mysql, mosquitto, etc. I have about 40 sensors (devices) connected and most of them are in remote locations and can not be physically accessed.

I am not sure if being able to send the keep alive manually would be useful in the long run. I only wanted the ability to manually set it so I could have control over when it was sent, hopefully to aid in my debugging process.

With point 2 from my original post, I now understand that if I don't pass it a value it will default to 1000mS. I really meant to ask what is the minimum time I could yield() for to keep everything working ok?

Sorry for the long post and appreciate your help and guidance.

gjt211 commented 7 years ago

Hi again @monstrenyatko

Maybe a simpler way to say what I would like with regard to yield() is as follows;

I would like to use mqtt->yield(0) in my main loop that loops say every 5mS. So mqtt->yield(0) only does it's stuff if there is something to do, and returns immediately if not.

Having a blocking delay while it's only waiting to see if something is coming in doesn't seem the best way to do things, unless I misunderstand how it's working (which is most likely!).

Further investigations after adding and using getIdleInterval shows me some strange behaviour. In my main loop from the post above where I call mqtt->yield(50L); after 150mS, I added the line Serial.println(mqtt->getIdleInterval()); and it prints out the time counting down from about 31000 milliseconds (which is correct based on 45 seconds * 0.7). Normally when it gets to 0, the keep alive is sent and the ACK is received, and the counter starts counting down again from about 31 seconds. What is strange however is if I publish a switch message (lets say at 5 seconds before the keep alive is due to be sent), the keep alive is NOT sent at the right time, but the reply from Serial.println(mqtt->getIdleInterval()); stays at zero for about 5 seconds after until the keep alive is actually sent.

Hopefully you can understand what I am trying to say.

monstrenyatko commented 7 years ago

@gjt211 keep alive depends on two timers. One of them tracks last TX message another one last RX message. Each TX and RX event resets the dedicated timer. If you always publish with QoS0 and never receives messages from the broker the RX timer will expire and trigger keep alive.

With the current implementation, the yield(0) will not be actually 0 because it depends on NET_MIN_TM_MS value. As I told you definitely need to take a look at the thread #12 . I am working on the improvement of the yield call to make it optimized for very short values like 0.

If the client is connected the getIdleInterval returns 0 in case the keep alive is sent and until the response is received from the broker. If the client is connected and getIdleInterval returns 0 you need to call yield to be able to receive the message. The keep alive processing is asynchronous and depends on yield call.