-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Native POSIX Flash support #12503
Conversation
All checks are passing now. Review history of this comment for details about previous failed status. |
b27006e
to
53b17ac
Compare
There was a problem hiding this 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few more
@@ -0,0 +1,531 @@ | |||
/* | |||
* Copyright (c) 2019 Jan Van Winkel <[email protected]> |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
651f674
to
1df6f1d
Compare
There was a problem hiding this 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"; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
samples/subsys/fs/shell/README.rst
Outdated
Ls | ||
-- | ||
|
||
List all files and directories in a given path |
There was a problem hiding this 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 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.
There was a problem hiding this comment.
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.
8301c70
to
3a974b9
Compare
3a974b9
to
511556b
Compare
There was a problem hiding this 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!
511556b
to
ee09fc5
Compare
Any update on this PR? |
@galak @nvlsianpu : Could you please review the DTS and file system sides respectively? |
Pending review by FS and DTS maintainers, the native_posix part was ok to the best of my recolection
There was a problem hiding this 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.
LGTM. |
Sorry my misunderstanding; I just split the PR in two, flash support and fuse access support. |
There was a problem hiding this 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 :) )
92acbaa
to
fe793c6
Compare
Done
Done |
Added DNM label as this PR depends on #16149 |
e96e76e
to
0ff64d9
Compare
0ff64d9
to
c9488bc
Compare
c9488bc
to
947b1cb
Compare
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]>
947b1cb
to
efec595
Compare
There was a problem hiding this 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)
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 directoryFollowing features were included in this PR either to make it functional or to test it:
Updated sanitycheck run directoryExtended FS API to read mount pointsAdded FS Shell SampleThis PR cover flash part mentioned in issue #6044
FUSE support will be moved to separate PR
Moved sanitycheck changes to PR #16149