Skip to content

AP_Filesystem: added buffering API #30671

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tridge
Copy link
Contributor

@tridge tridge commented Jul 16, 2025

broken out from #30637

tridge added 2 commits July 17, 2025 08:23
this allows for aligned, buffered write access for logging purposes,
making writing log data from subsystems a lot more efficient
@tridge tridge force-pushed the pr-filesystem-buffer branch from f1e1245 to 079c5b4 Compare July 16, 2025 22:23
@tpwrules
Copy link
Contributor

Thinking out loud, another confounding factor is that we limit to 4K blocks to avoid using too much DMA memory in the driver. That would limit the maximum value of this buffer to 4K as well, so we may as well do that since it's not much bigger than 2K in the scheme of things.

Can we allocate DMA memory for e.g. the logger so we can use that directly?

@timtuxworth
Copy link
Contributor

Thinking out loud, another confounding factor is that we limit to 4K blocks to avoid using too much DMA memory in the driver. That would limit the maximum value of this buffer to 4K as well, so we may as well do that since it's not much bigger than 2K in the scheme of things.

Can we allocate DMA memory for e.g. the logger so we can use that directly?

Is this an H7 thing? A clean 32GB SD Card seems to use 32k blocks. I'd certainly give up RAM on an H7 to get more SD card speed! (could it be a parameter)?

const auto n2 = write(f->fd, f->writebuf, AP_FILESYSTEM_STDIO_WRITEBUF_SIZE);
f->writebuf_used = 0;
if (n2 != AP_FILESYSTEM_STDIO_WRITEBUF_SIZE) {
f->error = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you write errno here and in other places in this function

@andyp1per
Copy link
Contributor

Thinking out loud, another confounding factor is that we limit to 4K blocks to avoid using too much DMA memory in the driver. That would limit the maximum value of this buffer to 4K as well, so we may as well do that since it's not much bigger than 2K in the scheme of things.

Can we allocate DMA memory for e.g. the logger so we can use that directly?

My recollection is that it's a little more complicated than this down at the ChibiOS layer. We have a bypass for 4k chunks down in the SPI driver that avoids a bunch of ChibiOS copying that can significantly impact write performance. This was one of the last things I had to debug in the previous ChibiOS upgrade and it was very hard to find it because the code worked its just that the performance was significantly worse.

@tpwrules
Copy link
Contributor

It wasn't entirely clear to me, does that mean we can't do larger chunks?

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.

4 participants