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

Add AP_Filesystem API #11936

Merged
merged 17 commits into from Aug 27, 2019
Merged

Add AP_Filesystem API #11936

merged 17 commits into from Aug 27, 2019

Conversation

tridge
Copy link
Contributor

@tridge tridge commented Aug 1, 2019

This adds an AP_FIlesystem API for all filesystem operations. It cleans up the existing posix abstraction in ChibiOS, and will enable us to add automatic remount on operation failures
A new parameter LOG_FILE_TIMEOUT is added which gives the timeout in seconds for log writes. Setting this to 30 allows you to remove the card for 30s and when re-inserted logging will continue.
This is an alternative to #8639 from @peterbarker

Issues:

  • adds about 1.7k to flash for unknown reason (fixed, is now smaller than master)
  • add automatic remount on IO error

@tridge
Copy link
Contributor Author

tridge commented Aug 1, 2019

@WickedShell please check if the AP_Scripting changes are OK (also need to check it all works).
Note that this fixes the recursive opendir issue

@tridge tridge force-pushed the pr-ap-filesystem branch 3 times, most recently from 15a9e51 to bf463cc Compare August 2, 2019 10:58
@tridge tridge force-pushed the pr-ap-filesystem branch 3 times, most recently from 611a708 to a967ec8 Compare August 3, 2019 07:39
@tridge
Copy link
Contributor Author

tridge commented Aug 4, 2019

this was flight tested today on a bixler

Copy link
Contributor

@WickedShell WickedShell left a comment

Choose a reason for hiding this comment

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

This breaks scripting on real boards. Works fine in SITL, but on a CubeBlack build the same script which previously worked now dies with unexpected symbol on the first line when parsing the script. I haven't gotten to investigate it yet, but it looks like it's probably reading corrupt data off the SD card. It's also notable that scripting is still implicitly using remove, and getting away with it, which is probably not correct with the AP_FS abstraction. The sample script I used was this:

function loop ()
  gcs:send_text(0, "LOOPING")
  file = io.open("test.lua", "r")
  gcs:send_text(0, file:read("*all"))
  file:close()
  return loop, 1000
end

file = io.open("test.lua", "a")
-- sets the default output file as test.lua
io.output(file)

-- appends a word test to the last line of the file
io.write("-- End of the test.lua file")

io.close(file)

return loop, 1000

@tridge
Copy link
Contributor Author

tridge commented Aug 9, 2019

@WickedShell ok, thanks. I'll fix this soon

@tridge
Copy link
Contributor Author

tridge commented Aug 15, 2019

@WickedShell I've now got it working with Lua scripts, and also now have a common FILE implementation between SITL and ChibiOS, so valgrind does useful checking of Lua IO calls

@@ -383,8 +383,8 @@ bool AP_Proximity_LightWareSF40C::process_reply()
case RequestType_Health:
// expect result in the form "0xhhhh"
if (element_len[0] > 0) {
int result;
if (sscanf(element_buf[0], "%x", &result) > 0) {
long int result = strtol(element_buf[0], nullptr, 16);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure why this change to the proximity sensor is included in the AP_Filesystem PR.? I haven't looked into the implications of the change from sscanf to strtol, and it's probably a harmless change, but thought I'd note it here anyway.

@davidbuzz
Copy link
Collaborator

I've read through all the code changes in the AP_Filesystem PR, and had just one comment that I added above. everything else looks like it's a pretty straight-forward relocation of the existing filesystem functions into the AP_FS:: namespace without too much to worry about.
... although it does touch a lot of files.

@tridge
Copy link
Contributor Author

tridge commented Aug 27, 2019

merged after discussion with mdb

@tridge tridge merged commit d40835c into ArduPilot:master Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants