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

Update the Mbed TLS README #8259

Merged
merged 5 commits into from Oct 27, 2018

Conversation

@sbutcher-arm
Contributor

sbutcher-arm commented Sep 26, 2018

Description

Update the Mbed TLS README.md file to include information contained in Mbed OS 3, which was omitted from Mbed OS 5.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Breaking change
Update the Mbed TLS README.md
Add content missing from the README.md taken from the Yotta/Mbed OS 3 Readme.
@sbutcher-arm

This comment has been minimized.

Contributor

sbutcher-arm commented Sep 26, 2018

This should be reviewed by @RonEld, @Patater and @mpg /or/ @gilles-peskine-arm .

Show resolved Hide resolved features/mbedtls/README.md Outdated
Show resolved Hide resolved features/mbedtls/README.md Outdated
Show resolved Hide resolved features/mbedtls/README.md Outdated
Show resolved Hide resolved features/mbedtls/README.md Outdated
Show resolved Hide resolved features/mbedtls/README.md Outdated
Show resolved Hide resolved features/mbedtls/README.md Outdated
Show resolved Hide resolved features/mbedtls/README.md Outdated
Show resolved Hide resolved features/mbedtls/README.md Outdated
Show resolved Hide resolved features/mbedtls/README.md Outdated
Show resolved Hide resolved features/mbedtls/README.md Outdated
Show resolved Hide resolved features/mbedtls/README.md Outdated
Show resolved Hide resolved features/mbedtls/README.md Outdated
To import into an instance of Mbed OS a different version of Mbed TLS, a `Makefile` script is provided to update the local git repository, extract a specific version, and to modify the configuration files to those used for the Mbed OS defaults.
To use the `Makefile`, you can either set `MBED_TLS_RELEASE` environment variable to the git tag or commit id of the Mbed TLS Release or version you want to use, or alternatively you can modify the `Makefile` itself.

This comment has been minimized.

@gilles-peskine-arm

gilles-peskine-arm Sep 26, 2018

Tag or commit? Not a branch name to get the tip of the branch? Can't I leave it blank to get the default, and would I get the latest release [master], the latest beta [development], or some untested experimental thing [we don't have this]?

@0xc0170 0xc0170 added needs: work and removed needs: review labels Oct 1, 2018

@mpg

I reviewed this and don't have anything to add to existing reviews. I'm happy with the changes in general, but a few details should be fixed/improved.

Update Mbed TLS README.md followng review
Numerous changes to language, grammar, and corrections, following review.

@sbutcher-arm sbutcher-arm force-pushed the sbutcher-arm:readme-update branch from d960bad to fee476e Oct 3, 2018

@sbutcher-arm

This comment has been minimized.

Contributor

sbutcher-arm commented Oct 3, 2018

I have responded to all review feedback. Please re-review and approve.

@Patater

Patater approved these changes Oct 3, 2018

@0xc0170 0xc0170 requested a review from AnotherButler Oct 9, 2018

@0xc0170

0xc0170 approved these changes Oct 9, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 9, 2018

@AnotherButler Please review

@AnotherButler AnotherButler requested a review from melwee01 Oct 9, 2018

@AnotherButler

This comment has been minimized.

Contributor

AnotherButler commented Oct 9, 2018

@MelindaWeed Could you please take this?

@melwee01

This comment has been minimized.

Contributor

melwee01 commented Oct 15, 2018

Sure, looking at it now.

@melwee01

I can apply most of these changes myself.

The [mbed TLS website](https://tls.mbed.org/) contains full documentation for the library, including function by function descriptions, knowledgebase articles, blogs and a support forum for questions to the community.
Several example programs are available that demonstrate the use of Mbed TLS with

This comment has been minimized.

@melwee01

melwee01 Oct 15, 2018

Contributor

Check these line endings so they render properly.

Show resolved Hide resolved features/mbedtls/README.md
For example, if you wanted to enable the options, `MBEDTLS_PEM_WRITE_C` and `MBEDTLS_CMAC_C`, and provide your own additional configuration file for Mbed TLS named `my_config.h`, you could define these in a top level `mbed_app.json` configuration file in the root directory of your project.
The Mbed TLS configuration file would be specified in the `.json` file as follows.

This comment has been minimized.

@melwee01

melwee01 Oct 15, 2018

Contributor

:

Show resolved Hide resolved features/mbedtls/README.md
## Getting Mbed TLS from GitHub
Mbed TLS is maintained and developed in the open, independently of Mbed OS, and its source can be found on GitHub here: [ARMmbed/mbedtls](https://github.com/ARMmbed/mbedtls). To import into an instance of Mbed OS a different version of Mbed TLS, a `Makefile` script is provided to update the local git repository, extract a specific version, and to modify the configuration files to those used for the Mbed OS defaults.

This comment has been minimized.

@melwee01

melwee01 Oct 15, 2018

Contributor

Git

Once these steps are complete, you can make your Mbed OS build normally with the new version of Mbed TLS.
## Differences between the standalone and Mbed OS editions

This comment has been minimized.

@melwee01

melwee01 Oct 15, 2018

Contributor

Just a general comment for myself when I get into the file: take out unnecessary articles.

## Differences between the standalone and Mbed OS editions
While the two editions share the same code base, there are still a number of differences, mainly in configuration and integration. You should keep in mind those differences when reading some articles in our [knowledge base](https://tls.mbed.org/kb), as currently all the articles are about the standalone edition.

This comment has been minimized.

@melwee01

melwee01 Oct 15, 2018

Contributor

You should keep those differences in mind if you consult our knowlege base, as these refer to the standalone edition.

Help and Support
----------------
The [Mbed TLS website](https://tls.mbed.org/) contains full documentation for the library, including function by function descriptions, knowledge base articles and blogs. In addition there is a [support forum](https://forums.mbed.com/c/mbed-tls) for questions to the community.

This comment has been minimized.

@melwee01

melwee01 Oct 15, 2018

Contributor

function-by-function

This comment has been minimized.

@melwee01

melwee01 Oct 15, 2018

Contributor

Additionally,

This comment has been minimized.

@melwee01

melwee01 Oct 15, 2018

Contributor

Rephrase this as 'you can' statements to grab the reader more.

@@ -21,5 +104,4 @@ We gratefully accept bug reports and contributions from the community. There are
- Simple bug fixes to existing code do not contain copyright themselves and we can integrate without issue. The same is true of trivial contributions.
- For larger contributions, such as a new feature, the code can possibly fall under copyright law. We then need your consent to share in the ownership of the copyright. We have a form for this, which we will send to you in case you submit a contribution or pull request that we deem this necessary for.
Contributions should be submitted to the [standalone mbed TLS project](https://github.com/ARMmbed/mbedtls), not to the mbed OS imported edition of mbed TLS.
Contributions should be submitted to the [standalone Mbed TLS project](https://github.com/ARMmbed/mbedtls), not to the version of Mbed TLS embedded within Mbed OS.

This comment has been minimized.

@melwee01

melwee01 Oct 15, 2018

Contributor

Submit contributions to...

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 23, 2018

@melwee01 Any updates? Were you going to copy edit the suggestions, or should @sbutcher-arm ?

@melwee01

This comment has been minimized.

Contributor

melwee01 commented Oct 23, 2018

Hmm. Since the OS repo functions differently, it's harder for me to apply changes myself. Probably easier if @sbutcher-arm does it, and asks me if he has any questions.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 23, 2018

Hmmm. @AnotherButler, you're able to copy edit @melwee01 suggestions, correct?

@sbutcher-arm

This comment has been minimized.

Contributor

sbutcher-arm commented Oct 24, 2018

Should I not make the edits myself @cmonr?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 24, 2018

Should I not make the edits myself @cmonr?

Please do

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 24, 2018

@sbutcher-arm We're just use to seeing the docs team do copy edits on their own.

Improve Mbed TLS README.md
Improves the language, formatting and clarity of the Mbed TLS README.md.
@sbutcher-arm

This comment has been minimized.

Contributor

sbutcher-arm commented Oct 24, 2018

I've addressed some of the feedback, and responded to all remaining points.

@cmonr cmonr added needs: review and removed needs: work labels Oct 24, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 24, 2018

@melwee01 @mpg @AnotherButler @RonEld When y'all get a chance.

Changes addressed. Please re-review.

@melwee01

This comment has been minimized.

Contributor

melwee01 commented Oct 25, 2018

I'm making a few additional editorial changes myself (finally know how to do that, yay).

@melwee01

I've made a number of small changes myself.

@mpg

mpg approved these changes Oct 25, 2018

@RonEld

One small comment on the default Mbed TLS version being extracted

Mbed TLS is maintained and developed in the open, independently of Mbed OS, and its source can be found on GitHub here: [ARMmbed/mbedtls](https://github.com/ARMmbed/mbedtls). To import a different version of Mbed TLS into an instance of Mbed OS, there is a `Makefile` script to update the local Git repository, extract a specific version, and modify the configuration files to Mbed OS defaults.
To use the `Makefile`, you can either set `MBED_TLS_RELEASE` environment variable to the Git tag or commit ID of the Mbed TLS release or version you want to use, or modify the `Makefile` itself. If `MBED_TLS_RELEASE` is unset, the HEAD of the main development branch will be extracted.

This comment has been minimized.

@RonEld

RonEld Oct 25, 2018

Contributor

If MBED_TLS_RELEASE is unset, the HEAD of the main development branch will be extracted.

This is not accurate. If it is not given as an argument to the Makefile, then the Mbed-OS release will be extracted, as you can see here

This comment has been minimized.

@sbutcher-arm

sbutcher-arm Oct 26, 2018

Contributor

That's not what the sentence was meant to convey. I meant something like:

If MBED_TLS_RELEASE is not set in the Makefile, the HEAD of the main development branch will be extracted.

It obviously isn't clear enough.

This comment has been minimized.

@melwee01

melwee01 Oct 26, 2018

Contributor

If MBED_TLS_RELEASE is not set in the Makefile, the HEAD of the main development branch will be extracted.

Is that what you want in place of the original sentence?

@0xc0170 0xc0170 added needs: work and removed needs: review labels Oct 25, 2018

Edit README.md
Edit file, mostly for active voice and removal of marketing language.

Comment has been addressed. Feel free to re-review!

@cmonr cmonr referenced this pull request Oct 26, 2018

Merged

Rollup PR: UK Docathon pt2 #8552

@cmonr

This comment has been minimized.

Contributor

cmonr commented Oct 26, 2018

Note: This PR is now a part of a rollup PR (#8552).

No further work is needed here, as once that PR is merged, this PR will also be closed and marked as merged.

@sbutcher-arm

This comment has been minimized.

Contributor

sbutcher-arm commented Oct 26, 2018

There's an open issue of clarity on one line. @RonEld raised an issue, but it hasn't been addressed by @AnotherButler's edits.

@cmonr - To fix it, do we have to create another PR now?

@melwee01

This comment has been minimized.

Contributor

melwee01 commented Oct 26, 2018

If the rollup hasn't been merged yet, can't we just make a commit to that?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 26, 2018

If the rollup hasn't been merged yet, can't we just make a commit to that?

Let's do that. There's one problem we need to fix there anyway. @sbutcher-arm commit that can be applied or just drop a line here, @melwee01 can add it this time

@cmonr cmonr merged commit 8bf4981 into ARMmbed:master Oct 27, 2018

9 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 536 files (+0 files)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9997 cycles (-9 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the needs: CI label Oct 27, 2018

@sbutcher-arm sbutcher-arm deleted the sbutcher-arm:readme-update branch Oct 27, 2018

@cmonr cmonr removed the rollup PR label Oct 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment