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_ROMFS: Enable/fix mavlink ftp ListDirectory for @ROMFS top level #19865

Closed
wants to merge 1 commit into from

Conversation

pap9ck
Copy link

@pap9ck pap9ck commented Jan 24, 2022

The AP_ROMFS had a dir list method added a while back, but it does not work to show the top level files in the ROMFS. For example, the mavproxy command "ftp list @RomfS" or "ftp list @ROMFS/" doesn't work. The existing code expects a directory name after the backend @ROMFS/ string, e.g @ROMFS/models.

With the suggested change, "ftp list @RomfS" will return a list of all files in the romfs embedded file. The previous behavior is retained as well and "ftp list @ROMFS/models" will only list the files with a name starting with models/ and stripping the directory part.

@khancyr khancyr added the Library label Mar 7, 2022
Copy link
Contributor

@khancyr khancyr left a comment

Choose a reason for hiding this comment

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

That is interesting and usefull.
it is working as advertised.

I am not qualifed enough to review this thus.
The commit would need to be splitted and have the lib name added on the start of the commit message. Using Tools/gittools/git-subsystems-split would do it for you

@@ -61,6 +61,7 @@ const AP_Filesystem::Backend AP_Filesystem::backends[] = {
{ nullptr, fs_local },
#ifdef HAL_HAVE_AP_ROMFS_EMBEDDED_H
{ "@ROMFS/", fs_romfs },
{ "@ROMFS", fs_romfs },
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be nicer to fix it so we don't need to list it twice. Just list it as @RomfS and cope with the '/'
we should fix @sys in the same way too

@IamPete1
Copy link
Member

Closed by #26225

@IamPete1 IamPete1 closed this Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants