Serial connection support for Windows #375

julianoes · 2018-04-29T19:13:25Z

This adds support for serial connections on Windows.

Todos:

make it work on Windows.
remove comments that this is not supported.
fix add_any_connection to work on Windows.

This is mainly used to parse the URI inputs such as: - serial:///dev/ttyS0 - serial://COM3 - udp://0.0.0.0:14540

This should not change the DroneCore interface, however it makes it possible use UDP connections on other local interfaces than 0.0.0.0. Also, this gets rid of the default for serial connections because these tend to be different on every system.

hamishwillee · 2018-04-30T23:27:16Z

core/dronecore.h

+    ConnectionResult add_any_connection(const std::string &connection_url);
+
+    /**
+     * @brief Adds a UDP connection to the specified port number on 0.0.0.0 on any local interface.


Does "any local interface" mean any network? (ie I am wondering what this means as opposed to if you did not have this text)

"0.0.0.0" does means "listen on any interface". So "on any local interface" here does not add information, but explicits what "0.0.0.0" means.

Here it is from Wikipedia:

In the context of servers, 0.0.0.0 can mean "all IPv4 addresses on the local machine". If a host has two IP addresses, 192.168.1.1 and 10.1.2.1, and a server running on the host is configured to listen on 0.0.0.0, it will be reachable at both of those IP addresses.

hamishwillee · 2018-04-30T23:28:16Z

core/dronecore.h

     *
-     * @param local_port_number The local UDP port to listen to (defaults to 14540, the same as mavros).
+     * To accept only connections local connections of the machine, use 127.0.0.1, for any


"127.0.0.1, for any" - make that comma into a full stop and split into 2 sentences.

hamishwillee · 2018-04-30T23:29:26Z

core/dronecore.h

+     * incoming connections, use 0.0.0.0.
+     *
+     * @param local_bind_ip The local UDP ip to listen to.
+     * @param local_port The local UDP port to listen to (defaults to 14540, the same as mavros).


mavros is the package name, MAVROS is the product name. I would say this is an instance of MAVROS.

hamishwillee · 2018-04-30T23:32:25Z

core/log.h

@@ -50,6 +43,18 @@ class NullStream

 namespace dronecore {

+enum class Color {


This is all much nicer :-)

JonasVautherin

LGTM. I don't really understand the new #if defined(WINDOWS) part in core/serial_connection.cpp, but anyway I don't really understand how it works for the other platforms, so I cannot really comment on that part :-).

JonasVautherin · 2018-05-02T16:57:21Z

core/dronecore.h

+    ConnectionResult add_any_connection(const std::string &connection_url);
+
+    /**
+     * @brief Adds a UDP connection to the specified port number on 0.0.0.0 on any local interface.


"0.0.0.0" does means "listen on any interface". So "on any local interface" here does not add information, but explicits what "0.0.0.0" means.

Here it is from Wikipedia:

In the context of servers, 0.0.0.0 can mean "all IPv4 addresses on the local machine". If a host has two IP addresses, 192.168.1.1 and 10.1.2.1, and a server running on the host is configured to listen on 0.0.0.0, it will be reachable at both of those IP addresses.

julianoes · 2018-05-02T18:13:04Z

Thanks @hamishwillee, I went through your comments and tried to fix them.

hamishwillee · 2018-05-02T23:58:34Z

core/dronecore.h

-     * - Default Bind host IP is localhost(127.0.0.1)
-     *
-     * @warning Serial connections are not supported on Windows (they are supported on Linux and macOS).
+     * Default URL : udp://0.0.0.0:14540.


@julianoes The new interface must take a connection_url string so presumably this is no longer a default value!

Why have we chosen to not have a default anymore? Even if we choose not to have a default in the interface, why not default in the examples - passed through as argument?

If this stays the same, in addition to this update, we will need to change the usage instructions in the examples that use this method (since they also refer to default).

No, the host is not a must, see:

- TCP - tcp://[Remote_host][:Remote_port] - Serial - serial://[Dev_Node][:Baudrate] - Serial - serial://Dev_Node[:Baudrate]

Sorry, still confused. The prototype is this:

ConnectionResult add_any_connection(const std::string &connection_url);

There is no default value specified there for connection_url (e.g. it isn't like:)

ConnectionResult add_any_connection(const std::string &connection_url=SOME_DEFAULT);

so AFAIK in C++ to call this method you must specify a value for the URL right?. So how can there be a default?

Hm, you're right. There is no default. You need to specify it, and that shouild usually be done using a command line argument I suggest.

@julianoes Before I do that, this is now inconsistent - the other methods (e.g. for adding UDP) do have defaults. Can you either add the default back OR remote all the other defaults?

hamishwillee · 2018-05-03T00:17:51Z

core/dronecore.h

@@ -23,11 +23,10 @@ class System;
 class DroneCore
 {
 public:
-    static constexpr auto DEFAULT_UDP_CONNECTION_URL = "udp://:14540";
+    static constexpr auto DEFAULT_UDP_BIND_IP = "0.0.0.0";


@julianoes These should have doc strings (ideally)

I see, I'll add it next time.

Cheers. I mean "all of them should"

Serial connection support for Windows

julianoes added 5 commits April 29, 2018 15:03

log: support colors on Windows

5b17b85

follow_me: don't use internal defines of log

404ec4b

serial_connection: add support for Windows

6d83429

core: fixed style

3afbe78

serial_connection: fix build errors Linux

41fdc84

julianoes requested review from Rjasuja and shakthi-prashanth-m April 29, 2018 19:13

julianoes added the in progress label Apr 29, 2018

julianoes added 3 commits April 30, 2018 10:53

core: added class for CLI argument parsing

1d4a078

This is mainly used to parse the URI inputs such as: - serial:///dev/ttyS0 - serial://COM3 - udp://0.0.0.0:14540

core: use new CliArg class

b4bb9b7

This should not change the DroneCore interface, however it makes it possible use UDP connections on other local interfaces than 0.0.0.0. Also, this gets rid of the default for serial connections because these tend to be different on every system.

example: remove add_any_connection without args

3a1eb2b

julianoes requested a review from JonasVautherin April 30, 2018 20:47

julianoes changed the title ~~[WIP] Serial connection support for Windows~~ Serial connection support for Windows Apr 30, 2018

hamishwillee reviewed Apr 30, 2018

View reviewed changes

core/log.h

@@ -50,6 +43,18 @@ class NullStream

namespace dronecore {

enum class Color {

Copy link

Collaborator

hamishwillee Apr 30, 2018

Choose a reason for hiding this comment

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

This is all much nicer :-)

JonasVautherin approved these changes May 2, 2018

View reviewed changes

core: improve dronecore.h docs after review

5b8344f

julianoes merged commit 3459479 into develop May 2, 2018

julianoes deleted the windows-serial branch May 2, 2018 18:54

hamishwillee reviewed May 2, 2018

View reviewed changes

hamishwillee reviewed May 3, 2018

View reviewed changes

hamishwillee mentioned this pull request May 3, 2018

[WIP] Update UDP connections with windows support following 375 mavlink/MAVSDK-docs#117

Closed

julianoes mentioned this pull request May 16, 2018

Fix connection comments #394

Merged

hamishwillee mentioned this pull request May 18, 2018

Update connections documentation mavlink/MAVSDK-docs#124

Merged

mrpollo mentioned this pull request Jun 5, 2018

v1.0.0 Changelog scratchpad #417

Closed

rt-2pm2 pushed a commit to rt-2pm2/DronecodeSDK that referenced this pull request Nov 27, 2018

Merge pull request mavlink#375 from dronecore/windows-serial

3694820

Serial connection support for Windows

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serial connection support for Windows #375

Serial connection support for Windows #375

julianoes commented Apr 29, 2018 •

edited

Loading

hamishwillee Apr 30, 2018

JonasVautherin May 2, 2018

hamishwillee Apr 30, 2018

hamishwillee Apr 30, 2018

hamishwillee Apr 30, 2018

JonasVautherin left a comment

JonasVautherin May 2, 2018

julianoes commented May 2, 2018

hamishwillee May 2, 2018 •

edited

Loading

julianoes May 3, 2018

hamishwillee May 4, 2018

julianoes May 4, 2018

hamishwillee May 7, 2018

hamishwillee May 3, 2018

julianoes May 3, 2018

hamishwillee May 4, 2018

		@@ -50,6 +43,18 @@ class NullStream

		namespace dronecore {

		enum class Color {

Serial connection support for Windows #375

Serial connection support for Windows #375

Conversation

julianoes commented Apr 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonasVautherin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianoes commented May 2, 2018

hamishwillee May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianoes commented Apr 29, 2018 •

edited

Loading

hamishwillee May 2, 2018 •

edited

Loading