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

AP_Filesystem: allow transfer of mission/fence/rally/param with mavftp #17102

Merged
merged 12 commits into from
Apr 20, 2021

Conversation

tridge
Copy link
Contributor

@tridge tridge commented Apr 5, 2021

This exposes a @mission filesystem for upload and download of mission/fence/rally and extends @param filesystem for parameter upload with ftp

mavproxy implementation: ArduPilot/MAVProxy#880

working:

  • mission download
  • mission upload
  • rally download
  • fence download
  • param upload

Copy link
Contributor

@magicrub magicrub left a comment

Choose a reason for hiding this comment

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

I breezed through it, looks straight-forward. I look forward to this getting merged!

@tridge tridge changed the title WIP: allow download of mission/fence/rally with mavftp AP_Filesystem: allow download of mission/fence/rally with mavftp Apr 7, 2021
@tridge tridge removed the WIP label Apr 7, 2021
@tridge tridge force-pushed the pr-mavftp-mission branch 2 times, most recently from b8a7c51 to 1998e2d Compare April 8, 2021 05:36
@tridge tridge changed the title AP_Filesystem: allow download of mission/fence/rally with mavftp AP_Filesystem: allow transfer of mission/fence/rally/param with mavftp Apr 8, 2021
@davidbuzz
Copy link
Collaborator

I've had a look through this, like @magicrub , and it looks pretty clean to me. I'd be more comfortable with all the low-level filesystem functionality being correct if it had auto-tests that exercised all the upload/download and compared the before-upload and the after-download, etc, but that's just me, i know auto-tests aren't really considered 'essential' at this point.

Copy link
Collaborator

@andyp1per andyp1per left a comment

Choose a reason for hiding this comment

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

Can't see anything obviously wrong! Two comments:

  • Lots of string manipulation, best to run through valgrind if you haven't already
  • Its quite a lot of code - conditional compile for 1Mb boards?

@tridge
Copy link
Contributor Author

tridge commented Apr 20, 2021

Its quite a lot of code - conditional compile for 1Mb boards?

what I'd like to do is make the whole mavlink ftp support conditional, and allow disable of it in the custom fw server

@tridge tridge requested a review from andyp1per April 20, 2021 07:22
@tridge
Copy link
Contributor Author

tridge commented Apr 20, 2021

Lots of string manipulation, best to run through valgrind if you haven't already

good call btw, there was a socket write of uninitialised memory, fixed now

@tridge tridge added the WikiNeeded needs wiki update label Apr 20, 2021
@tridge tridge merged commit 961e538 into ArduPilot:master Apr 20, 2021
@Hwurzburg Hwurzburg removed the WikiNeeded needs wiki update label May 31, 2021
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.

6 participants