[MQTT] Wait until connection is established. #3518

gichan-jang · 2021-10-12T00:56:23Z

Change to wait until the connection is successfully finished within the timeout limit.

This patch fixes the error:
ERROR: MQTTSrc: cb_mqtt_on_connect_failure: failed to connect to the broker: TCP/TLS connect failure.

Test pipeline:
Publisher

$ gst-launch-1.0 videotestsrc is-live=true ! video/x-raw,format=RGB,width=640,height=480,framerate=5/1 ! mqttsink pub-topic=test/videotestsrc

Subscriber

$ gst-launch-1.0 mqttsrc sub-topic=test/videotestsrc ! video/x-raw,format=RGB,width=640,height=480,framerate=5/1 ! videoconvert ! ximagesink

Signed-off-by: gichan gichan2.jang@samsung.com

Self evaluation:

Build test: [ *]Passed [ ]Failed [ ]Skipped
Run test: [ *]Passed [ ]Failed [ ]Skipped

taos-ci · 2021-10-12T00:56:25Z

📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #3518. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://nnstreamer.mooo.com/.

again4you · 2021-10-12T01:20:42Z

gst/mqtt/mqttsink.c

@@ -551,6 +552,8 @@ gst_mqtt_sink_start (GstBaseSink * basesink)
  gchar *haddr = g_strdup_printf ("%s:%s", self->mqtt_host_address,
      self->mqtt_host_port);
  int ret;
+  guint timer = 0;
+  guint tick = DEFAULT_MQTT_SLEEP_TIME_US / 1000U;


In my opinion, tick would be DEFAULT_MQTT_SLEEP_TIME_US.
If tick is 1U (i.e.DEFAULT_MQTT_SLEEP_TIME_US / 1000U) , while loop could run 5000 times in case of worst case. If your intent, then it's OK.

Thanks. I changed g_cond_wait_until instead of sleep. Please check it again.

gst/mqtt/mqttsrc.c

taos-ci · 2021-10-12T01:41:44Z

cibot: @gichan-jang, A builder checker could not be completed because one of the checkers is not completed. In order to find out a reason, please go to http://nnstreamer.mooo.com/nnstreamer/ci/repo-workers/pr-checker/3518-202110121016160.95147395133972-d5d8e971f8e546955f3a893bf640896bb80d7791/.

dongju-chae · 2021-10-12T08:35:06Z

gst/mqtt/mqttsink.c

@@ -586,6 +589,16 @@ gst_mqtt_sink_start (GstBaseSink * basesink)
    return FALSE;
  }

+  /* Waiting for the connection */
+  while (!self->is_connected) {


Q: not communicating with cond?

Thanks, I updated it. Please check it again.

gichan-jang · 2021-10-14T05:18:26Z

The mqtt test was modified because the harness could not go to the playing state with the wrong parameter.

taos-ci

@gichan-jang, 💯 All CI checkers are successfully verified. Thanks.

taos-ci

@gichan-jang, 💯 All CI checkers are successfully verified. Thanks.

anyj0527

LGTM ✔

myungjoo · 2021-10-22T07:41:30Z

Questions..
Q1: what happens if TIMEOUT expires with client (subscribers)?
Q2: what happens if TIMEOUT expires with server (publishers)?
Q3: can you justify the answers (the behaviors) of Q1/Q2?
Q4: do you expect (novice) users will be able to "know" what's wrong when this TIMEOUT expires?

Change to wait until connection is successfully established within timeout limit. This patch fix the error: ERROR: MQTTSrc: cb_mqtt_on_connect_failure: failed to connect to the broker: TCP/TLS connect failure. Test pipeline: Publisher ```bash $ gst-launch-1.0 videotestsrc is-live=true ! video/x-raw,format=RGB,width=640,height=480,framerate=5/1 ! mqttsink pub-topic=test/videotestsrc ``` Subscriber ```bash $ gst-launch-1.0 mqttsrc sub-topic=test/videotestsrc ! video/x-raw,format=RGB,width=640,height=480,framerate=5/1 ! videoconvert ! ximagesink ``` Signed-off-by: gichan <gichan2.jang@samsung.com> Signed-off-by: Gichan Jang <gichan2.jang@samsung.com>

gichan-jang · 2021-10-22T08:27:41Z

Questions.. Q1: what happens if TIMEOUT expires with client (subscribers)?

If subscriber fails to connect to the broker within the TIMEOUT, it cannot change to playing state and stopped.
I changed to post error message and some guide.

"Failed to connect to MQTT broker from mqttsrc."
"Please check broker is running status or broker host address."

Q2: what happens if TIMEOUT expires with server (publishers)?

Same with subscribers.

Q3: can you justify the answers (the behaviors) of Q1/Q2?

Answered.

Q4: do you expect (novice) users will be able to "know" what's wrong when this TIMEOUT expires?

I add some guide with error message.

taos-ci

@gichan-jang, 💯 All CI checkers are successfully verified. Thanks.

myungjoo · 2021-10-25T01:47:13Z

Questions.. Q1: what happens if TIMEOUT expires with client (subscribers)?

If subscriber fails to connect to the broker within the TIMEOUT, it cannot change to playing state and stopped. I changed to post error message and some guide.
"Failed to connect to MQTT broker from mqttsrc."
"Please check broker is running status or broker host address."
Q2: what happens if TIMEOUT expires with server (publishers)?

Same with subscribers.

Q3: can you justify the answers (the behaviors) of Q1/Q2?

Answered.

Q4: do you expect (novice) users will be able to "know" what's wrong when this TIMEOUT expires?

I add some guide with error message.

It's ok with subscribers to fail with timeout when publishers do not exist.

However, I still don't understand why publishers need to fail with no subscribers.

We may proceed with this PR, but, publishers (and query servers) definitely need to be able to keep running (either 'paused' or 'dropping at sink') even if there are no subscribers (or query clients) with next PRs.

Imagine you are a server pipeline author and want to start an object detection service for arbitrary IoT devices in TV, Home Hub, or Phone. You don't want to launch pipelines when there is a IoT device connecting for the service; you want to keep running pipelines (server process) and let pipeline wait for incoming IoT devices.

gichan-jang · 2021-10-25T05:21:39Z

It's ok with subscribers to fail with timeout when publishers do not exist.

However, I still don't understand why publishers need to fail with no subscribers.

We may proceed with this PR, but, publishers (and query servers) definitely need to be able to keep running (either 'paused' or 'dropping at sink') even if there are no subscribers (or query clients) with next PRs.

Imagine you are a server pipeline author and want to start an object detection service for arbitrary IoT devices in TV, Home Hub, or Phone. You don't want to launch pipelines when there is a IoT device connecting for the service; you want to keep running pipelines (server process) and let pipeline wait for incoming IoT devices.

No.. Even without a subscriber, the publisher does not fail.
This change is about the connection to the brokers.
The reason why I made this PR is that the mqtt sink tried to publish the data before it was connected to the broker.
So mqtt elements is to check the connection before publish/subscribe.

gichan-jang requested review from again4you, anyj0527, dongju-chae, helloahn, jaeyun-jung, jijoongmoon, kimjh12, kparichay, leemgs, lhs8928, myungjoo, wooksong, zhoonit and a team as code owners October 12, 2021 00:56

gichan-jang force-pushed the mqtt/sync branch 2 times, most recently from eb5ce9c to d5d8e97 Compare October 12, 2021 01:16

again4you reviewed Oct 12, 2021

View reviewed changes

dongju-chae reviewed Oct 12, 2021

View reviewed changes

gichan-jang force-pushed the mqtt/sync branch from d5d8e97 to 8cb6923 Compare October 14, 2021 05:15

taos-ci approved these changes Oct 14, 2021

View reviewed changes

gichan-jang force-pushed the mqtt/sync branch from 8cb6923 to 9897f41 Compare October 14, 2021 10:18

taos-ci approved these changes Oct 14, 2021

View reviewed changes

jaeyun-jung approved these changes Oct 19, 2021

View reviewed changes

anyj0527 approved these changes Oct 20, 2021

View reviewed changes

gichan-jang force-pushed the mqtt/sync branch from 9897f41 to 415ca06 Compare October 22, 2021 08:27

taos-ci approved these changes Oct 22, 2021

View reviewed changes

myungjoo merged commit 6bb8673 into nnstreamer:main Oct 27, 2021

gichan-jang deleted the mqtt/sync branch October 28, 2021 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MQTT] Wait until connection is established. #3518

[MQTT] Wait until connection is established. #3518

gichan-jang commented Oct 12, 2021 •

edited

taos-ci commented Oct 12, 2021

again4you Oct 12, 2021 •

edited

gichan-jang Oct 14, 2021

taos-ci commented Oct 12, 2021

dongju-chae Oct 12, 2021 •

edited

gichan-jang Oct 14, 2021

gichan-jang commented Oct 14, 2021

taos-ci left a comment

taos-ci left a comment

anyj0527 left a comment

myungjoo commented Oct 22, 2021

gichan-jang commented Oct 22, 2021

taos-ci left a comment

myungjoo commented Oct 25, 2021 •

edited

gichan-jang commented Oct 25, 2021

[MQTT] Wait until connection is established. #3518

[MQTT] Wait until connection is established. #3518

Conversation

gichan-jang commented Oct 12, 2021 • edited

taos-ci commented Oct 12, 2021

again4you Oct 12, 2021 • edited

Choose a reason for hiding this comment

gichan-jang Oct 14, 2021

Choose a reason for hiding this comment

taos-ci commented Oct 12, 2021

dongju-chae Oct 12, 2021 • edited

Choose a reason for hiding this comment

gichan-jang Oct 14, 2021

Choose a reason for hiding this comment

gichan-jang commented Oct 14, 2021

taos-ci left a comment

Choose a reason for hiding this comment

taos-ci left a comment

Choose a reason for hiding this comment

anyj0527 left a comment

Choose a reason for hiding this comment

myungjoo commented Oct 22, 2021

gichan-jang commented Oct 22, 2021

taos-ci left a comment

Choose a reason for hiding this comment

myungjoo commented Oct 25, 2021 • edited

gichan-jang commented Oct 25, 2021

gichan-jang commented Oct 12, 2021 •

edited

again4you Oct 12, 2021 •

edited

dongju-chae Oct 12, 2021 •

edited

myungjoo commented Oct 25, 2021 •

edited