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

Enable SE050 by default #471

Merged
merged 7 commits into from
Apr 12, 2024
Merged

Conversation

sosthene-nitrokey
Copy link
Collaborator

@sosthene-nitrokey sosthene-nitrokey commented Apr 4, 2024

  • Enable se050 by default for opcard if it is not yet used
  • Enable se050 after an opcard reset
  • Enable se050 after a full device reset
  • Enable se050 feature-flag by default

Depends on:

@sosthene-nitrokey sosthene-nitrokey force-pushed the space-migrate-enable-se050 branch 3 times, most recently from 31854c4 to ea1e181 Compare April 5, 2024 08:07
@nitrokey-ci
Copy link
Collaborator

nitrokey-ci commented Apr 5, 2024

metric value change
binary-size-nk3am 1418320 🔴 +223683 (+0.19%)
binary-size-nk3xn 508480 🔴 +78616 (+0.18%)
Insignifcant changes
metric value change
binary-size-nk3am-test 2021274 🔴 +12167 (+0.01%)
binary-size-nk3xn-test 547108 🔴 +3880 (+0.01%)
binary-size-nkpk 731246 -630 (-0.00%)

@sosthene-nitrokey sosthene-nitrokey marked this pull request as ready for review April 5, 2024 12:11
components/apps/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 161 to 168
impl Default for OpcardConfig {
fn default() -> Self {
Self {
#[cfg(feature = "se050")]
use_se050_backend: true,
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

AFAIS this silently changes the configuration when upgrading from the test release with se050 disabled. Do we really want that?

Copy link
Member

Choose a reason for hiding this comment

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

It also automatically enables the se050 backend when upgrading from stable with the default config. My idea last week was to use two separate config states:

  • a default state, i. e. Default::default, that is used when an option is not set explicitly
  • an init state that is set during provisioning or after a factory reset

Ideally, the init state would be the default state for most options, but here we need to make a distinction.

Copy link
Collaborator Author

@sosthene-nitrokey sosthene-nitrokey Apr 9, 2024

Choose a reason for hiding this comment

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

There's also the problem that after a factory reset the config is not saved again. I need to expose a way to do that with the admin-app.

So many edge cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be fixed now.

@sosthene-nitrokey sosthene-nitrokey force-pushed the space-migrate-enable-se050 branch 3 times, most recently from 1b49d07 to 02c8fbb Compare April 9, 2024 09:32
@sosthene-nitrokey sosthene-nitrokey merged commit aae21b8 into main Apr 12, 2024
9 checks passed
@sosthene-nitrokey sosthene-nitrokey deleted the space-migrate-enable-se050 branch April 12, 2024 12:26
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.

4 participants