Document `mavsdk_connect` vs. `start_mavlink` about mavsdk-python HOT 17 CLOSED

I like those and they work. However, I do want to bring something else up. In Swift naming conventions we don't use _ and each function parameter has both an argument label and a parameter name. So, it would look something like this;

startMavlinkServer(forDrone droneAddress: String) {
    let activeDrone = droneAddress
}

Then we could call it like so;

let drone = connectToServer(serverAddress: "x.x.x.x")
drone.startMavlinkServer(forDrone: "x.x.x.x")

If you don't want to deal with the parameter and argument label then we can define a function like this;

startMavlinkServer(_ droneAddress: String) {
    let activeDrone = droneAddress
}

and call it like so;

drone.startMavlinkServer("x.x.x.x")

but this isn't "Swifty" human readable code, the reason for my original suggestion. Just something to keep in mind on the Swift side of things.

from mavsdk-python.

Right. So to give context:

• start_mavlink() starts mavsdk_server, which manages the MAVLink connection
• connect() instantiates the object that contains the different plugins

So you typically do:

start_mavlink()
drone = connect(host='127.0.0.1')

In Swift, we do:

startMavlink()
let drone = Drone()

So in Python, connect could become create_drone() or something like that, maybe?

@unipheas was suggesting startMavlinkConnectionToDrone() or startConnectionToDrone() instead of startMavlink().

Opinions here?

from mavsdk-python.

My suggestions is based on Swift guidelines for being "Swifty" (No relations to the song Schwifty).

from mavsdk-python.

@unipheas Thanks for taking the time to bring the Swift point of view - really appreciated. @JonasVautherin I don't think I can add anything else to this, but feel free to drag me back in if you need to.

from mavsdk-python.

As in other issue, I prefer System, but then I don't need to do the work of changing from Drone.

I quite like the mechanism you propose above, particularly if the two addresses have default values that mean you don't need to enter anything if you don't want.

from mavsdk-python.

But then should it be system_address instead of drone_address/mavlink_address?

from mavsdk-python.

Something else that would help with this is if the function prototypes and docstrings were more clear.

In [8]: mavsdk.connect?                                                                                                       
Signature: mavsdk.connect(*args, **kwargs)
Docstring:
Generates a dronecore instance with all available Core plugins registered
and ready to use
File:      /usr/lib/python3.7/site-packages/mavsdk/__init__.py
Type:      function

From the examples, I know a parameter I can pass is host="ipaddress", but what other parameters are supported? *args, **kwargs doesn't tell me much. And then I look at the code and and it just passes everything I give it to AsyncPluginManager (which does have a nice docstring and signature, so once I get there I can get some documentation at least).

Same goes for start_mavlink(). To learn how to connect to a serial port I had to dig up an old ticket. I feel like I should have just been able to look at start_mavlink's docstring to see the supported connection_url schemes and maybe a few examples.

I'd really like to see something more like:

In [11]: mavsdk.start_mavlink?                                                                                                
Signature: mavsdk.start_mavlink(connection_url=None)
Docstring: Starts a gRPC server in a subprocess connected to connection_url and listening on localhost:50051

:param connection_url - url defining the drone connection. Supported schemas:
     tcp://[address]:port
     udp://[address]:port
        if an address is not provided, connect to a drone on localhost
     serial://serial_port:[baud]
        if baud is not provided, autodetection is used
   Examples:
     tcp://192.168.0.168:14540  - connect to drone on 192.168.0.168:14540 using TCP
     udp://:14541                        - connect to drone on localhost:14541 using UDP
     serial://COM14                    - connect to drone on COM14
     serial:///dev/ttyUSB0           - connect to drone on /dev/ttyUSB0
File:      /usr/lib/python3.7/site-packages/mavsdk/__init__.py
Type:      function

Or if it's not reasonable to fully document in the docstring (ex: all schemes from some upstream project are supported), then at least a pointer to where this is documented would be nice. The pull request looks nice, but also doesn't have any dostrings, so I'm not sure it addresses the "document" aspect of this issue.

from mavsdk-python.

FWIW

1. This is pretty clear - can users see it in the docs/alongside the source?

   start_mavlink() - starts mavsdk_server, which manages the MAVLink connection
   

   • I prefer the existing name to startMavlinkConnectionToDrone et al - to me this just doubles the confusion because we have two methods that "connect"
   • You could spell it out by renaming to start_mavlink_server.
   • does this manage just one connection, or is it managing multiple connections?

2. This is accurate, but not very helpful for end users who probably would prefer to think about it as setting up a connection rather than the guts of plugins:

   connect() - instantiates the object that contains the different plugins
   

   would it be more useful to say something like....

   connect() - create connection to a drone at the specified endpoint
   

Assuming the backend server really just manages one connection, is it an option to hide the starting of the server - ie just have one method?

from mavsdk-python.

Assuming the backend server really just manages one connection, is it an option to hide the starting of the server - ie just have one method?

Not really: connect() connects you to mavsdk_server, while start_mavlink() starts mavsdk_server on your device/computer. But it could be that mavsdk_server is running, say, on the remote controller, and you don't need start_mavlink() at all: just set the IP of the remote controller in connect().

Also, for now mavsdk_server manages only one (client) connection because of a limitation in MAVSDK-C++, but it could actually manage multiple ones, e.g. 2 apps could connect to the same mavsdk_server. I think we will go there at some point.

from mavsdk-python.

OK. So it sounds like it is actually "start_mavlink_server" and "connect_to_server"? That sounds not quite right, because then how would the connection from the backend to the drone itself be configured.

from mavsdk-python.

So it sounds like it is actually "start_mavlink_server" and "connect_to_server"?

Precisely.

how would the connection from the backend to the drone itself be configured.

The only configuration needed is the connection url. Right now in Python you do start_mavlink(connection_url), and in swift you have one more call setMavlinkPort(connection_url); startMavlink() (where setMavlinkPort(connectionUrl) could be renamed to setMavlinkUrl(...), but that is what started this whole wording discussion on the swift side 😅).

So it could become start_mavlink_server(connection_url) and drone = connect_to_server(ip_of_mavsdk_server). And then you can do await drone.action.arm(), for instance.

from mavsdk-python.

So it could become start_mavlink_server(connection_url) and drone = connect_to_server(ip_of_mavsdk_server). And then you can do await drone.action.arm(), for instance.

I think naming things to reflect what they really are is better - so to me this would be a step in the right direction.

• But connection_url is overloaded. And does this need to be a URL or can it be a serial port name?
• Does the mavlink server have to be specified as an IP address/URL?

So something⁺⁺ like:

• start_mavlink_server(drone_address)
• drone = connect_to_server(server_address).

⁺⁺I say "something like" because I'm illustrating a point, not because I expect you to precisely use this text :-)

Does that make sense?

And ideally these should have sensible defaults :-)

from mavsdk-python.

• start_mavlink_server(drone_address)
• drone = connect_to_server(server_address).

Those make sense to me. And swift would have let drone = Drone(server_address) instead.

from mavsdk-python.

me too, though we need to make it clear the possible format of both those addresses, and if there are sensible defaults to assign them.

@unipheas - does this naming convention make things more clear for you?

from mavsdk-python.

In #105, I propose a new syntax where we only do:

drone = Drone()
drone.connect()

We can specify the mavsdk_server_address and the drone_address, too:

drone = Drone(mavsdk_server_address='192.168.1.15'
drone.connect(drone_address='192.168.1.24')

@julianoes suggests that we change drone_address for mavlink_address.

@hamishwillee: do you have an opinion there?

I opened another (related) issue to address 'drone' vs 'vehicle' vs 'system'.

from mavsdk-python.

Yes.

from mavsdk-python.

@pklapperich: I updated #105 following your comment. Feel free to review there 👍.

from mavsdk-python.