refactor: Replace Arduino HTTP Client with esp native#391
Conversation
|
There was a problem hiding this comment.
Pull request overview
This pull request replaces the Arduino HTTP Client library with the ESP-IDF native HTTP client implementation. The change represents a significant architectural shift towards using ESP32's built-in networking capabilities, improving platform integration and potentially reducing dependency on third-party libraries.
Key Changes:
- Introduced new HTTP client architecture with
HTTPClient,HTTPClientState,HTTPResponse, andJsonResponseclasses - Replaced string_view URL parameters with const char* for HTTP client APIs
- Refactored rate limiting into a separate
RateLimitersmodule - Removed the old
HTTPRequestManagerimplementation and replaced all usages throughout the codebase
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/util/DomainUtils.cpp | New utility to extract domain from URL (critical bug: missing assignment on line 11) |
| src/util/ParitionUtils.cpp | Updated to use new HTTP client; logic error in status code check (line 63) |
| src/serialization/JsonAPI.cpp | Removed unused code parameter from JSON parsing functions |
| src/serial/command_handlers/domain.cpp | Updated to use new HTTP client with error handling |
| src/serial/command_handlers/authtoken.cpp | Updated to use new HTTP client in scoped block |
| src/serial/SerialInputHandler.cpp | Removed unused import |
| src/http/RateLimiters.cpp | New rate limiter management module extracted from old implementation |
| src/http/JsonAPI.cpp | Refactored to use new HTTP client, simplified header setting |
| src/http/HTTPRequestManager.cpp | Deleted - replaced by new implementation |
| src/http/HTTPClientState.cpp | New ESP-IDF HTTP client state management (bug in constructor initialization line 21, log message bug line 181) |
| src/http/HTTPClient.cpp | New HTTP client facade providing Get and GetJson methods |
| src/OtaUpdateManager.cpp | Updated all HTTP requests to use new client |
| src/GatewayConnectionManager.cpp | Updated API calls to use new HTTP client with improved error handling |
| include/util/PartitionUtils.h | Changed parameter type from string_view to const char* |
| include/util/DomainUtils.h | New header for domain extraction utility |
| include/serialization/JsonAPI.h | Updated parsing function signatures to remove code parameter, forward declared cJSON |
| include/http/*.h | New HTTP client interface headers (HTTPError.h has critical bug: empty error strings) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
hhvrc
added
the
status: blocked
|
Cannot merge until we switch over to ESP-IDF due to inability configure global certs when arduino es-idf framework has precompiled binaries |
hhvrc
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 11 comments.
Comments suppressed due to low confidence (1)
platformio.ini:48
- These configuration flags completely disable SSL/TLS certificate verification, making all HTTPS connections vulnerable to man-in-the-middle attacks. This is a serious security vulnerability that should not be enabled in production code.
Consider using proper certificate validation with either a CA bundle or certificate pinning instead of disabling verification entirely.
-DCONFIG_ESP_TLS_INSECURE=y
-DCONFIG_ESP_TLS_SKIP_SERVER_CERT_VERIFY=y
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| HTTPClient(const char* url, uint32_t timeoutMs = 10'000) | ||
| : m_state(std::make_shared<HTTPClientState>(url, timeoutMs)) | ||
| { | ||
| } |
There was a problem hiding this comment.
The new HTTPClient implementation does not integrate with the rate limiting system. The old implementation called _getRateLimiter() and checked rateLimiter->tryRequest() before making requests. Without this integration, the application may exceed API rate limits and get blocked by the server. Consider integrating the RateLimiters::GetRateLimiter functionality into the HTTPClient.
| OS_LOGE(TAG, "Unexpected response code: %d", response.code); | ||
| auto content = response.ReadJson(); | ||
| if (content.error != HTTP::HTTPError::None) { | ||
| OS_LOGE(TAG, "Error while reading response: %s %d", HTTP::HTTPErrorToString(response.Error()), response.StatusCode()); |
There was a problem hiding this comment.
Same issue - using response.Error() when the error occurred during content reading. Should use content.error instead.
| OS_LOGE(TAG, "Error while reading response: %s %d", HTTP::HTTPErrorToString(response.Error()), response.StatusCode()); | |
| OS_LOGE(TAG, "Error while reading response: %s %d", HTTP::HTTPErrorToString(content.error), response.StatusCode()); |
| OS_LOGE(TAG, "Unexpected response code: %d", response.code); | ||
| auto content = response.ReadJson(); | ||
| if (content.error != HTTP::HTTPError::None) { | ||
| OS_LOGE(TAG, "Error while reading response: %s %d", HTTP::HTTPErrorToString(response.Error()), response.StatusCode()); |
There was a problem hiding this comment.
Same issue - using response.Error() when the error occurred during content reading. Should use content.error instead.
| OS_LOGE(TAG, "Error while reading response: %s %d", HTTP::HTTPErrorToString(response.Error()), response.StatusCode()); | |
| OS_LOGE(TAG, "Error while reading response: %s %d", HTTP::HTTPErrorToString(content.error), response.StatusCode()); |
| OS_LOGI(TAG, "Successfully connected to \"%.*s\", version: %s, commit: %s, current time: %s", arg.length(), arg.data(), resp.data.version.c_str(), resp.data.commit.c_str(), resp.data.currentTime.c_str()); | ||
| auto content = response.ReadJson(); | ||
| if (content.error != OpenShock::HTTP::HTTPError::None) { | ||
| SERPR_ERROR("Tried to read response from backend, but failed (%s), refusing to save domain to config", OpenShock::HTTP::HTTPErrorToString(response.Error())); |
There was a problem hiding this comment.
The error message calls response.Error() but should call content.error since the error occurred during the ReadJson operation, not during the initial request. The response's Error() method may not reflect the actual error that occurred during content reading.
| SERPR_ERROR("Tried to read response from backend, but failed (%s), refusing to save domain to config", OpenShock::HTTP::HTTPErrorToString(response.Error())); | |
| SERPR_ERROR("Tried to read response from backend, but failed (%s), refusing to save domain to config", OpenShock::HTTP::HTTPErrorToString(content.error)); |
|
|
||
| auto content = response.ReadString(); | ||
| if (content.error != HTTP::HTTPError::None) { | ||
| OS_LOGE(TAG, "Failed to fetch firmware version: %s [%u] %s", HTTP::HTTPErrorToString(response.Error()), response.StatusCode(), content.data.c_str()); |
There was a problem hiding this comment.
Same issue as line 444 - attempting to log content.data.c_str() when content.error indicates an error occurred. The content data may be empty or invalid in this case.
| OS_LOGE(TAG, "Failed to fetch firmware version: %s [%u] %s", HTTP::HTTPErrorToString(response.Error()), response.StatusCode(), content.data.c_str()); | |
| OS_LOGE(TAG, "Failed to fetch firmware version: %s [%u]", HTTP::HTTPErrorToString(response.Error()), response.StatusCode()); |
| OS_LOGE(TAG, "Unexpected response code: %d", response.code); | ||
| auto content = response.ReadJson(); | ||
| if (content.error != HTTP::HTTPError::None) { | ||
| OS_LOGE(TAG, "Error while reading response: %s %d", HTTP::HTTPErrorToString(response.Error()), response.StatusCode()); |
There was a problem hiding this comment.
The error message calls response.Error() when the error actually occurred during ReadJson(), so it should reference content.error instead. The response's Error() method will likely return HTTPError::None since the initial request succeeded.
| OS_LOGE(TAG, "Error while reading response: %s %d", HTTP::HTTPErrorToString(response.Error()), response.StatusCode()); | |
| OS_LOGE(TAG, "Error while reading response: %s %d", HTTP::HTTPErrorToString(content.error), response.StatusCode()); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Cpp-Linter Report
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
This PR is blocked until we can get a global CA store imported. |
…ino-httpclient # Conflicts: # include/http/HTTPRequestManager.h # src/GatewayConnectionManager.cpp # src/OtaUpdateManager.cpp # src/http/HTTPRequestManager.cpp # src/util/ParitionUtils.cpp
|
Ready to review this PR? Stage has broken it down into 9 individual chapters for you: 
Chapters generated by Stage for commit 33144d6 on Jun 29, 2026 10:37am UTC. |
No description provided.