Mavlink FTP is too slow

[DrTon]

Download speed via USB is only ~10kbytes/s, this makes it completely useless for logs download. Using fetch_log.py I got ~250kbytes/s, not very fast but usable. Need to think how to improve download speed. On of the problem I see is that ALL encapsulated data packets (even ACKs) have the same size. Need to use variable size encapsulated data packets or introduce new messages for FTP.

[LorenzMeier]

Your analysis is missing the order of magnitude. The packet size is not the issue at all. What needs to be done is to send new packets while the old ones are not acked yet, so that you have multiple in flight.

[DrTon]

I.e. you mean that wasting CPU to receive dummy stream is ok? What about sending packets, I will try to write console FTP tool and play with it. As I understand it can be done on ground side, no changes on firmware side required?

[LorenzMeier]

No, I never would advise against efficiency 8). I'm just trying to direct our (limited) resources towards optimising the first 95% of performance (= timing), rather than the remaining 5% (= packet size). Note that the overhead per packet is not just bytes, its a context switch and other overhead (on both sides, on PX4 and on the computer). So reducing the packet size a lot will not win you much, because copying bytes and transmitting them via USB is reasonably fast compared to the latency which is currently limiting the bandwidth.

The main issue right now is that the autopilot is right now doing nothing most of the time, because he only replies with one packet per request. Which means that the defining factor for performance is the time it takes to send one packet from the autopilot and wait until the ack from the GCS comes back.

If the autopilot instead just keeps sending packets, we can actually saturate the link (and should get equal or more to your 250 kB/s. We still can win some efficiency with variable length packets - but compared to the timing win this will most likely negligible.

A standalone C library / command line client would be for sure a good example / starting point. If you do it, it would be most appreciated if you confine it to using POSIX primitives, so that it can be easily ported.

However: The client within QGroundControl is pretty self contained, so it would be mostly for your own desire to learn and not so much a requirement to prototype protocol improvements.

[DrTon]

I see one more problem when we will request packets too fast: FTP uses messages buffer in mavlink app, that can only contain 2 packets. Also file reading itself performed in low prio worker. I.e. each packet handled by 3 (!) threads: mavlink_recv, lpwork, mavlink_main. It would be good to reduce this number. File reading must be done in separate thread to avoid mavlink threads blocking, agree. But why not to send packets directly from lpwork thread to avoid using messages buffer? I don't see anything bad here if we implement thread-safe send_message. Or I missed something?

[LorenzMeier]

The lpwork thread doesn't have a valid file handle, so you would need to implement a thread safe send routine, which is the equivalent of the message buffer (assuming the buffer runs at a decent timing / rate). Apart from cross checking that everything works as intended and we have no timing issues there is really no benefit in creating another buffer just for this use case.

[LorenzMeier]

@DrTon On the buffer argument: It would be good to check how much space is left before copying a message there and backing off and rescheduling if necessary. I would guess that's not done yet. Once its done, we can essentially send at the maximum rate.

[DrTon]

The lpwork thread doesn't have a valid file handle

Ah, you mean uart port handle? I forgot about this... :(

[LorenzMeier]

There is really no issue in using the message buffer. It's there for sends from a different thread and it can be emptied at link speed, so we shouldn't have an issue there.

[DonLakeFlyer]

FYI: I'm back home tomorrow. I can start to look at speeding this up in a couple days once I catch up from being gone so long.

On Jul 30, 2014, at 8:22 AM, "Lorenz Meier" notifications@github.com wrote:

There is really no issue in using the message buffer. It's there for sends from a different thread and it can be emptied at link speed, so we shouldn't have an issue there.

— Reply to this email directly or view it on GitHub.

[DrTon]

There is really no issue in using the message buffer.

The issue with message buffer is that it emtyed by main mavlink thread at some constant/limited rate. To get more speed we have to increase buffer size to few kbytes and review buffer emptying logic to send multiple messages at iteration, or use semaphores to send immediately when buffer is not empty.

[LorenzMeier]

Yes - that was a consequence of our (my fault, I started with this design pattern) poor-mans QoS 8). Now that we work with the actual bandwidth, we can empty the message buffer essentially immediately whenever we have some free bandwidth on the link, up to a maximum usage of e.g. 30%. This should happen at the same location as the param and mission items handling is done, and has essentially the same requirements. If we change the main loop of the mavlink app to a blocking wait on a mutex with timeout, we can ensure to run with a minimum rate (for normal telemetry), and to run immediately if something else is ready (FTP, params or waypoints). However, this should be done after the current rework and in a separate PR, as its orthogonal.

How far are we from being able to merge the current rework?

[DrTon]

Yes, of course, current rework is already large.

[DonLakeFlyer]

Trying to catch up on 3 weeks worth of email. I think the rework being talked about is to handle the telemetry rates more dynamically depending on throughput right? If so, can someone let me know when this is done (is there a PR # for it?) and then I can implement a more streaming oriented protocol for file download instead of the current ack/response protocol.

On Jul 30, 2014, at 3:31 AM, Anton Babushkin notifications@github.com wrote:

Yes, of course, current rework is already large.

— Reply to this email directly or view it on GitHub.

[LorenzMeier]

It's merged to master, so you can code away 8)

[LorenzMeier]

@DonLakeFlyer Any news on this? Keeping multiple packets in flight + showing a progress bar would allow to turn this from a developer into a user feature.

[DonLakeFlyer]

I was waiting to here back from this:

I’m about to begin more work on FTP additional support. Specifically implement sequence numbers as well as a more streaming file download mechanism to improve speed. Along the way I want to restructure the code so that it doesn’t have as much code in the header files. Before I do that I’d like to write a unit test for this code, since it is getting quite complex. In order to do that I'd like to use a pattern where the base class for the object you are communicating with is an interface (abstract base class). This in turn allows you to write mock implementation of the object, which you can then connect to the code you are trying to test.

In this specific case it would mean creating a MavlinkInterface abstract base class, which is then the parent of the Mavlink class. I then can implement a mock Mavlink class which I pass to the FTP code to test against. As far as functionality it would not change anything functionally with the existing Mavlink code. It would do two things though:

1) Mavlink objects would be slightly larger due to vtable size 2) Calling Mavlink methods would be very slightly slower since they would be vtable instead of direct calls

Wanted to run this by folks before I started work to make sure it was ok. I could certainly hack in a different mechanism to the FTP code to test it, but that ends up being more invasive. Also by doing it this way it makes the MavlinkInterface available for others to write mock implementations to test other parts of the system as well. So it becomes a useful generic change.

Should I proceed?

On Aug 8, 2014, at 4:32 AM, Lorenz Meier notifications@github.com wrote:

@DonLakeFlyer Any news on this? Keeping multiple packets in flight + showing a progress bar would allow to turn this from a developer into a user feature.

— Reply to this email directly or view it on GitHub.

[LorenzMeier]

I think there is no way to give a good answer here without an actual quantitative test of flash and performance impact. Yes, we are sensitive to both, in particular for large code pieces. No, just a tiny overhead wouldn't be too bad. I have one question though: If you only compile the mock class in the test makefile (px4fmu-v2_test config), is GCC smart enough to strip the vtable and directly use the only implementation? Would you know a way to validate this?

[DonLakeFlyer]

I don’t think GCC would be smart enough to do that. I should be able to measure both size increase as well as performance impact fairly easily. How about I do the conversion, measure things and report back?

On Aug 8, 2014, at 3:29 PM, Lorenz Meier notifications@github.com wrote:

I think there is no way to give a good answer here without an actual quantitative test of flash and performance impact. Yes, we are sensitive to both, in particular for large code pieces. No, just a tiny overhead wouldn't be too bad. I have one question though: If you only compile the mock class in the test makefile (px4fmu-v2_test config), is GCC smart enough to strip the vtable and directly use the only implementation? Would you know a way to validate this?

— Reply to this email directly or view it on GitHub.

[LorenzMeier]

I don't want to send you off into a time consuming experiment - if you feel its done quickly then yes, please measure the binary size and CPU load of the mavlink app

• binary size: http://pixhawk.org/dev/runtime_and_ram_optimization
• CPU usage: "top" in NSH

[DonLakeFlyer]

Can you point at a place to start looking at the dynamic telemetry throughput code? Just need a starting point then I can pull the thread from there.

On Aug 8, 2014, at 3:41 PM, Lorenz Meier notifications@github.com wrote:

I don't want to send you off into a time consuming experiment - if you feel its done quickly then yes, please measure the binary size and CPU load of the mavlink app

binary size: http://pixhawk.org/dev/runtime_and_ram_optimization CPU usage: "top" in NSH — Reply to this email directly or view it on GitHub.

[LorenzMeier]

Its pretty simple and here: https://github.com/PX4/Firmware/blob/master/src/modules/mavlink/mavlink_main.cpp#L1160

It just checks if the bandwidth is sufficient for all streams and scales them down linearly if needed. This might benefit from additional smarts.

[DrTon]

Yes, but to use this feature, need to use MavlinkStream API, to report estimated "send size per update" (stream->get_size()) for stream. But rate of FTP packets controlled first of all by ground station, so I'm not know how to do this better.

[DonLakeFlyer]

@julianoes You modified QGCUASFileManager to check for compid != MAV_COMP_ID_IMU so that it wouldn't process video messages as FTP messages. The problem is that FTP sends on component id 50 from the firmware. It just calls _mavlink->get_component_id. I'm not very educated on the ins and outs of component ids. There is not a #define for component id 50. If I hardcode the QGCUASFileManager check to 50, will that break video?

[julianoes]

Hm, did that slip to master? I thought I only tried that to debug stuff...

[DonLakeFlyer]

@julianoes I believe I updated the UAS code to handle not thinking it has an image when there are ftp packets by making sure that UAS::imagePackets is initialized to 0. This will only be bumped to a value when the image transmission handshake happens. Then the code which processes encapsulated data in UAS doesn't do anything in imagePackets is 0. So I think the UAS side is correct.

There is a problem is in the QGCUASFileManager side: It's missing the magic byte check which is letting video through to the FTP code. That said I don't think the magic byte check is enough to keep all image data from bleeding through to the FTP code. I'd imagine image data could at some point start with an 'f' byte.

So the question is: Does the image data come through on a different component id such that you can depend on that to filter it out of the QGC FTP code.

[LorenzMeier]

@DonLakeFlyer It does come from a different component ID, yes.

[DonLakeFlyer]

Ahh, good. So I can prevent things from bleeding through. Hopefully last question on this subject. FTP is coming through on component ID 50, but there is no #define for id 50 in MAV_COMPONENT enum in mavlink. Shouldn't there be? Don't want to hardcode a magic number.

[LorenzMeier]

@DonLakeFlyer @julianoes Could you please for now work around this on local branches and I'll bring up a complete spec for image transmission and FTP with proper message support? I'd like to keep improving FTP in parallel, but please do not bother / get sidetracked with resolving this. What we need is to move to this message or create a similar specific FTP message: https://pixhawk.ethz.ch/mavlink/#V2_EXTENSION

@DonLakeFlyer What would you prefer? I want to make it variable length anyway.

[DonLakeFlyer]

I've got something hacked together which I think works. I think specific messages for both types would be better. Once I work my way past streaming download I could add the new FTP message if you want. I'm about to push sequence number support, file sizes on directory list, an file size coming back in open command in prep for download progress indicator (both QGC and Firmware changes). After that I'll do download progress indicator with speed. And then last streaming download.

[LorenzMeier]

This has been addressed with a 30x speedup, which is about the max you can get through the USB interface we're using.