Expose keys & erase_all fns on EspKeyValueStorage

[JacksonUtsch]

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

• I have updated existing examples or added new ones (if applicable).
• I have used cargo fmt command to ensure that all changed code is formatted correctly.
• I have used cargo clippy command to ensure that all changed code passes latest Clippy nightly lints.
• My changes were added to the CHANGELOG.md in the proper section.

Pull Request Details 📖

Expose keys & erase_all functions on EspKeyValueStorage

Description

No direct way to get keys and erase_all via EspKeyValueStorage. Enables the capabilities

Testing

Runs locally, uses internal API.

[ivmarkov]

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]

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]

Why is EspNvs not fulfilling your arbitrary data requirement?

[JacksonUtsch]

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]

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]

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]

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]

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]

Json serde via get/set_blob should work just fine.