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_Scripting: only open the scripts directory on filesystems that support it #27404

Closed
wants to merge 2 commits into from

Conversation

andyp1per
Copy link
Collaborator

No description provided.

@andyp1per andyp1per added the BUG label Jun 27, 2024
@andyp1per andyp1per requested a review from IamPete1 June 27, 2024 21:03
@IamPete1
Copy link
Member

If we make this change I think we also need to narrow the scope of the error here:

#if !AP_FILESYSTEM_FILE_READING_ENABLED

It should be !AP_FILESYSTEM_FILE_WRITING_ENABLED && !defined(HAL_HAVE_AP_ROMFS_EMBEDDED_LUA)

@andyp1per
Copy link
Collaborator Author

If we make this change I think we also need to narrow the scope of the error here:

#if !AP_FILESYSTEM_FILE_READING_ENABLED

It should be !AP_FILESYSTEM_FILE_WRITING_ENABLED && !defined(HAL_HAVE_AP_ROMFS_EMBEDDED_LUA)

I'm not sure HAL_HAVE_AP_ROMFS_EMBEDDED_LUA needs to exist. We already have:

#define AP_FILESYSTEM_FILE_READING_ENABLED (AP_FILESYSTEM_FILE_WRITING_ENABLED || AP_FILESYSTEM_ROMFS_ENABLED)

So I think we could actually leave this and simply change HAL_HAVE_AP_ROMFS_EMBEDDED_LUA to AP_FILESYSTEM_ROMFS_ENABLED

@peterbarker
Copy link
Contributor

So I think we could actually leave this and simply change HAL_HAVE_AP_ROMFS_EMBEDDED_LUA to AP_FILESYSTEM_ROMFS_ENABLED

Someone may not want to embed LUA scripts but still want to have ROMFS (e.g. defaults.parm files)

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

You don't actually state what the bug is here.

I'm guessing that it's the fact it emits a warning, "Lua: open directory (%s) failed" on boards that can't opendir? Which boards/filesystems are they, BTW?

libraries/AP_Scripting/lua_scripts.cpp Outdated Show resolved Hide resolved
@andyp1per
Copy link
Collaborator Author

I'm guessing that it's the fact it emits a warning, "Lua: open directory (%s) failed" on boards that can't opendir? Which boards/filesystems are they, BTW?

Yes that's it - MambaH743v4 was the one I was testing

@peterbarker
Copy link
Contributor

I've created this alternative PR which takes a different approach: #28012

@tridge
Copy link
Contributor

tridge commented Sep 9, 2024

@andyp1per we have merged the alternative from @peterbarker - it's less likely to have unintended side effects

@tridge tridge closed this Sep 9, 2024
@andyp1per andyp1per deleted the pr-scripts-nodir-romfs branch September 10, 2024 12:41
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.

6 participants