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

examples/filesystem: Update README and increase default stack size #20439

Merged
merged 2 commits into from
Mar 1, 2024

Conversation

crasbe
Copy link
Contributor

@crasbe crasbe commented Feb 28, 2024

Contribution description

This pull request updates the filesystem example. In a previous commit e657590 the behaviour of the filesystem example was changed, but the README was not updated, making it hard for new users to apply it to an actual board.
Therefore, the README was extended.

Furthermore, the Makefile was extended to add an option to increase stack size for larger flash memories to avoid Kernel Panics due to a stack overflow.

Testing procedure

The changes were tested with a Nordic Semiconductor nRF52840-DK development board. The behaviour of the native implementation is unchanged and reflects the behavior shown in the README.

The behaviour with a real board should equally reflect the behavior shown in the README in the section about the nrf52840dk board.

Issues/PRs references

fixes #20373

@crasbe crasbe requested a review from jia200x as a code owner February 28, 2024 13:48
@github-actions github-actions bot added Area: doc Area: Documentation Area: examples Area: Example Applications labels Feb 28, 2024
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Thank you for taking care of this!
Some notes, as a presence of a MTD device is not enough, we also need a mountpoint.

examples/filesystem/README.md Outdated Show resolved Hide resolved
examples/filesystem/README.md Outdated Show resolved Hide resolved
examples/filesystem/README.md Outdated Show resolved Hide resolved
examples/filesystem/README.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@crasbe crasbe left a comment

Choose a reason for hiding this comment

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

Thank you for the feedback, I think all suggestions are now applied to the PR.

@crasbe
Copy link
Contributor Author

crasbe commented Feb 28, 2024

I think the only remaining failed tests are the commit tests now, which require the commits to be sqashed. But the guidelines say to not do that unless requested by a maintainer. So should I do anything beyond this point?

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Thank you, please squash directly.

examples/filesystem/README.md Outdated Show resolved Hide resolved
examples/filesystem/README.md Outdated Show resolved Hide resolved
@crasbe crasbe changed the title Update Filesystem Example README to reflect actual behaviour examples/filesystem: Update README and increase default stack size Feb 29, 2024
@benpicco benpicco added the CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs label Feb 29, 2024
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 29, 2024
@riot-ci
Copy link

riot-ci commented Feb 29, 2024

Murdock results

✔️ PASSED

4b33e3e examples/filesystem: Increase main stack size

Success Failures Total Runtime
1 0 1 11s

Artifacts

@benpicco benpicco added this pull request to the merge queue Feb 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 29, 2024
@crasbe
Copy link
Contributor Author

crasbe commented Feb 29, 2024

The build fails because of the "nucleo-l011k4" target: https://ci.riot-os.org/details/3a64dfbefb1a493abb7fce7dd25ced4e/builds/examples:filesystem

The Makefile.include specifies the stack size to 512 Bytes, which is less than would be required for the filesystem example (at least with littlefs). https://github.com/RIOT-OS/RIOT/blob/master/boards/nucleo-l011k4/Makefile.include#L5
The STM32L011K4 microcontroller only has 2kB of RAM, so it is likely that this example won't run either way on it.

I don't know why the target is not ignored by the CI, it is present in the Makefile.ci exclusion list for insufficient memory already: https://github.com/RIOT-OS/RIOT/blob/master/examples/filesystem/Makefile.ci#L9
All other targets with insufficient memory are built successfully.

Other examples redefine the stack size as well, but they define THREAD_STACKSIZE_MAIN. Perhaps that's what this example should do as well?

@crasbe
Copy link
Contributor Author

crasbe commented Feb 29, 2024

I can confirm that the example still works on the Nordic nRF52840-DK with THREAD_STACKSIZE_MAIN=2048 instead of THREAD_STACKSIZE_DEFAULT=2048.
The stack size of main is only 128 bytes bigger when using THREAD_STACKSIZE_DEFAULT.

With THREAD_STACKSIZE_MAIN=2048;

> ps
	pid | name                 | state    Q | pri | stack  ( used) ( free) | base addr  | current     
	  - | isr_stack            | -        - |   - |    512 (  164) (  348) | 0x20000000 | 0x200001c8
	  1 | main                 | running  Q |   7 |   2048 ( 1424) (  624) | 0x20000400 | 0x20000b84 
	    | SUM                  |            |     |   2560 ( 1588) (  972)

With THREAD_STACKSIZE_DEFAULT=2048:

> ps
	pid | name                 | state    Q | pri | stack  ( used) ( free) | base addr  | current     
	  - | isr_stack            | -        - |   - |    512 (  164) (  348) | 0x20000000 | 0x200001c8
	  1 | main                 | running  Q |   7 |   2560 (  676) ( 1884) | 0x20000400 | 0x20000d84 
	    | SUM                  |            |     |   3072 (  840) ( 2232)

@@ -32,4 +32,8 @@ USEMODULE += constfs
# Enable to automatically format if mount fails
#USEMODULE += vfs_auto_format

# For LittleFS on real devices, the default stack size has to be
# increased:
CFLAGS += -DTHREAD_STACKSIZE_DEFAULT=2048
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right it’s actually enough to bump the main thread stack since that’s the only one doing file operations. (Ok, it’s probably the only thread in this example, but still)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like this fixed it. Nice :)

crasbe and others added 2 commits February 29, 2024 23:43
Reflect the updated behavior of the filesystem example and added more examples on the usage on real boards.

Remove the remarks about mtd and MTD_0.

Co-Authored-By: benpicco <benpicco@googlemail.com>
The stack size for the main thread is insufficient for some real targets to run this example, which leads to kernel panics in the MEM MANAGE HANDLER when trying to access the Flash
@benpicco benpicco added this pull request to the merge queue Mar 1, 2024
Merged via the queue into RIOT-OS:master with commit 1966a20 Mar 1, 2024
26 checks passed
@crasbe crasbe deleted the patch-1 branch March 1, 2024 09:15
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: doc Area: Documentation Area: examples Area: Example Applications CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

README in Filesystem example is inconsistent with actual code
4 participants