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

Explaining how to opt in modules #10991

Merged
merged 3 commits into from
Sep 6, 2019

Conversation

RonEld
Copy link
Contributor

@RonEld RonEld commented Jul 8, 2019

Description

Add a section in the CC Readme file explaining how to enable the optional module.
Dependent on #10907 , #10913 , #10947

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[x ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@ARMmbed/mbed-os-crypto

Release Notes

Add a section in the CC Readme file explaining how to enable the optional module.
@ciarmcom ciarmcom requested a review from a team July 8, 2019 17:00
@ciarmcom
Copy link
Member

ciarmcom commented Jul 8, 2019

@RonEld, thank you for your changes.
@ARMmbed/mbed-os-crypto @ARMmbed/mbed-os-maintainers please review.

@ciarmcom ciarmcom requested a review from a team July 8, 2019 17:00
There are three additional modules that have alternative implementation support, which are not enabled by default.
The reason is to allow backwards compatability, as these modules don't have full functionality, and return `MBEDTLS_ERR_PLATFORM_FEATURE_UNSUPPORTED` for some features.
The modules are:
* `AES` which only supports 128 bit key size, in opposed to previous suipport for all key sizes.
Copy link
Contributor

Choose a reason for hiding this comment

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

support typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

suipport -> support
@RonEld
Copy link
Contributor Author

RonEld commented Jul 9, 2019

@0xc0170 Does this require review from @ARMmbed/mbed-docs ?

Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

@AnotherButler will do better :) Otherwise looks good to me


## Enabling optional alternative drivers

There are three additional modules that have alternative implementation support, which are not enabled by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
There are three additional modules that have alternative implementation support, which are not enabled by default.
There are three additional modules with alternative implementation support, disabled by default.

@artokin
Copy link
Contributor

artokin commented Jul 15, 2019

@AnotherButler , please add your review comments.

Edit file, mostly for active voice, formatting and parallel construction.
@AnotherButler
Copy link
Contributor

Should this be added to our public, published Mbed OS documentation, or is it not going onto master?

@RonEld
Copy link
Contributor Author

RonEld commented Jul 16, 2019

@AnotherButler This is secifically for Cryptocell, so I think this readme file is enough.
It is also intended for the patch release, as for next release, we will make these optional features, not optional, I believe

@AnotherButler
Copy link
Contributor

@Patater Will we want to add this to the published docs when these features are no longer optional?

@Patater
Copy link
Contributor

Patater commented Jul 19, 2019

We want this documented both for the patch release, where the feature is optional (default off), as well as for a later release, where the feature becomes default on.

@RonEld
Copy link
Contributor Author

RonEld commented Jul 21, 2019

@Patater Why do you want to keep this when the feature is not optional? Perhaps we should modify the file how to opt-out features, which will be turned on by default.

@AnotherButler Since this is specific for the cryptocell porting, I don't see a reason why to put this in our published docs, unless:

  1. We make a document specific for Cryptocell porting, which I don't think is needed at the moment, since currently we only have one mbed os platform with Cryptocell feature.
  2. We make a generic document on how to opt out \ in features, or modify https://os.mbed.com/docs/mbed-os/v5.13/porting/hardware-accelerated-crypto.html with this information

@AnotherButler
Copy link
Contributor

FYI @iriark01

@oliverjharper
Copy link

@yanesca can you take a look at this in Jaeden's absence please?

@yanesca
Copy link
Contributor

yanesca commented Aug 6, 2019

I agree with @Patater that it is best to keep this around. The user might want either hardware acceleration or full functionality. Whichever is the default, we should explain how to achieve the other.

@RonEld you mention two ways to sync the readme with the default option in the source:

  1. update the readme when changing the default option and describe how to opt-out.
  2. make the readme generic explaining both opt-in and opt-out

Which one would you like to do?

@RonEld
Copy link
Contributor Author

RonEld commented Sep 2, 2019

@yanesca

@RonEld you mention two ways to sync the readme with the default option in the source:

  1. update the readme when changing the default option and describe how to opt-out.
  2. make the readme generic explaining both opt-in and opt-out
    Which one would you like to do?

I think the first option is prefered.

The preceding PRs have been merged, and I think we should merge this PR.
What is blocking this?

@yanesca
Copy link
Contributor

yanesca commented Sep 2, 2019

@RonEld When is the default going to change? How can we make it sure that we don't forget to update this documentation?

@RonEld
Copy link
Contributor Author

RonEld commented Sep 2, 2019

My plan was that the PRs will be inserted to the patch releases, and for next major release, make default opted-in.
However, the PRs were merged into next major release( 2.14), so we will probably change the default in 2.15, with the documentation in the same PR.

Once this PR will be merged, I will make the PR changing the default in the modules, updating this documentation with it, targeting 2.15

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 5, 2019

terget stm32F446ze on nucleo board

They were integrated. @RonEld shall we progress here?

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 5, 2019

CI started

@RonEld
Copy link
Contributor Author

RonEld commented Sep 5, 2019

@0xc0170 I don't understand your comment
This PR is not related to stm32F446ze on nucleo board

IMHO, we can and should progress with this PR

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 5, 2019

Dependent on #10907 , #10913 , #10947

This one I was supposed to copy-paste, not the nucleo text ! :-)

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 5, 2019

CI started

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

Successfully merging this pull request may close these issues.

None yet

9 participants