Conversation
I'd prefer to be able to distinguish between the different types easily in support chat.
…o feat/arduino-3.0
Merge develop into feat/arduino-3.0. Replace remaining tcb::span with std::span, add missing WiFi.h include. Build not yet clean — remaining issues: Serial namespace rename (SerialCmds), USBSerial API, and CaptivePortalConfig struct changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Migrates the firmware project to Arduino 3.0 (via pioarduino’s espressif32 platform), updating APIs impacted by the new core/IDF and modernizing several internal interfaces.
Changes:
- Switch project platform to pioarduino’s Arduino 3.0–based espressif32 and adjust related dependencies/tools.
- Replace the bundled
tcb::spanwith C++20std::spanacross HTTP/WebSocket/serialization/captive portal APIs. - Update Serial command handler namespace usage and refactor RMT-based transmit code for Arduino 3.0.
Reviewed changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/wifi/WiFiManager.cpp | Adjust WiFi init and IP formatting |
| src/util/Base64Utils.cpp | Switch to std::span in encode |
| src/serial/command_handlers/version.cpp | Serial command handler namespace update |
| src/serial/command_handlers/validgpios.cpp | Serial command handler namespace update |
| src/serial/command_handlers/sysinfo.cpp | Remove IPv6 reporting; namespace update |
| src/serial/command_handlers/rftxpin.cpp | Serial command handler namespace update |
| src/serial/command_handlers/rftransmit.cpp | Serial command handler namespace update |
| src/serial/command_handlers/restart.cpp | Serial command handler namespace update |
| src/serial/command_handlers/rawconfig.cpp | Serial command handler namespace update |
| src/serial/command_handlers/networks.cpp | Update TAG + namespace update |
| src/serial/command_handlers/keepalive.cpp | Serial command handler namespace update |
| src/serial/command_handlers/jsonconfig.cpp | Serial command handler namespace update |
| src/serial/command_handlers/hostname.cpp | Update TAG + namespace update |
| src/serial/command_handlers/factoryreset.cpp | Serial command handler namespace update |
| src/serial/command_handlers/estop.cpp | Serial command handler namespace update |
| src/serial/command_handlers/echo.cpp | Serial command handler namespace update |
| src/serial/command_handlers/domain.cpp | Update TAG + namespace update |
| src/serial/command_handlers/authtoken.cpp | Serial command handler namespace update |
| src/serial/command_handlers/CommandEntry.cpp | Update using namespace to SerialCmds |
| src/serial/SerialInputHandler.cpp | Use SerialCmds command groups/registry |
| src/radio/RFTransmitter.cpp | Update RMT init/write API usage |
| src/message_handlers/websocket/Local.cpp | Switch handler payload to std::span |
| src/message_handlers/websocket/Gateway.cpp | Switch handler payload to std::span |
| src/http/HTTPRequestManager.cpp | Add WiFi include; switch to std::span |
| src/captiveportal/Manager.cpp | Switch BIN send APIs to std::span |
| src/captiveportal/CaptivePortalInstance.cpp | Switch WS payload to std::span |
| src/WebSocketDeFragger.cpp | Switch callback payload to std::span |
| src/OtaUpdateManager.cpp | Update task WDT configuration API |
| src/GatewayConnectionManager.cpp | Switch BIN send APIs to std::span |
| src/GatewayClient.cpp | Switch BIN send APIs to std::span |
| platformio.ini | Use pioarduino platform + tool packages |
| include/wifi/WiFiManager.h | Remove IPv6 address API |
| include/visual/RgbLedDriver.h | Remove RMT handle member declaration |
| include/util/Base64Utils.h | Switch to std::span, include <span> |
| include/span.h | Remove bundled span implementation |
| include/serialization/CallbackFn.h | Switch serialization callback to std::span |
| include/serial/command_handlers/CommandEntry.h | Move types to OpenShock::SerialCmds |
| include/radio/RFTransmitter.h | Remove RMT handle member; update ok() |
| include/message_handlers/WebSocket.h | Switch handler payloads to std::span |
| include/http/HTTPRequestManager.h | Add <span> + Response constructor |
| include/captiveportal/Manager.h | Switch BIN send APIs to std::span |
| include/captiveportal/CaptivePortalInstance.h | Switch BIN send/WS payload to std::span |
| include/WebSocketDeFragger.h | Switch event callback to std::span |
| include/TinyVec.h | Include <span>; add span conversions/iterators |
| include/Hashing.h | Update mbedTLS hashing API calls |
| include/GatewayConnectionManager.h | Include <span>; switch BIN send API |
| include/GatewayClient.h | Include <span>; switch BIN send API |
| include/Common.h | Enforce C++20 requirement messaging |
| include/Checksum.h | Add C++20 concept-based overloads |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // if (set_esp_interface_dns(ESP_IF_WIFI_STA, IPAddress(1, 1, 1, 1), IPAddress(8, 8, 8, 8), IPAddress(9, 9, 9, 9)) != ESP_OK) { | ||
| // OS_LOGE(TAG, "Failed to set DNS servers"); | ||
| // return false; | ||
| // } |
There was a problem hiding this comment.
The DNS configuration block in Init() is now fully commented out, which makes runtime behavior unclear and leaves dead code in-place. If this needs to be disabled for Arduino 3.0, consider removing it entirely or gating it behind a feature flag / core-version check with an explanatory comment so it’s clear when/why custom DNS is skipped.
| uint8_t result = 0; | ||
|
|
||
| for (std::size_t i = 0; i < sizeof(T); ++i) { | ||
| result += static_cast<uint8_t>((value >> (i * 8)) & 0xFF); | ||
| } |
There was a problem hiding this comment.
In Sum8(std::integral T), shifting signed integral values is implementation-defined and can yield inconsistent byte extraction for negative inputs. Consider converting to std::make_unsigned_t (or bit_casting to bytes) before shifting/masking so the checksum is stable for all integral types.
| .idle_core_mask = (1 << CONFIG_FREERTOS_NUMBER_OF_CORES) - 1, // Bitmask of all cores | ||
| .trigger_panic = true, | ||
| }; | ||
| return esp_task_wdt_init(&twdt_config); |
There was a problem hiding this comment.
otaum_set_wdt_timeout() uses esp_task_wdt_init(&twdt_config) to change the timeout. On ESP-IDF v5, re-calling init when already initialized commonly returns ESP_ERR_INVALID_STATE; use esp_task_wdt_reconfigure() (or deinit+init) to actually apply the new timeout during OTA.
| return esp_task_wdt_init(&twdt_config); | |
| esp_err_t err = esp_task_wdt_reconfigure(&twdt_config); | |
| if (err == ESP_OK) { | |
| return ESP_OK; | |
| } | |
| if (err == ESP_ERR_INVALID_STATE) { | |
| return esp_task_wdt_init(&twdt_config); | |
| } | |
| return err; |
| OpenShock::SerialCmds::CommandGroup OpenShock::SerialCmds::CommandHandlers::VersionHandler() | ||
| { | ||
| auto group = OpenShock::Serial::CommandGroup("version"sv); | ||
| auto group = OpenShock::SerialCmds::CommandGroup("version"sv); |
There was a problem hiding this comment.
This handler is now implemented under OpenShock::SerialCmds, but the shared command handler index header (serial/command_handlers/index.h, pulled in via common.h) still declares OpenShock::Serial::CommandHandlers / OpenShock::Serial::CommandGroup. That mismatch will break compilation until index.h (and remaining handlers like ledtest.cpp) are updated to SerialCmds.
|
|
||
| // Register command handlers | ||
| s_commandGroups = OpenShock::Serial::CommandHandlers::AllCommandHandlers(); | ||
| s_commandGroups = OpenShock::SerialCmds::CommandHandlers::AllCommandHandlers(); |
There was a problem hiding this comment.
SerialInputHandler now references OpenShock::SerialCmds::CommandHandlers::AllCommandHandlers(), but the shared command handler index header (serial/command_handlers/index.h, included via common.h) still declares OpenShock::Serial::CommandHandlers and OpenShock::Serial::CommandGroup. This mismatch will prevent compilation unless the header (and any remaining Serial::CommandGroup usages in this file, e.g. printCommandHelp) are updated to SerialCmds as well.
| s_commandGroups = OpenShock::SerialCmds::CommandHandlers::AllCommandHandlers(); | |
| s_commandGroups = OpenShock::Serial::CommandHandlers::AllCommandHandlers(); |
| @@ -1,23 +1,16 @@ | |||
| #pragma once | |||
|
|
|||
| #include <bit> | |||
| #include <concepts> | |||
| #include <cstddef> | |||
| #include <cstdint> | |||
| #include <memory> | |||
There was a problem hiding this comment.
Checksum.h uses std::is_trivially_copyable_v (via the requires-clause/static_assert below) but doesn’t include <type_traits>. Add <type_traits> to this header to avoid build breaks from missing transitive includes.
| #include <memory> | |
| #include <memory> | |
| #include <type_traits> |
| gpio_num_t m_gpioPin; | ||
| uint8_t m_brightness; // 0-255 | ||
| std::vector<RGBState> m_pattern; | ||
| rmt_obj_t* m_rmtHandle; | ||
| TaskHandle_t m_taskHandle; | ||
| SimpleMutex m_taskMutex; |
There was a problem hiding this comment.
RgbLedDriver’s header removed the m_rmtHandle member, but the implementation (src/visual/RgbLedDriver.cpp) still uses m_rmtHandle for rmtInit/rmtWriteBlocking/rmtDeinit. Either update the implementation to the new Arduino 3.0 RMT API (similar to RFTransmitter) or restore an appropriate handle/member so this class compiles and functions correctly.
| @@ -6,10 +6,10 @@ | |||
|
|
|||
| #include <functional> | |||
| #include <map> | |||
| #include <span> | |||
| #include <string> | |||
| #include <string_view> | |||
There was a problem hiding this comment.
This header uses std::move in Response’s constructor, but is not included. Add here to avoid relying on transitive includes for std::move.
| #include <string_view> | |
| #include <string_view> | |
| #include <utility> |
| IPAddress ip = WiFi.localIP(); | ||
| snprintf(ipAddress, IPV4ADDR_FMT_LEN + 1, IPV4ADDR_FMT, IPV4ADDR_ARG(ip)); | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| bool WiFiManager::GetIPv6Address(char* ipAddress) | ||
| { | ||
| if (!IsConnected()) { | ||
| return false; | ||
| } | ||
|
|
||
| IPv6Address ip = WiFi.localIPv6(); | ||
| const uint8_t* ipPtr = ip; // Using the implicit conversion operator of IPv6Address | ||
| snprintf(ipAddress, IPV6ADDR_FMT_LEN + 1, IPV6ADDR_FMT, IPV6ADDR_ARG(ipPtr)); | ||
| snprintf(ipAddress, IPV6ADDR_FMT_LEN + 1, "%s", ip.toString()); | ||
|
|
There was a problem hiding this comment.
WiFiManager::GetIPAddress() now formats an IPv4 address using IPV6ADDR_FMT_LEN and passes IPAddress::toString() directly to snprintf. This is likely a compile error (toString() returns an Arduino String, not a const char*), and using the IPv6 length constant can lead to buffer-size contract mismatches. Use an IPv4-appropriate max length (or accept a buffer size parameter) and pass ip.toString().c_str() (or equivalent) to snprintf/strlcpy.
| // WiFi.setAutoConnect(false); | ||
| WiFi.setAutoReconnect(false); | ||
| WiFi.enableSTA(true); |
There was a problem hiding this comment.
There is newly commented-out behavior (WiFi.setAutoConnect(false)) left in Init(). If this call is intentionally removed due to Arduino 3.0 API changes, prefer removing the dead line or guarding it with a version/platform macro and a short explanation, rather than leaving it commented out.
No description provided.