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

Native POSIX Flash support #12503

Merged
merged 3 commits into from
May 29, 2019

Conversation

vanwinkeljan
Copy link
Member

@vanwinkeljan vanwinkeljan commented Jan 16, 2019

This PR adds flash support for the Native POSIX target, the flash data is placed on a binary file on the host system. Further the PR provides the possibility to access FS partition on the flash by means of FUSE file system mounted on a host directory

Following features were included in this PR either to make it functional or to test it:

  • DTS support for Native POSIX
  • Updated test cases that require a flash but where not covered by QEMU
  • Updated sanitycheck run directory
  • Extended FS API to read mount points
  • Added FS Shell Sample

This PR cover flash part mentioned in issue #6044

FUSE support will be moved to separate PR
Moved sanitycheck changes to PR #16149

@zephyrbot
Copy link
Collaborator

zephyrbot commented Jan 16, 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.

Copy link
Member

@aescolar aescolar left a comment

Choose a reason for hiding this comment

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

This will take a while to review.. Some minor things after a quick look thru

boards/posix/native_posix/Kconfig Outdated Show resolved Hide resolved
boards/posix/native_posix/native_posix.dts Outdated Show resolved Hide resolved
boards/posix/native_posix/doc/board.rst Outdated Show resolved Hide resolved
@aescolar aescolar added Maintainer At least 2 days before merge, maintainer review required Enhancement Changes/Updates/Additions to existing features labels Jan 16, 2019
Copy link
Member

@aescolar aescolar left a comment

Choose a reason for hiding this comment

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

a few more

boards/posix/native_posix/doc/board.rst Outdated Show resolved Hide resolved
@@ -0,0 +1,531 @@
/*
* Copyright (c) 2019 Jan Van Winkel <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

After a quick look it would seem this file is not really part of the native_posix board, but could be shared by any posix arch based board, and could therefore have been anywhere else(?). If that is the case maybe it would be nicer to place it with the flash driver itself(?)
(The Kconfig option and cmake paragraph would go with it)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point but would it not be better to move it then to subsys/fs?

Copy link
Member

Choose a reason for hiding this comment

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

I have no opinion, I think it would be best if one of the flash or fs maintainers would comment here.

Copy link
Member Author

Choose a reason for hiding this comment

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

boards/posix/native_posix/doc/board.rst Outdated Show resolved Hide resolved
boards/posix/native_posix/doc/board.rst Outdated Show resolved Hide resolved
boards/posix/native_posix/doc/board.rst Outdated Show resolved Hide resolved
boards/posix/native_posix/fuse_flash.c Outdated Show resolved Hide resolved
boards/posix/native_posix/doc/board.rst Outdated Show resolved Hide resolved
dts/bindings/posix/zephyr,posix.yaml Outdated Show resolved Hide resolved
subsys/settings/src/settings_file.c Outdated Show resolved Hide resolved
tests/subsys/dfu/mcuboot/src/main.c Outdated Show resolved Hide resolved
boards/posix/native_posix/doc/board.rst Outdated Show resolved Hide resolved
boards/posix/native_posix/doc/board.rst Outdated Show resolved Hide resolved
samples/subsys/fs/shell/README.rst Outdated Show resolved Hide resolved
samples/subsys/fs/shell/README.rst Outdated Show resolved Hide resolved
samples/subsys/fs/shell/README.rst Outdated Show resolved Hide resolved
samples/subsys/fs/shell/README.rst Outdated Show resolved Hide resolved
samples/subsys/fs/shell/README.rst Outdated Show resolved Hide resolved
boards/posix/native_posix/doc/board.rst Outdated Show resolved Hide resolved
@vanwinkeljan vanwinkeljan force-pushed the flash_native_posix branch 2 times, most recently from 651f674 to 1df6f1d Compare January 21, 2019 21:59
Copy link
Member

@aescolar aescolar left a comment

Choose a reason for hiding this comment

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

I have no experience or knowledge about DTS or the filesystem part, so somebody else will need to review those.
@galak Would you do the honors for the DTS part? (or both)

reg = <0x00000000 DT_SIZE_K(2048)>;

partitions {
compatible = "fixed-partitions";
Copy link
Member

Choose a reason for hiding this comment

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

It really feels a bit funny to define partitions or the flash size in a file for the native_posix board, when it could have any size the developer would want to test with. Maybe somebody who know more about DTS (@galak) could comment here?

Copy link
Member Author

Choose a reason for hiding this comment

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

You should see it more as a default config the developer can overrule any of the DST values in a overlay.

boards/posix/native_posix/doc/board.rst Outdated Show resolved Hide resolved
boards/posix/native_posix/doc/board.rst Outdated Show resolved Hide resolved
boards/posix/native_posix/doc/board.rst Outdated Show resolved Hide resolved
drivers/flash/Kconfig.native_posix Outdated Show resolved Hide resolved
boards/posix/native_posix/native_posix.dts Outdated Show resolved Hide resolved
drivers/flash/flash_native_posix.c Show resolved Hide resolved
boards/posix/native_posix/fuse_flash.c Outdated Show resolved Hide resolved
samples/subsys/fs/shell/README.rst Outdated Show resolved Hide resolved
Ls
--

List all files and directories in a given path
Copy link
Member

@aescolar aescolar Jan 26, 2019

Choose a reason for hiding this comment

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

I have no experience with the filesystem subsystem, but I can't even get to list an empty mount point.

uart:~$ fs mount nffs /nffs
Successfully mounted fs:/nffs
uart:~$ fs ls /nffs
Unable to open /nffs (err -22)

or to mess with it from the FUSE mount side. I wonder if that's me or something is broken.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not you, I did hit the same issue.
After looking through the FS code it seems that it is limitation in Zephyrs FS implementation it seems that current implementation expects the root slash.

So fs ls /nffs/ should do the trick, btw I did put a workaround in the Fuse code to handle this case.

Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

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

+1 for docs, thanks!

@jhedberg
Copy link
Member

jhedberg commented Apr 9, 2019

Any update on this PR?

@aescolar
Copy link
Member

aescolar commented Apr 9, 2019

@galak @nvlsianpu : Could you please review the DTS and file system sides respectively?

@aescolar aescolar dismissed their stale review April 9, 2019 10:34

Pending review by FS and DTS maintainers, the native_posix part was ok to the best of my recolection

galak
galak previously requested changes May 14, 2019
Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

Sorry if I didn't mention this before, but can you pull the 'sanitycheck: Execute binary in output directory' commit into its own PR.

@galak
Copy link
Collaborator

galak commented May 14, 2019

LGTM.

@vanwinkeljan
Copy link
Member Author

@galak

Sorry if I didn't mention this before, but can you pull the 'sanitycheck: Execute binary in output directory' commit into its own PR.

Sorry my misunderstanding; I just split the PR in two, flash support and fuse access support.
I will move the sanitycheck commit in another PR, of course this PR will be blocked until it is merged and the commit will stay visible in this PR.

Copy link
Member

@aescolar aescolar left a comment

Choose a reason for hiding this comment

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

No complaints from me (note I did not retest it). But again, somebody who knows more about the flash side of things needs to give his opinion.
Just one request: Please add yourself also as codeowner to drivers/flash/*native_posix*
(Otherwise you could be sure I would be adding you to any review or bug for these files :) )

@vanwinkeljan
Copy link
Member Author

Just one request: Please add yourself also as codeowner to drivers/flash/native_posix

Done

Sorry if I didn't mention this before, but can you pull the 'sanitycheck: Execute binary in output directory' commit into its own PR.

Done

@vanwinkeljan vanwinkeljan added the DNM This PR should not be merged (Do Not Merge) label May 14, 2019
@vanwinkeljan
Copy link
Member Author

Added DNM label as this PR depends on #16149

CODEOWNERS Outdated Show resolved Hide resolved
@aescolar aescolar requested a review from galak May 14, 2019 17:35
@vanwinkeljan vanwinkeljan force-pushed the flash_native_posix branch 2 times, most recently from e96e76e to 0ff64d9 Compare May 16, 2019 06:37
@vanwinkeljan vanwinkeljan removed the DNM This PR should not be merged (Do Not Merge) label May 23, 2019
Added device tree support for POSIX architecture based boards.

Signed-off-by: Jan Van Winkel <[email protected]>
Added native POSIX flash driver that writes flash content to a binary
file.

Signed-off-by: Jan Van Winkel <[email protected]>
Added native POSIX boards as target for flash related tests.

Signed-off-by: Jan Van Winkel <[email protected]>
Copy link
Member

@aescolar aescolar left a comment

Choose a reason for hiding this comment

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

I guess it is unlikely to get more reviews, so I think we can get this in, there is nothing wrong to me. (There would be just 1 nit: The 2nd UART being described in DTS for consistency, but it can come later)

@nashif nashif merged commit afdbc20 into zephyrproject-rtos:master May 29, 2019
@vanwinkeljan vanwinkeljan deleted the flash_native_posix branch May 29, 2019 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Flash area: native port Host native arch port (native_sim) Enhancement Changes/Updates/Additions to existing features Maintainer At least 2 days before merge, maintainer review required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants