Fixes issue #585 -> Resolve contains() method returning false for existing string values by danielmeza · Pull Request #588 · esp-rs/esp-idf-svc

danielmeza · 2025-07-25T19:07:57Z

Fixes issue #585 where EspNvs::contains() incorrectly returned false for string values that actually existed in NVS partitions. The root cause was that contains() only checked for u64 and blob data types, never checking for string values stored with nvs_set_str().

Changes:

Replace broken len()-based contains() with nvs_get_type() implementation
Add contains_key_of_type() method for type-specific key checking
Add find_key_type() method to retrieve key data types
Add NvsDataType enum with From<nvs_type_t> conversion
Introduce EspKeyValueStorage wrapper for consistent serialized storage behavior

The new contains() implementation uses nvs_get_type() to detect any key type, ensuring consistent behavior across all NVS data types (u8, u16, u32, u64, i8, i16, i32, i64, str, blob).

Fixes #585

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 📖

Description

Please provide a clear and concise description of your changes, including the motivation behind these changes. The context is crucial for the reviewers.

Testing

Describe how you tested your changes.

…ng values Fixes issue esp-rs#585 where EspNvs::contains() incorrectly returned false for string values that actually existed in NVS partitions. The root cause was that contains() only checked for u64 and blob data types, never checking for string values stored with nvs_set_str(). Changes: - Replace broken len()-based contains() with nvs_get_type() implementation - Add contains_key_of_type() method for type-specific key checking - Add find_key_type() method to retrieve key data types - Add NvsDataType enum with From<nvs_type_t> conversion - Introduce EspKeyValueStorage wrapper for consistent serialized storage behavior The new contains() implementation uses nvs_get_type() to detect any key type, ensuring consistent behavior across all NVS data types (u8, u16, u32, u64, i8, i16, i32, i64, str, blob). Fixes esp-rs#585

Added Debug, Clone, Copy, PartialEq, and Eq derives to the NvsDataType enum. Removed redundant comments from error handling branches in EspNvs methods for improved code clarity.

Refactored the logic in key existence and type checking methods to use the new find_key_type helper, reducing code duplication and improving readability.

Refactored EspKeyValueStorage to use a generic NvsPartitionId and updated its RawStorage implementation. Fixed derive attribute typo for NvsDataType and removed redundant code in contains and remove methods. Improved code clarity and reduced duplication by consolidating remove logic into EspNvs and updating field accesses.

Corrects spacing in field accesses (self.0.1 to self.0 .1) to comply with Rust's parsing rules and avoid ambiguity. No functional changes, only syntax clarification.

Corrects handle usage in NVS erase and commit operations by removing incorrect tuple field access. Updates the default build target in .cargo/config.toml to 'xtensa-esp32-espidf' and comments out the previous RISC-V target.

Replaces calls to EspNvs methods with EspKeyValueStorage methods in the StorageBase implementation, ensuring the correct struct's methods are invoked.

The NvsDataType enum now uses #[repr(u8)] to ensure its discriminants match the underlying type. The From<nvs_type_t> implementation is simplified to a direct cast, reducing boilerplate and improving maintainability.

Introduces a public remove method to EspKeyValueStorage, allowing removal of a key by name and returning a Result indicating success.

Added #[allow(non_upper_case_globals)] to suppress lint warnings in the From<nvs_type_t> implementation for NvsDataType.

Added conditional compilation attributes to the contains, contains_key_of_type, and find_key_type methods in EspNvs to restrict their availability based on esp-idf version. Also updated example to use EspKeyValueStorage instead of EspNvs for raw storage operations.

Copilot

Pull Request Overview

This pull request fixes issue #585 where the EspNvs::contains() method incorrectly returned false for string values that existed in NVS partitions. The fix involves replacing the broken length-based contains() implementation with a proper type-checking approach using ESP-IDF's nvs_find_key() API.

Key changes:

Replaces the faulty len()-based contains() method with nvs_find_key() implementation that works across all NVS data types
Introduces EspKeyValueStorage wrapper for consistent serialized storage behavior, maintaining the original raw storage functionality
Adds comprehensive type checking utilities including NvsDataType enum and related helper methods

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
src/nvs.rs	Main implementation - fixes contains() method and adds EspKeyValueStorage wrapper with type checking utilities
examples/nvs_get_set_raw_storage.rs	Updates example to use new EspKeyValueStorage instead of EspNvs
.cargo/config.toml	Changes build target from RISC-V to Xtensa ESP32

ivmarkov

Yes, this is the direction. Some small suggestions and nits.

ivmarkov · 2025-07-26T09:20:09Z

+/// - **Large values (>7 bytes)**: Stored as ESP-IDF blobs
+/// - **Memory efficient**: No unnecessary allocations for small values
+/// - **Flash efficient**: Optimized storage format reduces wear on flash memory
+pub struct EspKeyValueStorage<T: NvsPartitionId>(EspNvs<T>);


Can you move this struct declaration right above its impl block? This is the current practice in the esp-idf-* crate.

Yes!! moved.

based on the build issue, we have to keep the old implementation for IDF v5.2 blow, since nvs_find_key is only available from v5.2+

How about the following idea instead:

Since find_key would only be available for IDF v5.3+ (or suchlike), we need an alternative implementation of EspKeyValueStorage that works on older IDFs

However this does not mean we necessarily need to revert to the old implementation, which used to call the raw NVS C APIs from esp-idf-sys

We rather need an implementation that - while not relying on find_key - calls only into safe APIs of EspNvs

This is all possible as the old impl did exactly that. I just want the new impl to do it based on safe EspNvs APIs that's all

I think this is completely possible? We just have "simulate" the missing "contains" method by first calling EspNvs::get_u64 - and if that fails with an error - call EspNvs::get_blob.

Right?

Let's address this ^^^ too.

I create 2 implementations, first one v5+ uses find_key and the seconds for v5- uses the safe api and simulate the contains like the old way but this time using the safe API. So I think it should be covered.

Or do we want to keep a single version of this?

Just keep a single version - the old one.

Updated NvsDataType to use C constant values and changed its repr to u32 for better FFI compatibility. Deprecated EspNvs::contains in favor of find_key, which now returns the type of the key. Refactored EspKeyValueStorage to use the new find_key method, simplified raw get/set logic, and improved type safety and clarity in storage operations.

…entation

Introduces conditional implementations of EspKeyValueStorage for different ESP-IDF versions, ensuring compatibility with both v4 and v5.1. Adds trait impls for StorageBase and RawStorage for references to EspKeyValueStorage, and refactors set_raw and remove methods for version-specific mutability requirements.

Simplifies the code by removing an unnecessary return statement in the None branch of the match, directly returning the result of self.0.blob_len(name).

ivmarkov · 2025-07-28T06:51:12Z

Current CI fails because of a no-longer necessary mut in the examples. Can you fix this?

Also see my earlier comment on how the EspKeyValueStorage should work - ideally, it should not rely on find_key, and thus it should be available for earlier IDFs as well.

The 'storage' variable no longer needs to be mutable since it is not mutated after initialization. This simplifies the code and clarifies intent.

Deleted the EspKeyValueStorage implementation for ESP-IDF versions other than 4.x and 5.1, leaving only the version-specific implementation. This simplifies the codebase and removes conditional compilation for unsupported or unused ESP-IDF versions.

ivmarkov

I'll remove the impl<T: NvsPartitionId> StorageBase for &EspKeyValueStorage<T> { as well as the nvs() method, for the reasons already mentioned, but other than that, LGTM! And thanks!

danielmeza added 2 commits July 25, 2025 14:05

Fix format

4f82b9b

Copilot AI review requested due to automatic review settings July 25, 2025 19:07

danielmeza marked this pull request as draft July 25, 2025 19:08

This comment was marked as outdated.

Sign in to view

Add derives to NvsDataType and clean up error handling

f0e1fff

Added Debug, Clone, Copy, PartialEq, and Eq derives to the NvsDataType enum. Removed redundant comments from error handling branches in EspNvs methods for improved code clarity.

danielmeza mentioned this pull request Jul 25, 2025

ESP-IDF-SVC NVS contains() Bug Report - String Values Not Detected #585

Closed

danielmeza changed the title ~~fix(nvs): resolve contains() method returning false for existing string values~~ Fixes issue #585 -> Resolve contains() method returning false for existing string values Jul 25, 2025

danielmeza added 14 commits July 25, 2025 14:30

Update nvs.rs

ce96f7a

Refactor key type checks to use find_key_type helper

b73d037

Refactored the logic in key existence and type checking methods to use the new find_key_type helper, reducing code duplication and improving readability.

Fix format

fb885ec

Fix field access spacing in EspKeyValueStorage

2cb5965

Corrects spacing in field accesses (self.0.1 to self.0 .1) to comply with Rust's parsing rules and avoid ambiguity. No functional changes, only syntax clarification.

Fix NVS handle usage and update target architecture

b1738d0

Corrects handle usage in NVS erase and commit operations by removing incorrect tuple field access. Updates the default build target in .cargo/config.toml to 'xtensa-esp32-espidf' and comments out the previous RISC-V target.

Fix spacing in nvs_erase_key call

7f21855

Fix trait method calls to use correct struct

064c83e

Replaces calls to EspNvs methods with EspKeyValueStorage methods in the StorageBase implementation, ensuring the correct struct's methods are invoked.

Add repr(u8) to NvsDataType and simplify conversion

04927e4

The NvsDataType enum now uses #[repr(u8)] to ensure its discriminants match the underlying type. The From<nvs_type_t> implementation is simplified to a direct cast, reducing boilerplate and improving maintainability.

Add remove method to EspKeyValueStorage

f52b312

Introduces a public remove method to EspKeyValueStorage, allowing removal of a key by name and returning a Result indicating success.

Restore convertion

3e53e4e

Fix format

5a5b169

Allow non-upper case globals in NvsDataType impl

24cfb55

Added #[allow(non_upper_case_globals)] to suppress lint warnings in the From<nvs_type_t> implementation for NvsDataType.

danielmeza marked this pull request as ready for review July 26, 2025 01:32

danielmeza requested a review from Copilot July 26, 2025 01:32

Copilot AI reviewed Jul 26, 2025

View reviewed changes

Comment thread src/nvs.rs Outdated

Comment thread src/nvs.rs

Comment thread src/nvs.rs

ivmarkov requested changes Jul 26, 2025

View reviewed changes

Comment thread src/nvs.rs Outdated

Comment thread src/nvs.rs Outdated

Update target configuration and refactor NVS key-value storage implem…

52863a1

…entation

ivmarkov reviewed Jul 26, 2025

View reviewed changes

Comment thread src/nvs.rs

danielmeza added 8 commits July 26, 2025 13:46

Remove unused import

5a26746

Remove mutability from base remove() method

65766bd

Update nvs_get_set_raw_storage.rs

bbad284

Format code

d328391

Remove redundant return statement in blob_len call

7207fe6

Simplifies the code by removing an unnecessary return statement in the None branch of the match, directly returning the result of self.0.blob_len(name).

Update nvs.rs

7116cb8

Update nvs_get_set_raw_storage.rs

2815a89

danielmeza added 2 commits July 28, 2025 08:34

Remove mutable binding from storage variable

d07f4c2

The 'storage' variable no longer needs to be mutable since it is not mutated after initialization. This simplifies the code and clarifies intent.

danielmeza requested a review from ivmarkov July 30, 2025 17:33

ivmarkov approved these changes Jul 30, 2025

View reviewed changes

ivmarkov merged commit 6f6c2c8 into esp-rs:master Jul 30, 2025
20 checks passed

Uh oh!

Conversation

danielmeza commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thank you for your contribution!

Submission Checklist 📝

Pull Request Details 📖

Description

Testing

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ivmarkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ivmarkov commented Jul 28, 2025

Uh oh!

ivmarkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danielmeza commented Jul 25, 2025 •

edited

Loading