-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Feature/secure element ci #7816
Feature/secure element ci #7816
Conversation
merge master
Merge Master
merge master
merge Master
merge master
…nnectedhomeip into feature/secureElement-ci
…nnectedhomeip into feature/secureElement-ci
Could you expand a bit more on the "Secure Element build needs to be added in CI for K32W" description? We are starting to be sensitive on the time our build takes and how long CI takes to finish. Adding more platforms should be done if they provide significant differences from the rest. Does the secure element differ significantly from the regular one? At the very least, could we compile just one of the lock or light app instead of both (and maybe delete one of them from the 'regular' build so that there is no net compile time increase? |
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.
Requesting changes: concerned about extra build times adding up at only partial incremental gain.
Suggest to drop one of the non-secure-element builds and replace with se build, like "ligthing app uses regular build and lock app uses se build".
Sure .. Update as suggested.. Now lock-app without SE and lighting app with SE to maintain the same executino time |
const CHIP_ERROR error = Spake2p_KeyConfirm_HSM(&hsm_pake_context, role, in, in_len); | ||
CHIP_ERROR error = CHIP_ERROR_INTERNAL; | ||
error = Spake2p_KeyConfirm_HSM(&hsm_pake_context, role, in, in_len); |
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.
Why is this change needed?
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.
And in particular, why can't error
stay const 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.
I agree. The change was not required..It is the same now(no changes now after the latest commit). This had got changed for VerifyOrExit which was being used sometime earlier and "error variable" was being reused. now VerifyOrReturnError is being used and the change is not required.
* script for building with Secure Element * debug ==> release as it is actually built in release mode * added secure element build * Added with secure element build * added build instrctions * added lf * spacing * spacing * added se * image of se * Updated Readme * default host is host_k32w * restyled * spell corrected * Only One binary for code size analysis * extended for se * Removed the extra copy paste line * build fixes. Motivation fo this PR * restyled * Review comment updates * made it simpler * made const * labelling it correctly
Problem
Secure Element build needs to be added in CI for K32W
we did observe that the crypto files were moving ahed with changes but the changes were not reflected ni the Secure Element build as the Secure Element Build was not part of CI. Adding it to CI will provide us immediate feedback when something is broken. Also now take care that there is no net compile time increase as suggested by andy31415
Change overview
Added Secure Element build for K32W
Testing
Manually tested on K32W061 with Secure Element