Conversation
erase_all functionality or alternatively listing all stores keys
src/lib.rs
Outdated
| /// # Errors | ||
| /// | ||
| /// If a flash error occured or the namespace is malformed/not found | ||
| pub fn list_keys(&mut self, namespace: &Key) -> Result<Vec<Key>, Error> { |
There was a problem hiding this comment.
Would be nice if we could eliminate allocating a Vec<Key> here if we're not using the keys to manipulate the &mut self when iterating them.
There was a problem hiding this comment.
I would be in favor of returning an impl Iterator<Item = Result<&Key, Error>> here and let the user decide if they need a Vec or not.
What do you think about making the namespace an Option<&Key> as we only filter after loading the item anyway? It would be good to have the option to just return all impl Iterator<Item = Result(&Key, &Key), Error>> and let the user do any filtering they want
There was a problem hiding this comment.
Returning &Key would be ideal, but not sure how to do that, given that load_item returns an owned item.
erase_all functionality or alternatively listing all stores keys
src/lib.rs
Outdated
| /// # Errors | ||
| /// | ||
| /// If a flash error occured or the namespace is malformed/not found | ||
| pub fn list_keys(&mut self, namespace: &Key) -> Result<Vec<Key>, Error> { |
There was a problem hiding this comment.
I would be in favor of returning an impl Iterator<Item = Result<&Key, Error>> here and let the user decide if they need a Vec or not.
What do you think about making the namespace an Option<&Key> as we only filter after loading the item anyway? It would be good to have the option to just return all impl Iterator<Item = Result(&Key, &Key), Error>> and let the user do any filtering they want
e096957 to
e7a51e3
Compare
|
In case you are wondering about the handwritten iterators: Code like pub fn keys(&mut self) -> impl Iterator<Item = Result<(u8, Key), Error>> {
self.pages.iter().flat_map(|page| {
page.items(&mut self.hal).map(move |result| {
let Ok(value) = result else {
// TODO: Replace with match to return actual error
return Err(Error::FlashError);
};
Ok((value.namespace_index, value.key))
})
})
}doesn't work, and the compiler rejects it with error: captured variable cannot escape `FnMut` closure body
--> src\lib.rs:222:13
|
220 | pub fn keys(&mut self) -> impl Iterator<Item = Result<(u8, Key), Error>> {
| --------- variable defined here
221 | self.pages.iter().flat_map(|page| {
| - inferred to be a `FnMut` closure
222 | page.items(&mut self.hal).map(move |result| {
| ^ ---- variable captured here
| _____________|
| |
223 | | let Ok(value) = result else {
224 | | // TODO: Replace with match to return actual error
225 | | return Err(Error::FlashError);
... |
228 | | Ok((value.namespace_index, value.key))
229 | | })
| |______________^ returns a closure that contains a reference to a captured variable, which then escapes the closure body
|
= note: `FnMut` closures only have access to their captured variables while they are executing...
= note: ...therefore, they cannot allow references to captured variables to escape
error: could not compile `esp-nvs` (lib) due to 1 previous errorThe issue is that |
|
Did some more investigating regarding the above, and it looks like this PR isn't ready yet. By now I have found two mistakes:
|
|
There were more mistakes, and now I understand why my naive implementation didn't erase things correctly. I have added a test for iterating through the namespaces, and one for the In it, I have the items: [src\lib.rs:195:17] item = Ok(
Item {
namespace_index: 1,
type_: BlobData,
span: 2,
chunk_index: 0,
crc: 580535695,
key: Key(b"init"),
},
)
[src\lib.rs:195:17] item = Ok(
Item {
namespace_index: 1,
type_: BlobIndex,
span: 1,
chunk_index: 255,
crc: 2801133294,
key: Key(b"init"),
},
)
[src\lib.rs:195:17] item = Ok(
Item {
namespace_index: 1,
type_: BlobData,
span: 2,
chunk_index: 0,
crc: 587524443,
key: Key(b"max_pos"),
},
)
[src\lib.rs:195:17] item = Ok(
Item {
namespace_index: 1,
type_: BlobIndex,
span: 1,
chunk_index: 255,
crc: 2390569967,
key: Key(b"max_pos"),
},
)
[src\lib.rs:195:17] item = Ok(
Item {
namespace_index: 1,
type_: BlobData,
span: 2,
chunk_index: 128,
crc: 2049146457,
key: Key(b"cur_pos"),
},
)
[src\lib.rs:195:17] item = Ok(
Item {
namespace_index: 1,
type_: BlobIndex,
span: 1,
chunk_index: 255,
crc: 2605027038,
key: Key(b"cur_pos"),
},
)
[src\lib.rs:195:17] item = Ok(
Item {
namespace_index: 2,
type_: BlobData,
span: 2,
chunk_index: 0,
crc: 541344424,
key: Key(b"init"),
},
)
[src\lib.rs:195:17] item = Ok(
Item {
namespace_index: 2,
type_: BlobIndex,
span: 1,
chunk_index: 255,
crc: 2754339273,
key: Key(b"init"),
},
)
[src\lib.rs:195:17] item = Ok(
Item {
namespace_index: 2,
type_: BlobData,
span: 67,
chunk_index: 0,
crc: 3540989349,
key: Key(b""),
},
)
[src\lib.rs:195:17] item = Ok(
Item {
namespace_index: 2,
type_: BlobIndex,
span: 1,
chunk_index: 255,
crc: 1216488744,
key: Key(b""),
},
)
[src\lib.rs:195:17] item = Ok(
Item {
namespace_index: 2,
type_: BlobData,
span: 2,
chunk_index: 0,
crc: 1092296233,
key: Key(b""),
},
)
[src\lib.rs:195:17] item = Ok(
Item {
namespace_index: 2,
type_: BlobIndex,
span: 1,
chunk_index: 255,
crc: 3415319792,
key: Key(b""),
},
)and the [
Ok((Key(b"window-covering"), Key(b"init"))),
Ok((Key(b"window-covering"), Key(b"max_pos"))),
Ok((Key(b"window-covering"), Key(b"cur_pos"))),
Ok((Key(b"matter"), Key(b"init"))),
Ok((Key(b"matter"), Key(b""))),
Ok((Key(b"matter"), Key(b""))),
]I am not sure about the blobs, ideally the iterator would only return once for each key and not multiple times. There are other things I am uncertain about like currently it ignores the state of the page, do I have to skip items where the crc is invalid?, does the Update: I have looked at the esp-idf implementation and handled blobs like they do. Not sure about my real-world nvs which still returns the same key twice, but that might be a bug somewhere else or expected behavior. |
|
Forgot to mark this as ready for review. |
|
The duplicates you encountered may stem from the bug fixed in 9d51b50. I also just noticed that while I released 0.2.0 as crate, I forgot to tag it in the repo... Could you please rebase the PR? |
f63a4db to
3c7f75c
Compare
e64be2a to
0c68529
Compare

This PR implements parts of #6. An erase method is missing, but users can implement this by iterating through the keys and then removing them.