Skip to content

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

Closed
wants to merge 9 commits into from

Conversation

kegilbert
Copy link
Contributor

@kegilbert kegilbert commented Oct 31, 2017

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.

Copy link
Contributor

@AnotherButler AnotherButler left a 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.
Copy link
Contributor

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?

Copy link
Contributor Author

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:

  1. Add a new physical or virtual storage device, requiring the base abilities of reading, writing, and erasing it.
  2. 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).
  3. 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.


[![View code](https://www.mbed.com/embed/?type=library)](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.
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@sg- sg- left a 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

@kegilbert
Copy link
Contributor Author

@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.

@sg-
Copy link
Contributor

sg- commented Nov 1, 2017

was meant to show implementation details for porting as an example.

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.

@kegilbert
Copy link
Contributor Author

Fair enough.

@kegilbert
Copy link
Contributor Author

@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>).
Copy link
Contributor

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.

Copy link
Contributor Author

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.
@kegilbert
Copy link
Contributor Author

@sg- Do the new changes look good to you? Did you want anything else changed?

@sg-
Copy link
Contributor

sg- commented Nov 15, 2017

Can you confirm how compliant each standard library is with POSIX? If not lets reduce the use of that term. Looking better.

@kegilbert
Copy link
Contributor Author

@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.

@kegilbert
Copy link
Contributor Author

@sg- Were you thinking scratch the POSIX references from this page or leave as is?

@sg-
Copy link
Contributor

sg- commented Nov 27, 2017

scratch them and just document the function name and what it does

@kegilbert
Copy link
Contributor Author

@sg- @AnotherButler Removed POSIX references.

Amanda Butler added 2 commits December 1, 2017 15:22
Make minor edits for active voice and link fixes.
Update links to point to Doxy instead of GitHub, and add related content section.
Copy link
Contributor

@AnotherButler AnotherButler left a 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.

AnotherButler pushed a commit to geky/mbed_Handbook that referenced this pull request Dec 6, 2017
Add changes from KG's pending PR: ARMmbed#324
Copy link
Contributor

@sg- sg- left a 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?

@kegilbert
Copy link
Contributor Author

@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?

@sg-
Copy link
Contributor

sg- commented Dec 7, 2017

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.

@AnotherButler
Copy link
Contributor

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.

@kegilbert
Copy link
Contributor Author

They're pretty hard to get to honestly. Had to do the following:

@sg-
Copy link
Contributor

sg- commented Dec 7, 2017

In that case a ppt version or artistic rendering of the whiteboard ftw!

@AnotherButler
Copy link
Contributor

Could we screenshot the image from the Doxy?

@sg-
Copy link
Contributor

sg- commented Dec 7, 2017

Its not good enough for that

@kegilbert
Copy link
Contributor Author

This isn't a diagram we intend to change frequently right? Seems pretty set. I'd be fine with a screenshot/artistic rendition.

AnotherButler pushed a commit that referenced this pull request Dec 11, 2017
Make new PR from old PR I'm closing to avoid merge conflicts.
@AnotherButler
Copy link
Contributor

Please see PR #358 for further changes on the contributing storage page. We've pulled the API storage page into the existing file.

AnotherButler pushed a commit that referenced this pull request Dec 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants