Skip to content

picolibc: enable read()/write() with !VFS, implement missing fs functions#14843

Merged
benpicco merged 2 commits intoRIOT-OS:masterfrom
benpicco:picolibc_stdio_offset
Sep 11, 2020
Merged

picolibc: enable read()/write() with !VFS, implement missing fs functions#14843
benpicco merged 2 commits intoRIOT-OS:masterfrom
benpicco:picolibc_stdio_offset

Conversation

@benpicco
Copy link
Contributor

@benpicco benpicco commented Aug 24, 2020 •
edited
Loading

Contribution description

Picolibc does not implement read()/write() on it's own. We already have them implemented for the VFS adaption, but
the read()/write() functions still work without VFS when reading/writing to stdin, stdout or stderr.

This also provide the remaining functions necessary to build tests/pkg_fatfs_vfs with PICOLIBC.

Testing procedure

make -C examples/gnrc_networking BOARD=samr21-xpro PICOLIBC=1 -j

works and the output of ping6 looks normal.

make -C tests/pkg_fatfs_vfs BOARD=samr21-xpro PICOLIBC=1 -j

should now also work.

Issues/PRs references

alternative to #14841

@benpicco benpicco requested review from bergzand and maribu August 24, 2020 21:57
@benpicco benpicco added Area: fs Area: File systems Area: sys Area: System Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 24, 2020
@benpicco benpicco requested a review from kaspar030 August 24, 2020 21:59
@benpicco benpicco force-pushed the picolibc_stdio_offset branch from e4930d5 to 2512c7f Compare August 24, 2020 22:01
maribu
maribu reviewed Aug 25, 2020
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

Some comments inline

@benpicco benpicco force-pushed the picolibc_stdio_offset branch 2 times, most recently from 7526304 to 21988b5 Compare August 25, 2020 06:02
@benpicco
Copy link
Contributor Author

benpicco commented Aug 25, 2020

Uh guess it was late yesterday 😅
We do have vfs_bind_stdio() which does exactly what it should.

So no need for any offsets then.
Still worth implementing those functions for the !VFS case though.

@benpicco
picolibc: enable read/write etc. functions without VFS
eb00ca8 
The read()/write() functions still work without VFS when
reading/writing to stdin, stdout or stderr.

Provide dummy functions for the remaining fs functions so
linking does not fail with !VFS.
@benpicco
picolibc: implement rmdir(), remove() & rewind()
3f35564 
Those are used by `tests/pkg_fatfs_vfs`.
@benpicco benpicco force-pushed the picolibc_stdio_offset branch from 21988b5 to 3f35564 Compare August 25, 2020 06:26
@benpicco benpicco changed the title picolibc: add offset to VFS fds picolibc: enable read()/write() with !VFS, implement missing fs functions Aug 25, 2020
@benpicco benpicco mentioned this pull request Aug 25, 2020
Closed
@benpicco benpicco added State: waiting for CI update State: The PR requires an Update to CI to be performed first and removed State: waiting for CI update State: The PR requires an Update to CI to be performed first labels Aug 30, 2020
@benpicco benpicco requested a review from fjmolinas September 2, 2020 09:22
@benpicco benpicco requested a review from aabadie September 10, 2020 17:41
@bergzand bergzand added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 11, 2020
@fjmolinas
Copy link
Contributor

fjmolinas commented Sep 11, 2020

It's not compiling for:

/data/riotbuild/riotbase/sys/picolibc_syscalls_default/syscalls.c:112:5: error: implicit declaration of function 'FDEV_SETUP_STREAM' [-Werror=implicit-function-declaration]
  112 |     FDEV_SETUP_STREAM(picolibc_put, picolibc_get, picolibc_flush, _FDEV_SETUP_RW);
      |     ^~~~~~~~~~~~~~~~~

@fjmolinas
Copy link
Contributor

fjmolinas commented Sep 11, 2020

It's not compiling for:

I'm probably missing something...

@benpicco
Copy link
Contributor Author

benpicco commented Sep 11, 2020

What version of picolibc do you use / how did you install it?

@bergzand
Copy link
Member

bergzand commented Sep 11, 2020

tests/pkg_fatfs_vfs compiles just fine here using the Docker image. The test appears to hang though (or maybe it takes just very long).

@fjmolinas
Copy link
Contributor

fjmolinas commented Sep 11, 2020

I'm probably missing something...

I allays forget to set the environment variable with DOCKER_ENVIRONMENT_CMDLINE='-e PICOLIBC=1', works fine now, sorry for the noise

@benpicco
Copy link
Contributor Author

benpicco commented Sep 11, 2020

I just installed the Debian package, no need for Docker 😄

bergzand
bergzand approved these changes Sep 11, 2020
View reviewed changes
Copy link
Member

@bergzand bergzand left a comment

Choose a reason for hiding this comment

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

Improves the current situation and only affects PicoLIBC. ACK!

fjmolinas
fjmolinas approved these changes Sep 11, 2020
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

I ran BOARD=nrf52840dk make -C tests/pkg_fatfs_vfs/ flash term -j with and without PICOLIBC, I get the same result as on master, not sure if its the expected result, but its the same as in master.

2020-09-11 17:28:56,511 # main(): This is RIOT! (Version: 2020.10-devel-954-g3f3556-pr-14843)
2020-09-11 17:28:56,771 # Tests for FatFs over VFS - test results will be printed in the format test_name:result
2020-09-11 17:28:57,025 # test_mount__mount:[FAILED]
2020-09-11 17:28:57,027 # test_mount__umount:[FAILED]
2020-09-11 17:28:57,281 # test_open__mount:[FAILED]
2020-09-11 17:28:57,283 # test_open__open:[OK]
2020-09-11 17:28:57,285 # test_open__open_ro:[FAILED]
2020-09-11 17:28:57,288 # test_open__close_ro:[FAILED]
2020-09-11 17:28:57,290 # test_open__open_wo:[FAILED]
2020-09-11 17:28:57,293 # test_open__close_wo:[FAILED]
2020-09-11 17:28:57,295 # test_open__open_rw:[FAILED]
2020-09-11 17:28:57,298 # test_open__close_rw:[FAILED]
2020-09-11 17:28:57,300 # test_open__umount:[FAILED]
2020-09-11 17:28:57,555 # test_rw__mount:[FAILED]
2020-09-11 17:28:57,556 # test_rw__open_ro:[FAILED]
2020-09-11 17:28:57,559 # test_rw__read_ro:[FAILED]
2020-09-11 17:28:57,561 # test_rw__write_ro:[OK]
2020-09-11 17:28:57,563 # test_rw__close_ro:[FAILED]
2020-09-11 17:28:57,565 # test_rw__open_wo:[FAILED]
2020-09-11 17:28:57,567 # test_rw__read_wo:[OK]
2020-09-11 17:28:57,570 # test_rw__close_wo:[FAILED]
2020-09-11 17:28:57,572 # test_rw__open_rw:[FAILED]
2020-09-11 17:28:57,574 # test_rw__read_rw:[FAILED]
2020-09-11 17:28:57,577 # test_rw__write_rw:[FAILED]
2020-09-11 17:28:57,579 # test_rw__lseek:[FAILED]
2020-09-11 17:28:57,581 # test_rw__read_rw:[FAILED]
2020-09-11 17:28:57,583 # test_rw__close_rw:[FAILED]
2020-09-11 17:28:57,586 # test_rw__open_rwc:[FAILED]
2020-09-11 17:28:57,589 # test_rw__write_rwc:[FAILED]
2020-09-11 17:28:57,591 # test_rw__lseek_rwc:[FAILED]
2020-09-11 17:28:57,594 # test_rw__read_rwc:[FAILED]
2020-09-11 17:28:57,596 # test_rw__close_rwc:[FAILED]
2020-09-11 17:28:57,598 # test_rw__umount:[FAILED]
2020-09-11 17:28:57,852 # test_dir__mount:[FAILED]
2020-09-11 17:28:57,854 # test_dir__opendir:[FAILED]
2020-09-11 17:28:57,856 # test_dir__readdir1:[FAILED]
2020-09-11 17:28:57,859 # test_dir__readdir2:[FAILED]
2020-09-11 17:28:57,862 # test_dir__readdir_name:[FAILED]
2020-09-11 17:28:57,864 # test_dir__readdir3:[FAILED]
2020-09-11 17:28:57,867 # test_dir__closedir:[FAILED]
2020-09-11 17:28:57,869 # test_dir__umount:[FAILED]
2020-09-11 17:28:58,123 # test_rename__mount:[FAILED]
2020-09-11 17:28:58,126 # test_rename__rename:[FAILED]
2020-09-11 17:28:58,128 # test_rename__opendir:[FAILED]
2020-09-11 17:28:58,131 # test_rename__readdir1:[FAILED]
2020-09-11 17:28:58,134 # test_rename__readdir2:[FAILED]
2020-09-11 17:28:58,137 # test_rename__check_name:[FAILED]
2020-09-11 17:28:58,139 # test_rename__readdir3:[FAILED]
2020-09-11 17:28:58,142 # test_rename__closedir:[FAILED]
2020-09-11 17:28:58,145 # test_rename__umount:[FAILED]
2020-09-11 17:28:58,399 # test_unlink__mount:[FAILED]
2020-09-11 17:28:58,401 # test_unlink__unlink1:[FAILED]
2020-09-11 17:28:58,405 # test_unlink__unlink2:[FAILED]
2020-09-11 17:28:58,407 # test_unlink__opendir:[FAILED]
2020-09-11 17:28:58,409 # test_unlink__readdir:[FAILED]
2020-09-11 17:28:58,412 # test_unlink__closedir:[FAILED]
2020-09-11 17:28:58,414 # test_unlink__umount:[FAILED]
2020-09-11 17:28:58,670 # test_mkrmdir__mount:[FAILED]
2020-09-11 17:28:58,672 # test_mkrmdir__mkdir:[FAILED]
2020-09-11 17:28:58,675 # test_mkrmdir__opendir1:[FAILED]
2020-09-11 17:28:58,677 # test_mkrmdir__closedir:[FAILED]
2020-09-11 17:28:58,680 # test_mkrmdir__rmdir:[FAILED]
2020-09-11 17:28:58,682 # test_mkrmdir__opendir2:[OK]
2020-09-11 17:28:58,685 # test_mkrmdir__umount:[FAILED]
2020-09-11 17:28:58,939 # test_create__mount:[FAILED]
2020-09-11 17:28:58,941 # test_create__open_wo:[FAILED]
2020-09-11 17:28:58,944 # test_create__write_wo:[FAILED]
2020-09-11 17:28:58,947 # test_create__close_wo:[FAILED]
2020-09-11 17:28:58,950 # test_create__open_wo2:[FAILED]
2020-09-11 17:28:58,953 # test_create__write_wo2:[FAILED]
2020-09-11 17:28:58,955 # test_create__close_wo2:[FAILED]
2020-09-11 17:28:58,958 # test_create__umount:[FAILED]
2020-09-11 17:28:59,212 # test_stat__mount:[FAILED]
2020-09-11 17:28:59,214 # test_stat__open:[FAILED]
2020-09-11 17:28:59,216 # test_stat__write:[FAILED]
2020-09-11 17:28:59,219 # test_stat__close:[FAILED]
2020-09-11 17:28:59,221 # test_stat__open:[FAILED]
2020-09-11 17:28:59,223 # test_stat__stat:[FAILED]
2020-09-11 17:28:59,225 # test_stat__close:[FAILED]
2020-09-11 17:28:59,228 # test_stat__size:[FAILED]
2020-09-11 17:28:59,230 # test_stat__umount:[FAILED]
2020-09-11 17:28:59,484 # test_libc__mount:[FAILED]
2020-09-11 17:28:59,486 # test_libc__fopen:[OK]
2020-09-11 17:28:59,488 # test_libc__fopen_w:[FAILED]
2020-09-11 17:28:59,491 # test_libc__fopen_r:[FAILED]
2020-09-11 17:28:59,493 # test_libc__remove:[FAILED]
2020-09-11 17:28:59,495 # test_libc__fopen_a:[FAILED]
2020-09-11 17:28:59,498 # test_libc__fopen_a2:[FAILED]
2020-09-11 17:28:59,501 # test_libc__remove:[FAILED]
2020-09-11 17:28:59,503 # test_libc__umount:[FAILED]
2020-09-11 17:28:59,504 # Test end.
2020-09-11 17:29:07,393 # Exiting Pyterm

@benpicco
Copy link
Contributor Author

benpicco commented Sep 11, 2020

Well the test uses mtd_sdcard, so unless you have an SD card wired up, it will fail.

@benpicco benpicco merged commit 819dc7d into RIOT-OS:master Sep 11, 2020
@benpicco benpicco deleted the picolibc_stdio_offset branch September 11, 2020 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maribu maribu maribu left review comments

@bergzand bergzand bergzand approved these changes

@fjmolinas fjmolinas fjmolinas approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030

@aabadie aabadie Awaiting requested review from aabadie

Assignees

No one assigned

Labels

Area: fs Area: File systems Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@benpicco @fjmolinas @bergzand @maribu