-
Notifications
You must be signed in to change notification settings - Fork 178
Add storage contributing overview doc #324
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
Conversation
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.
Please review my edits to make sure I didn't accidentally change the meaning of anything. Please also address my two queries.
|
||
Backing file systems on new hardware requires adding a block device implementation. You can extend the [BlockDevice](https://github.com/ARMmbed/mbed-os/blob/master/features/filesystem/bd/HeapBlockDevice.h) class to provide support for unsupported storage. | ||
|
||
If you want to port a new file system to Mbed OS on existing storage options, you can skip to the following section. |
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.
Query: What do you mean by the following section? File systems two lines below? Or the next page? What should the reader do if the reader doesn't want to port a new file system to Mbed OS?
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.
Correct, the following section is referring to the file system section.
A reader going through this section will want one of the following 3 situations:
- Add a new physical or virtual storage device, requiring the base abilities of reading, writing, and erasing it.
- Add a new filesystem implementation on an existing storage solution (Say on a SD card which Mbed OS supports). They do not need to port any block device code themselves (what the last line in the Block Device section is referring to).
- Combine the above two, a reader may want to implement a new filesystem solution on a block device that is not supported in Mbed OS.
|
||
[](https://github.com/ARMmbed/mbed-os/blob/master/features/filesystem/FileSystem.h#L205) | ||
|
||
A block device must back file systems in Mbed OS. If you are using supported hardware, then you can continue. Otherwise, view the block device porting section above. |
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.
Query: What does continue mean here? Read the next page or continue what the reader's doing? Please clarify.
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 was worded a bit poorly, continue as in continue with your filesystem porting. Otherwise you'll need a block device port to back the filesystem and should implement that first.
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.
Linking to github or header files is not accepted, sorry :) This should be API pages in the docs
@sg- I can update all of the header file links. Question though, do we have a way to link to cpp files in particular? The link to https://github.com/ARMmbed/mbedos/blob/master/features/filesystem/fat/FATFileSystem.cpp was meant to show implementation details for porting as an example. @AnotherButler Is there a way to link to header file locations in Github from the class reference page? If a reader is being told to extend a class or look at an implementation it would be nice to know where to find said class in the source without too much manual work on their part. |
In that case, just mentioning a class / implementation name google will find should suffice. I dont want to have links suggest where code lives in the case of linking a repo. |
Fair enough. |
9cc66a9
to
004a72e
Compare
@AnotherButler @sg- Replaced the github links, note that the filesystem links will be broken until the doxy files are regenerated to suck in the filesystem changes from ARMmbed/mbed-os#5395 . Updated the PR description to note this. |
@@ -7,15 +7,15 @@ The storage APIs present in Arm Mbed OS are: | |||
|
|||
#### Declaring a file system | |||
|
|||
The <a href="https://github.com/ARMmbed/mbed-os/blob/master/features/filesystem/FileSystem.h" target="_blank">FileSystem</a> class provides the core API for file system operations. You must provide a block device to back the file system. When you declare a file system with a name, you can open files on the file system through standard POSIX functions (see <a href="http://pubs.opengroup.org/onlinepubs/009695399/functions/open.html" target="_blank">open</a> or <a href="http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html" target="_blank">fopen</a>). | |||
The [FileSystem](https://os.mbed.com/docs/v5.6/mbed-os-api-doxy/class_file_system.html) class provides the core API for file system operations. You must provide a block device to back the file system. When you declare a file system with a name, you can open files on the file system through standard POSIX functions (see <a href="http://pubs.opengroup.org/onlinepubs/009695399/functions/open.html" target="_blank">open</a> or <a href="http://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html" target="_blank">fopen</a>). |
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.
we should not be linking to open, fopen, rather have these documented ourselves.
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.
@sg- Replaced external links with short blurbs on open and fopen.
Replaced with short description of open and fopen.
@sg- Do the new changes look good to you? Did you want anything else changed? |
Can you confirm how compliant each standard library is with POSIX? If not lets reduce the use of that term. Looking better. |
@sg- IAR and ARM's standard libraries I don't believe are completely compliant with POSIX. I spoke with Chris a bit on the history of these docs, it's a bit murky. The reference to POSIX here was mainly pointing out the subset of POSIX functions Mbed OS has C++ wrappers for. If it's less ambiguous I can remove the mention of POSIX from the docs in this page. |
@sg- Were you thinking scratch the POSIX references from this page or leave as is? |
scratch them and just document the function name and what it does |
@sg- @AnotherButler Removed POSIX references. |
Make minor edits for active voice and link fixes.
Update links to point to Doxy instead of GitHub, and add related content section.
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.
Please make sure I didn't accidentally change the meaning of anything.
Add changes from KG's pending PR: ARMmbed#324
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.
Looks good but probably conflicts with littlefs pr? Also seems to be missing the information about the FileSystem class public members being the application API and the protected members being the porting interface. And did we get a UML diagram for the class types and implementations per the white board for the storage intro page?
@sg- Yes it does conflict with the LittleFS PR. We merged in some of the changes with geky@0c4f2e6. We get the UML diagrams from the doxygen in the class reference, did you want those embedded in a different way/place? |
Do those render in the docs site? I didnt think so but could be wrong. They do miss the point of usability when compared to what we drew showing an example of classes that exist today not to be confused with the requirement to always keep upto date. |
We don't transclude those images on the docs site. That's a feature we have to ask for, so I don't know how well they render. |
They're pretty hard to get to honestly. Had to do the following:
|
In that case a ppt version or artistic rendering of the whiteboard ftw! |
Could we screenshot the image from the Doxy? |
Its not good enough for that |
This isn't a diagram we intend to change frequently right? Seems pretty set. I'd be fine with a screenshot/artistic rendition. |
Make new PR from old PR I'm closing to avoid merge conflicts.
Please see PR #358 for further changes on the contributing storage page. We've pulled the API storage page into the existing file. |
Split the existing storage overview doc in the API chapter to form separate API and contributing overview pages.
@AnotherButler @sg-
Was this primarily what you were looking for? Let me know if you want more content or think anything should move between pages.