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

subsys: disk: ram: Make RAM disk size be configurable #13150

Merged
merged 2 commits into from
Feb 8, 2019

Conversation

pfalcon
Copy link
Contributor

@pfalcon pfalcon commented Feb 7, 2019

Hardcoded 96KB starts to overload RAM regions and fail CI tests.
Quick test shows that 80KB ramdisk is ok, (passes tests/posix/fs).
And of course, targets with wealth of RAM may want to use bigger
ramdisks.

Signed-off-by: Paul Sokolovsky paul.sokolovsky@linaro.org

@pfalcon pfalcon added area: File System area: Tests Issues related to a particular existing or missing test labels Feb 7, 2019
@pfalcon pfalcon added this to the v1.14.0 milestone Feb 7, 2019
@pfalcon
Copy link
Contributor Author

pfalcon commented Feb 7, 2019

Dunno how we survived for so long without configurable ramdisk. As mentioned, starts to fail CI for me, e.g. https://app.shippable.com/github/zephyrproject-rtos/zephyr/runs/33594/1/tests

Apparently, everyone was scared away by that comment that 96K is the min. I for sure look not for the 1st time into it. Turns out to be an urban legend.

@zephyrbot
Copy link
Collaborator

zephyrbot commented Feb 7, 2019

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

@pfalcon pfalcon force-pushed the ramdisk-configurable branch from 64f8450 to 11f0eca Compare February 7, 2019 19:14
@codecov-io
Copy link

codecov-io commented Feb 7, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@580de0f). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #13150   +/-   ##
=========================================
  Coverage          ?   48.97%           
=========================================
  Files             ?      317           
  Lines             ?    46587           
  Branches          ?    10751           
=========================================
  Hits              ?    22816           
  Misses            ?    19219           
  Partials          ?     4552
Impacted Files Coverage Δ
subsys/disk/disk_access_ram.c 80.76% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 580de0f...3afec03. Read the comment docs.

@@ -20,10 +20,10 @@
*/
#include "fat12_ramdisk.h"
#else
/* A 96KB RAM Disk, which meets ELM FAT fs's minimum block requirement. Fit for
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this comment should be deleted.
Is there a Kconfig which indicates the filesystem in use? Seems like if the size is made smaller than 96 the fs will be invalid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As written in the commit message, I tested tests/posix/fs with 80K and it passed. So, we either have an fs test which doesn't detect invalid fs, we have another fs rather than fatfs by default, or that comment isn't exactly right. Can make bets which is it ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As written in the commit message, I tested tests/posix/fs with 80K and it passed.

To prove the point, I cherry-picked that change from #12984, as it's going to hit on master soon anyway.

@pfalcon pfalcon force-pushed the ramdisk-configurable branch from 11f0eca to 86f5a7f Compare February 8, 2019 12:04
@pfalcon pfalcon requested a review from nniranjhana as a code owner February 8, 2019 12:04
Hardcoded 96KB starts to overload RAM regions and fail CI tests.
Quick test shows that 80KB ramdisk is ok, (passes tests/posix/fs).
And of course, targets with wealth of RAM may want to use bigger
ramdisks.

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
This test seems to be fine in the current master, but doing
development, it easily starts to overflow RAM of some boards and/or
crash due to stack checks. So, decrease the ramdisk (the biggest
eater of RAM here) from default 96K to 80K, and bump main stack
size proactively.

Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>
@pfalcon pfalcon force-pushed the ramdisk-configurable branch from 86f5a7f to 3afec03 Compare February 8, 2019 12:54
@nashif nashif merged commit 55ba23e into zephyrproject-rtos:master Feb 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: File System area: Tests Issues related to a particular existing or missing test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants