Skip to content

Make devicekey remainder test more meaningful #11628

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

Merged
merged 1 commit into from
Oct 15, 2019

Conversation

kyle-cypress
Copy link

Description

As of 722628b, the "remainder" configuration also uses the default location near the end of flash. Which makes the two tests nearly identical with the exception that the "last two sectors" test correctly handles parts with a low (possibly 1:1) erase size to program size ratio.
Therefore, change the "remainder" test to instead be a "default" test that uses the tdb_internal_address/size values, so that it
a.) tests something meaningfully different
b.) tests using the custom TDB address/size values if they are provided.
c.) functions correctly on devices where the default sector-based size computation does not work (e.g. because of the low erase size to program size ratio) and therefore a custom location and size has been specified.
The is_conf_tdb_internal variable is unused and therefore removed.

Pull request type

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

Reviewers

Release Notes

As of 722628b, the "remainder" configuration
also uses the default location near the end of flash. Which makes the two tests
nearly identical with the exception that the "last two sectors" test correctly
handles parts with a low (possibly 1:1) erase size to program size ratio.
Therefore, change the "remainder" test to instead be a "default" test that uses
the tdb_internal_address/size values, so that it
a.) tests something meaningfully different and
b.) tests using the custom TDB address/size values if they are provided.
c.) functions correctly on devices where the default sector-based size computation
    does not work (e.g. because of the low erase size to program size ratio)
    and therefore a custom location and size has been specified.
The is_conf_tdb_internal variable is unused and therefore removed.
@ciarmcom
Copy link
Member

ciarmcom commented Oct 4, 2019

@kyle-cypress, thank you for your changes.
@ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

@adbridge
Copy link
Contributor

adbridge commented Oct 7, 2019

@SeppoTakalo please review

@@ -50,7 +50,7 @@ static inline uint32_t align_down(uint64_t val, uint64_t size)
return (((val) / size)) * size;
}

int get_virtual_TDBStore_position(uint32_t conf_start_address, uint32_t conf_size, bool is_conf_tdb_internal,
int get_virtual_TDBStore_position(uint32_t conf_start_address, uint32_t conf_size, bool is_conf_tdbd_internal,
Copy link
Contributor

Choose a reason for hiding this comment

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

The parameter doesn't seem to be used for anything actually.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed - I think this is a vestige from the old version of the test prior to 722628b . I'll push another commit to remove.

Copy link
Author

Choose a reason for hiding this comment

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

Removed the unused parameter

@kyle-cypress kyle-cypress force-pushed the pr/directaccess-devicekey-test branch from e4cd4a0 to 1cb6f7b Compare October 10, 2019 14:51
Copy link
Contributor

@VeijoPesonen VeijoPesonen left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 15, 2019

I restarted Travis and started CI

@mbed-ci
Copy link

mbed-ci commented Oct 15, 2019

Test run: SUCCESS

Summary: 5 of 5 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit fcd40ab into ARMmbed:master Oct 15, 2019
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.

7 participants