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

dlt-receive: add reconnect option #620

Conversation

maiananthan
Copy link
Contributor

Issue

  • When dlt-receive is not connected to dlt-daemon, dlt-receive process is getting ended.

Fix

  • Added a option (-r) to reconnect dlt-daemon automatically.
  • Added a option value which can be mentioned as milli seconds as user input.

@minminlittleshrimp
Copy link
Collaborator

Hello @maiananthan
thank you.
So it's actually not a bug, but a feature/enhancement.
I'm fine with the point to hang the dlt-client like this, but could you kindly explain a little bit about the purpose?
IMO, dlt client should stop immediately when daemon down, since the pipe/socket closed and nothing further to send/flush to client.
Regards

@minminlittleshrimp
Copy link
Collaborator

Hello @maiananthan thank you. So it's actually not a bug, but a feature/enhancement. I'm fine with the point to hang the dlt-client like this, but could you kindly explain a little bit about the purpose? IMO, dlt client should stop immediately when daemon down, since the pipe/socket closed and nothing further to send/flush to client. Regards

Sorry not reading carefully, so I suppose I am a user and would rather the client side stay there when I do something like reboot daemon, I think it is good to hang awhile. My concern is:

  1. What is the timeout, client should not stay forever!!!?
  2. This is only applied for dlt-receive, so we cannot say that this is applicable for other clients, so in fact this not a bug, but enhancement for dlt-receive

I am fine with merging this patch. Let see @michael-methner opinion.

// if reconnect flag is enabled, wait for mentioned milli
// seconds
poll(NULL, 0, dltdata.rvalue);
continue;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the loop stop here? If not, the loop goes on forever!

@minminlittleshrimp
Copy link
Collaborator

I suggest my fix, which I applied and checked that it works:

$ git df
diff --git a/src/console/dlt-receive.c b/src/console/dlt-receive.c
index ad42e42..58bc0b3 100644
--- a/src/console/dlt-receive.c
+++ b/src/console/dlt-receive.c
@@ -637,14 +637,15 @@ int main(int argc, char *argv[])
                 // if reconnect flag is enabled, wait for mentioned milli
                 // seconds
                 poll(NULL, 0, dltdata.rvalue);
-                continue;
-            }
-            else {
-                /* Dlt Client Cleanup */
-                dlt_client_cleanup(&dltclient, dltdata.vflag);
+                //continue;
                 break;
             }
         }
+        else {
+            /* Dlt Client Cleanup */
+            dlt_client_cleanup(&dltclient, dltdata.vflag);
+            break;
+        }
     } while (dltdata.rflag == 1 && poll(NULL, 0, dltdata.rvalue) == 0);
 
     /* dlt-receive cleanup */

@@ -162,6 +168,7 @@ void usage()
printf(" -R Enable resync serial header\n");
printf(" -y Serial device mode\n");
printf(" -u UDP multicast mode\n");
printf(" -r msecs Reconnect server with milli seconds specified\n");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the wording more precise: "Reconnect to server after milli seconds"

break;
}
}
} while (dltdata.rflag == 1 && poll(NULL, 0, dltdata.rvalue) == 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is somehow duplicated to the poll instruction before. You could use a while(1) instead.

I would prefer sleep() or nanosleep() instead of the poll to implement a sleep function.

@minminlittleshrimp
Copy link
Collaborator

Hello @maiananthan
Kindly update the patch for review and merge.
Thank you a lots.

@duvanan13
Copy link
Contributor

Duplicated by #640

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants