docs: document intentional masking in get_str() behavior

[gdellis]

Summary

Document intentional behavior in Config::get_str() - when key is not found, returns default value and ESP_OK.

Changes

• firmware/main/config.hpp: Add docstring note to get_device_name() explaining that callers who need to distinguish "stored value" vs "default" should use nvs_get_str() directly.

This is intentional design per issue #49 discussion - the helper simplifies common use case at cost of masking "not found" condition.

Closes #49

[github-actions]

PR #67 Review: "docs: document intentional masking in get_str() behavior"

CI Status

Most checks still appear in progress at time of this review. Lint checks passed, but Build Firmware and Host Tests are pending.

---

Issue 1: Misleading PR Title and Commit Message

Severity: [MEDIUM]

The PR title claims this is a documentation-only change to get_str() behavior, but the diff includes:

File	Actual Changes
ble.hpp	static_assert for struct sizes + [[nodiscard]] on methods
state_machine.cpp	ESP_ERROR_CHECK() wrapping ble_.send_alert()

These are code changes, not documentation. The commit message is misleading.

---

Issue 2: Duplicate Changes from Other Open PRs

Severity: [MEDIUM]

The changes in this PR duplicate work in other open PRs:

This PR	Duplicate of	Title
static_assert + [[nodiscard]] in ble.hpp	#65	fix: add static_assert for BLE structs and [[nodiscard]] to methods
ESP_ERROR_CHECK in state_machine.cpp	#64	fix: add NULL check to nmea_validate_checksum() (though this is different)

This creates merge conflict risk if both PRs are merged.

---

Issue 3: ESP_ERROR_CHECK in state_machine.cpp:224 - Unchecked Return Ignored on Purpose

Severity: [HIGH]

ESP_ERROR_CHECK (ble_.send_alert (alert));

Using ESP_ERROR_CHECK() here will cause an immediate panic/reset if ble_.send_alert() ever returns an error (e.g., BLE disconnected). In a geofence breach handler, this seems intentionally aggressive, but:

1. The code previously ignored the return value silently
2. Changing to ESP_ERROR_CHECK changes runtime behavior from "continue on error" to "crash on any error"

If BLE is temporarily disconnected during a geofence breach alert, the device will reset instead of retrying or logging the failure.

Consider: Is this the intended behavior for production? A more graceful handling might be:

• Log the error
• Set a flag to retry later
• Only panic if repeated failures occur

---

Issue 4: Missing Docstring Format Consistency

Severity: [LOW]

The new docstring in config.hpp:45-53 uses @note which is good, but other methods in this class lack docstrings:

// Line 54 - has docstring
static esp_err_t get_device_name (char* name, size_t max_len);

// Line 55 - no docstring
static esp_err_t set_device_name (const char* name);

Consider: Adding docstrings to set_device_name() for consistency, or clarifying in the existing docstring whether set_device_name() has the same "returns ESP_OK even on not-found" behavior.

---

Issue 5: static_assert Messages Could Be More Descriptive

Severity: [LOW]

static_assert(sizeof(BleLocationData) == 17, "BleLocationData must be 17 bytes for BLE");
static_assert(sizeof(BleAlertData) == 18, "BleAlertData must be 18 bytes for BLE");

The messages say "must be 17/18 bytes" but don't explain why this size is required (e.g., "BLE GATT characteristic requires fixed-size payload").

---

Summary

Category	Count
[BUG]	0
[HIGH]	1 (ESP_ERROR_CHECK panic behavior)
[MEDIUM]	2 (misleading PR title, duplicate changes)
[LOW]	2 (docstring consistency, assert messages)

Recommendation: The PR description should be updated to reflect all code changes, not just the documentation addition. Consider splitting this into separate PRs for clarity, or merging related changes that belong together logically.

opencode session  |  github run