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

pbio/sys/storage: Implement persistent storage. #249

Merged
merged 10 commits into from
Jun 7, 2024

Conversation

laurensvalk
Copy link
Member

Implements pybricks/support#1622

Since validity is checked against the firmware version and programs are erased, we shouldn't need to reserve space for spare or yet unknown settings. This adds a single bool setting for the Bluetooth enabled state.

NB: This PR is against a non-master branch specifically to make review easier. This skips one blanket rename commit that didn't change any code.


Should program_size below also manually forced to be word aligned? It does seem to do it automatically right now.

Could also require the settings size to be word sized, but I'd be curious to hear what's the cleanest way to do it.

/**
 * System settings. All data types are little-endian.
 */
typedef struct _pbsys_storage_settings_t {
    /**
     * User has enabled Bluetooth Low Energy.
     */
    bool bluetooth_ble_user_enabled : 1;
} pbsys_storage_settings_t;
    (...)
    uint8_t user_data[PBSYS_CONFIG_STORAGE_USER_DATA_SIZE];
    /**
     * System settings. Settings will be reset to defaults when the firmware
     * version changes due to an update.
     */
    pbsys_storage_settings_t settings;
    /**
     * Size of the application program (size of code only).
     */
    uint32_t program_size;
    /**
     * Data of the application program (code + heap).
     */
    uint8_t program_data[] __attribute__((aligned(sizeof(void *))));
} pbsys_storage_data_map_t;

This ensures we don't run broken programs when the memory layout changes. See pybricks/support#1622
This function already existed, so we might as well use it throughout to save 20 bytes.
This makes it show correctly even if already disabled on boot.
@laurensvalk
Copy link
Member Author

Could also require the settings size to be word sized, but I'd be curious to hear what's the cleanest way to do it.

Digging into the C spec, I see that bit fields aren't guaranteed to be in the same order, so we can't write it to disk and read it back with a different build and assume it's the same. So I'll just use bit flags on a word then.

This ensures it is always enabled on platforms without a toggle, such as Move Hub or the pbio tests.
@coveralls
Copy link

Coverage Status

coverage: 56.575% (+0.1%) from 56.454%
when pulling fe54637 on storage
into 8387de0 on skip-big-rename-commit.

Bit fields are not guaranteed to have the same order if we write them to disk and read them back with another firmware build.
Adapt it so that it is correct even as we add new fields in between going forward.
@coveralls
Copy link

Coverage Status

coverage: 56.592% (+0.1%) from 56.454%
when pulling 31c36c6 on storage
into 8387de0 on skip-big-rename-commit.

@coveralls
Copy link

Coverage Status

coverage: 56.592% (+0.1%) from 56.454%
when pulling 83e372a on storage
into 8387de0 on skip-big-rename-commit.

@laurensvalk laurensvalk merged commit 83e372a into skip-big-rename-commit Jun 7, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants