NVStore: add the allocate_key API (instead of set_alloc_key) #6480

davidsaada · 2018-03-27T14:33:14Z

Description

Add the allocate_key API. This replaces the previously added set_alloc_key API (which allocates a key and sets the value at the same time).
Reason for the change: Key allocation will typically be used by other storage features (like StorageLite), keeping the allocated keys in another location. Previous API created problems in the case key allocation and value setting couldn't be done at the same time (for instance, if the set value was derived from the allocated key, such as hash or CMAC).

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[x] Breaking change

theotherjimmy · 2018-03-29T15:15:11Z

features/nvstore/source/nvstore.h

     *          NVSTORE_NO_FREE_KEY       Couldn't allocate a key for this call.
     *
     */
-    int set_alloc_key(uint16_t &key, uint16_t buf_size, const void *buf);
+    int allocate_key(uint16_t &key);


This is a breaking change.

It is indeed, but the replaced function (set_alloc_key) is a new one that was implemented after the 5.8 release. It's still not even documented. Figured that the API we had defined was wrong in that case, hence the change. Can add the "old" API back, but that that would be just dead code.

This PR contains a breaking change, so marked for 5.9.0.

That's OK. It is mainly used for StorageLite feature, which should be out in 5.9.0.

theotherjimmy · 2018-03-29T15:15:21Z

features/nvstore/source/nvstore.h

     * @param[in]  buf_size               Buffer size (bytes).
     * @param[in]  buf                    Input Buffer.
     * @param[in]  flags                  Record flags.
     *
     * @returns 0 for success, nonzero for failure.
     */
-    int do_set(uint16_t &key, uint16_t buf_size, const void *buf, uint16_t flags);
+    int do_set(uint16_t key, uint16_t buf_size, const void *buf, uint16_t flags);


This is also a breaking change.

That's not a breaking change, as the function is private (maybe the doxy comments are confusing to think it is not).

Ah, I missed the private at the top. Thanks.

davidsaada · 2018-04-02T16:18:59Z

Anything missing in order for this PR to move forward (not that I'm trying to push anyone)?

davidsaada · 2018-04-03T10:15:33Z

@cmonr @marcuschangarm I said i didn't have a problem to have this fix on a temporary basis, and try resolving that when I'm back. However, until seeing @cmonr's comment, didn't notice the fact that this was on master, while other changes were on the feature branch. This will prevent me from testing any fix properly, as much of the memory consumption based fixes are based on trial and error (to make sure they work on all boards). Maybe it would be better to move this change to the feature branch as well?
EDIT: sorry - commented on the wrong PR...

0xc0170 · 2018-04-03T10:31:38Z

It is a breaking change, that is correct, but what we missed , @davidsaada to confirm - as this functionality that is changing here was just on master (added after 5.8, only available on master). Therefore the reasoning:

Reason for the change: Key allocation will typically be used by other storage features (like StorageLite), keeping the allocated keys in another location. Previous API created problems in the case key allocation and value setting couldn't be done at the same time (for instance, if the set value was derived from the allocated key, such as hash or CMAC).

This PR should be considered and reviewed.

davidsaada · 2018-04-03T10:38:28Z

Confirmed - the modified API isn't there even on 5.8.1. It was added to master after the 5.8 release, then when found my mistake, fixed it quickly.

0xc0170

LGTM, just one small change

0xc0170 · 2018-04-04T13:53:51Z

features/nvstore/source/nvstore.cpp

@@ -597,16 +599,16 @@ int NVStore::do_get(uint16_t key, uint16_t buf_size, void *buf, uint16_t &actual
    }

    _mutex->lock();
-    record_offset = _offset_by_key[key];
+
+	record_offset = _offset_by_key[key];


misaligned- tabs here?

Strange, my editor is setup to insert spaces only. Anyway, fixed it.

0xc0170 · 2018-04-10T11:41:18Z

/morph build

mbed-ci · 2018-04-10T12:31:24Z

Build : SUCCESS

Build number : 1705
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6480/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-04-10T16:29:09Z

Exporter Build : SUCCESS

Build number : 1339
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6480/

mbed-ci · 2018-04-11T01:27:51Z

Test : FAILURE

Build number : 1499
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6480/1499

0xc0170 · 2018-04-11T12:56:37Z

/morph test

mbed-ci · 2018-04-12T14:52:43Z

Test : SUCCESS

Build number : 1525
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6480/1525

cmonr · 2018-04-12T15:44:33Z

Waiting on @sg- and @SenRamakri for review.

cmonr · 2018-04-12T23:42:03Z

@davidsaada Also, this looks like it'll need to go through a rebase.

Add the allocate_key API. This replaces the previously added set_alloc_key API (which allocates a key and sets the value at the same time). Reason for the change: Key allocation will typically be used by other storage features (like StorageLite), keeping the allocated keys in another location. Previous API created problems in the case key allocation and value setting couldn't be done at the same time (for instance, if the set value was derived from the allocated key, such as hash or CMAC).

davidsaada · 2018-05-14T16:04:06Z

Rebased following merge of PR #6864. No longer depending on that PR.

cmonr · 2018-05-15T17:00:09Z

/morph build

mbed-ci · 2018-05-15T17:31:57Z

Build : SUCCESS

Build number : 2029
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6480/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-05-16T05:16:50Z

Test : FAILURE

Build number : 1835
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6480/1835

mbed-ci · 2018-05-16T07:59:37Z

Exporter Build : FAILURE

Build number : 1678
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6480/

cmonr · 2018-05-16T21:08:11Z

/morph test

cmonr · 2018-05-16T21:08:55Z

/morph export-build

cmonr · 2018-05-16T21:24:17Z

/morph test

cmonr · 2018-05-16T21:25:14Z

/morph test

mbed-ci · 2018-05-17T02:43:13Z

Test : SUCCESS

Build number : 1849
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6480/1849

mbed-ci · 2018-05-17T05:34:59Z

Test : SUCCESS

Build number : 1851
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6480/1851

mbed-ci · 2018-05-17T10:58:06Z

Exporter Build : SUCCESS

Build number : 1687
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6480/

adbridge · 2018-05-21T14:06:46Z

@SenRamakri @0xc0170 Are you happy with the updates ?

cmonr · 2018-05-21T22:31:55Z

Looks like @0xc0170 was already good with the changes.

0xc0170 added the needs: review label Mar 27, 2018

0xc0170 requested a review from SenRamakri March 27, 2018 14:47

theotherjimmy reviewed Mar 29, 2018

View reviewed changes

theotherjimmy added the release-version: 5.9.0-rc1 label Mar 29, 2018

0xc0170 requested a review from sg- April 3, 2018 10:26

0xc0170 requested changes Apr 4, 2018

View reviewed changes

davidsaada force-pushed the david_nvstore_alloc_key branch from 2155e86 to f0c9f19 Compare April 4, 2018 21:16

0xc0170 previously approved these changes Apr 5, 2018

View reviewed changes

davidsaada dismissed 0xc0170’s stale review via 49cab79 April 8, 2018 11:04

davidsaada force-pushed the david_nvstore_alloc_key branch 2 times, most recently from 49cab79 to c6d2bdd Compare April 8, 2018 11:14

0xc0170 previously approved these changes Apr 10, 2018

View reviewed changes

cmonr added ready for merge and removed needs: review labels Apr 12, 2018

cmonr added needs: work and removed ready for merge labels Apr 12, 2018

davidsaada dismissed 0xc0170’s stale review via 3634bf5 April 13, 2018 06:14

cmonr added needs: preceding PR release-version: 5.9.0-rc1 and removed do not merge labels May 14, 2018

davidsaada force-pushed the david_nvstore_alloc_key branch from ead4901 to c90182a Compare May 14, 2018 16:01

0xc0170 removed the needs: preceding PR label May 15, 2018

0xc0170 approved these changes May 15, 2018

View reviewed changes

davidsaada mentioned this pull request May 15, 2018

Add the StorageLite feature #6915

Closed

cmonr added needs: CI BREAKING-CHANGE labels May 15, 2018

0xc0170 added needs: review and removed needs: CI labels May 17, 2018

SenRamakri approved these changes May 21, 2018

View reviewed changes

cmonr merged commit db73ed0 into ARMmbed:master May 21, 2018

cmonr removed the needs: review label May 21, 2018

davidsaada deleted the david_nvstore_alloc_key branch July 9, 2018 13:12

NVStore: add the allocate_key API (instead of set_alloc_key) #6480

NVStore: add the allocate_key API (instead of set_alloc_key) #6480

Uh oh!

Conversation

davidsaada commented Mar 27, 2018 • edited by theotherjimmy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidsaada commented Apr 2, 2018

Uh oh!

davidsaada commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidsaada commented Apr 3, 2018

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Apr 10, 2018

Uh oh!

mbed-ci commented Apr 10, 2018

Triggering tests

Uh oh!

mbed-ci commented Apr 10, 2018

Uh oh!

mbed-ci commented Apr 11, 2018

Uh oh!

0xc0170 commented Apr 11, 2018

Uh oh!

mbed-ci commented Apr 12, 2018

Uh oh!

cmonr commented Apr 12, 2018

Uh oh!

cmonr commented Apr 12, 2018

Uh oh!

davidsaada commented May 14, 2018

Uh oh!

cmonr commented May 15, 2018

Uh oh!

mbed-ci commented May 15, 2018

Triggering tests

Uh oh!

mbed-ci commented May 16, 2018

Uh oh!

mbed-ci commented May 16, 2018

Uh oh!

cmonr commented May 16, 2018

Uh oh!

cmonr commented May 16, 2018

Uh oh!

cmonr commented May 16, 2018

Uh oh!

cmonr commented May 16, 2018

Uh oh!

mbed-ci commented May 17, 2018

Uh oh!

mbed-ci commented May 17, 2018

Uh oh!

davidsaada commented Mar 27, 2018 •

edited by theotherjimmy

Loading

davidsaada commented Apr 3, 2018 •

edited

Loading

0xc0170 commented Apr 3, 2018 •

edited

Loading