Skip to content

Expose keys & erase_all fns on EspKeyValueStorage #641

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.

Sign up for GitHub

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

Open
JacksonUtsch wants to merge 3 commits into
base: master
Choose a base branch
from
+10 −0
Open

Expose keys & erase_all fns on EspKeyValueStorage #641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8d47f2c
Expose keys & erase_all fns on EspKeyValueStorage
JacksonUtsch Feb 28, 2026
96c43e4
Update CHANGELOG.md
JacksonUtsch Feb 28, 2026
9e3a0b9
Add cfg check
JacksonUtsch Feb 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Check https://github.com/esp-rs/esp-idf-hal/pull/529 for details on that change
- HTTP: added `task_caps` option into server `Configuration`
- Update `heapless` dependency
- NVS: exposed `keys` and `erase_all` functions on `EspKeyValueStorage`

### Fixed
- `WifiDriver::get_ap_info` not takes `&self` instead of `&mut self`. Convenience method `EspWifi::get_ap_info` that delegates
Expand Down
9 changes: 9 additions & 0 deletions src/nvs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1021,6 +1021,15 @@ impl<T: NvsPartitionId> EspKeyValueStorage<T> {
Ok(true)
}
}

#[cfg(esp_idf_version_at_least_5_2_0)]
pub fn keys(&self, data_type: Option<NvsDataType>) -> Result<EspNvsKeys<'_>, EspError> {

@ivmarkov ivmarkov Mar 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The data_type parameter of this new method does not make sense for EspKeyValueStorage. Also, EspNvsKeys should be wrapped, as its next_key method returns a name + type, but we only care about the name.

Finally, the EspKeyValueStorage type exists to be consumed primarily through the RawStorage trait, so I wonder what's the point of having public methods on the ESP-IDF trait impl, and not on the trait itself?

I mean, if you don't care about being platform-agnostic, or you do it your own way without the embedded-svc storage traits, why using EspKeyValueStorage in the first place? Isn't it better to use the wrapped EspNvs type directly?

@JacksonUtsch JacksonUtsch Mar 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right in that the data_type does not make sense also the changed return type, can just use None inside the function and remove the parameter. See if my wrapping looks correct.

#[cfg(esp_idf_version_at_least_5_2_0)]
pub struct EspKeyValueStorageKeys<'a>(EspNvsKeys<'a>);

#[cfg(esp_idf_version_at_least_5_2_0)]
impl<'a> EspKeyValueStorageKeys<'a> {
    pub fn next_key(&mut self) -> Option<&str> {
        self.0.next_key().map(|k|k.0)
    }
}

#[cfg(esp_idf_version_at_least_5_2_0)]
impl<'a> Drop for EspKeyValueStorageKeys<'a> {
    fn drop(&mut self) {
        drop(&self.0)
    }
}

pub fn keys(&self) -> Result<EspKeyValueStorageKeys<'_>, EspError> {
    self.0.keys(None).map(|k|EspKeyValueStorageKeys(k))
}

I'm not sure the ergonomics of how RawStorage is used. I am just using EspKeyValueStorage directly as I need the ability for arbitrary data.

@ivmarkov ivmarkov Mar 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is EspNvs not fulfilling your arbitrary data requirement?

@JacksonUtsch JacksonUtsch Mar 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You mean get_blob and set_blob specifically? It was failing some reason and I saw the newer API of EspKeyValueStorageKeys and tried it without issues. Why would we have EspKeyValueStorageKeys then?

@ivmarkov ivmarkov Mar 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes - I mean get_blob and set_blob. The EspKeyValueStorage thing IS implemented in terms of these two calls (mostly) after all and the only reason why it exists is so that we implement the RawStorage trait. But then what exactly do you mean by mentioning EspKeyValueStorageKeys in this same context is beyond me. get/set_blob is for reading/writing values. The Keys thing is an iterator over the keys/values which are currently saved. So these two things serve two completely different purposes and why you are conflating those is beyond me.

Also, creating new - potentially questionable APIs - because the existing ones "was failing some reason" is not a good argument for those.

@JacksonUtsch JacksonUtsch Mar 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Whoops, I mean EspKeyValueStorage not EspKeyValueStorageKeys. EspKeyValueStorage is an existing API it would just be exposing the functionalities it did not seem to have which its underlying type has.

I mean why is EspKeyValueStorage a public API if it does not have basic functionalities supported?

I don't know the usefulness of the RawStorage trait but the example code shows using EspKeyValueStorage directly.

You can question the APIs as that is a point of a PR, like those changes I made.

@ivmarkov ivmarkov Mar 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't know the usefulness of the RawStorage trait but the example code shows using EspKeyValueStorage directly.

The example is wrong/out of date and needs to be fixed.

Again: EspKeyValueStorage's purpose in life is to implement the RawStorage trait. Does it implement it successfully? Yes. As to why is it public, well, how would you construct it otherwise? You might ask why it has other public methods then and that's valid kinda, but not like a super-strong argument. Then if you say its API's are not good enough you are basically saying RawStorage APIs are not good enough. Which is valid but then we have to actually extend the RawStorage API as well. Basically this and the second paragraph in there in particular all-over again.

I mean, I don't know how to say it more clearly: if you don't care about RawStorage, just use the damn EspNvs directly! :D
It has a richer API, its methods are transactional (unlike EspKeyValueStorage), EspKeyValueStorage is anyway implemented on top of it so not sure why we are turning in circles here still?

@JacksonUtsch JacksonUtsch Mar 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay if it is not advisable to use EspKeyValueStorage then I will have to try EspNvs again with the blob functions and figure out where the issue is happening. FYI I am using JSON

@ivmarkov ivmarkov Mar 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Json serde via get/set_blob should work just fine.

self.0.keys(data_type)
}

pub fn erase_all(&self) -> Result<(), EspError> {
self.0.erase_all()
}
}

impl<T: NvsPartitionId> StorageBase for EspKeyValueStorage<T> {
Expand Down