docs: propose Olimex ESP32-C6 local UI variant - #173
Conversation
✅MegaLinter analysis: Success
See detailed reports in MegaLinter artifacts Your project could benefit from a custom flavor, which would allow you to run only the linters you need, and thus improve runtime performances. (Skip this info by defining
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84fd0b382f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| board = esp32-c6-devkitc-1 ; or a custom Olimex board definition if needed | ||
| framework = arduino | ||
| build_flags = | ||
| ${common.build_flags} |
There was a problem hiding this comment.
When this block is added to the current platformio.ini, ${common.build_flags} references an option that does not exist—[common] defines only serial_speed—and the environment also omits the dependencies explicitly shared through ${common_env_data.lib_deps}. Consequently, the proposed environment cannot build the existing application as written; define a common set of flags and dependencies or explicitly inherit/import an existing base environment. Agents.mdL103-L109
Useful? React with 👍 / 👎.
| Recommended local menu scope: | ||
|
|
||
| 1. Status overview: water temperature, pump state, mode, Wi-Fi/MQTT state. | ||
| 2. Operation mode: auto/manual/off as already supported by the controller. |
There was a problem hiding this comment.
If the local menu passes these values to the existing controller, off is rejected: OperationModeNode::setMode() accepts only auto, manu, boost, and timer, which are also the four modes cycled by the current NORVI UI. Thus auto/manual/off is not already supported as claimed and leaves the intended behavior of off undefined; list the real modes or document an explicit mapping such as switching to manual mode and separately disabling the pump.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d803257257
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| `ConfigManager` remains the source of truth for persisted settings. The local UI | ||
| is only another frontend for reading and changing selected settings. |
There was a problem hiding this comment.
When LocalSettingsMenu is implemented against ConfigManager as specified, operation-mode and temperature edits will not reliably affect the controller: PoolControllerContext::setup() initializes values from ConfigManager but then calls operationModeNode.loadState(), while OperationModeNode persists those same values separately through StateManager. Existing web/MQTT handlers explicitly update both stores and the live node. Define a single settings action/service that updates OperationModeNode and synchronizes persistence, or consolidate the two stores before treating ConfigManager as the source of truth.
Useful? React with 👍 / 👎.
| ├────────────────────────────┤ | ||
| │ SOLAR OK │ | ||
| │ 35.1 °C │ | ||
| │ Ventil: SOLAR │ |
There was a problem hiding this comment.
For the existing controller targeted by this design, there is no valve state to populate this row: PoolController.cpp creates poolPumpNode and solarPumpNode, and the web status exports solar_pump. An exhaustive repository search found no valve node or controller flow, so implementing this layout would fabricate or hard-code the advertised status. Change the row to the solar-pump state, or explicitly add a valve actuator and its control semantics to the design.
Useful? React with 👍 / 👎.
stritti
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a96dc1d1ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| Main risks: | ||
|
|
||
| - ESP32-C6 is a single-core RISC-V target, unlike the original ESP32 target. |
There was a problem hiding this comment.
When this variant is built for ESP32-C6, the current ESP32TemperatureNode still unconditionally declares and calls the original-ESP32 ROM symbol temprature_sens_read(), which is not provided for the C6 target, so the new environment cannot link even after its PlatformIO inheritance is corrected. Include a target-specific temperature implementation or disable this node for C6 as part of the board adapter. Agents.mdL26-L30
Useful? React with 👍 / 👎.
| framework = arduino | ||
| build_flags = | ||
| ${common.build_flags} | ||
| -D OLIMEX_ESP32_C6_EVB |
There was a problem hiding this comment.
With the environment as specified, a repo-wide search finds no consumer of OLIMEX_ESP32_C6_EVB, so Config.hpp falls through to the generic ESP32 mapping and assigns the two DS18B20 nodes to GPIO32 and GPIO33; ESP32-C6 only has GPIO0–30. Numeric pin constants can still compile, leaving the deployed controller without working pool or solar sensors, so the design should require an Olimex board adapter containing the verified sensor, relay, and UI pin map. Agents.mdL26-L30
Useful? React with 👍 / 👎.
|
|
||
| ```ini | ||
| [env:olimex_esp32_c6_evb] | ||
| platform = espressif32 |
There was a problem hiding this comment.
Using the unversioned platform = espressif32 makes this environment resolve whichever toolchain release is current, while both existing firmware environments are pinned to espressif32 @ 7.0.1. Because C6 framework compatibility is itself called out as a risk, builds validated against one Arduino/ESP-IDF version can later change or fail without a repository change; pin the tested release or centralize the platform version with the other environments. Agents.mdL172-L175
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1986052718
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| 1. Status overview: water temperature, pump state, mode, Wi-Fi/MQTT state. | ||
| 2. Operation mode: auto/manual/off as already supported by the controller. | ||
| 3. Pump/manual actions where safe. |
There was a problem hiding this comment.
When the local menu is opened while the controller is in auto, boost, or timer, this scope does not require the pump action to be rejected or disabled; a confirmation screen does not enforce that invariant. Both existing command paths explicitly permit pump changes only in manu (WebPortal::apiTogglePump() at src/WebPortal.cpp:862-866 and the MQTT handler at src/MqttPublisher.cpp:1124-1129). A local implementation that toggles the relay directly can therefore conflict with the active rule and produce a transient or immediately overwritten pump state, so the design should explicitly require manual mode or an atomic transition to it before exposing these actions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9dfb0b484
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| constexpr std::uint8_t PIN_ENCODER_CLK{12}; | ||
| constexpr std::uint8_t PIN_ENCODER_DT{13}; |
There was a problem hiding this comment.
The fresh implementation plan now assigns the encoder to GPIO12 and GPIO13, which are the ESP32-C6's fixed USB D-/D+ signals and are connected to the Olimex board's programming/debug USB interface. Connecting a KY-040 there can break USB flashing/logging, while USB traffic can appear as encoder transitions; select two genuinely free exposed GPIOs instead.
Useful? React with 👍 / 👎.
| case LocalMenuItem::MODE: | ||
| case LocalMenuItem::PUMP: | ||
| currentPage_ = LocalUiPage::OVERVIEW; | ||
| break; |
There was a problem hiding this comment.
The new implementation plan now makes both actionable menu entries simply return to the overview, and no later task adds an edit, confirmation, controller update, or persistence path. Following the plan therefore produces a “local settings” interface in which selecting Mode or Pump does nothing; these cases need explicit state/action flows wired to the live controller before the plan is complete.
Useful? React with 👍 / 👎.
| tft.print("--.- C"); | ||
| tft.setTextSize(2); | ||
| tft.setCursor(8, 92); | ||
| tft.print("Pumpe: --"); |
There was a problem hiding this comment.
The fresh implementation code confirms that the pool temperature and pump state are permanent string literals, and no subsequent task replaces them with poolTemperatureNode or poolPumpNode data. Thus even after all seven tasks are completed, the principal status page always displays --.- C and Pumpe: -- rather than the actual equipment state.
Useful? React with 👍 / 👎.
| if (!swPressed && buttonWasPressed_) { | ||
| buttonWasPressed_ = false; | ||
| const std::uint32_t heldMs = nowMs - buttonPressStartMs_; | ||
| return heldMs >= LONG_PRESS_MS ? LocalUiEvent::LONG_PRESS : LocalUiEvent::SHORT_PRESS; |
There was a problem hiding this comment.
On a physical KY-040, contact bounce can briefly alternate swPressed during press or release; this code treats the first low-to-high sample as an immediate press event and a subsequent bounce as another press, so one click can select multiple menu items or be misclassified as a short press. Require the button state to remain stable for a debounce interval and add bounce sequences to the decoder tests.
Useful? React with 👍 / 👎.
| #if defined(OLIMEX_ESP32_C6_EVB) && defined(HAS_LOCAL_TFT_UI) | ||
| #include "OlimexLocalUi.hpp" | ||
| #endif |
There was a problem hiding this comment.
This step explicitly adds Olimex #if branches to PoolController.cpp, contradicting the repository rule that board-specific adapters and #ifdef directives belong under src/platform/. Expose an unconditional local-UI interface to the application and select the Olimex/no-op implementation in the platform layer instead, otherwise every new board continues expanding controller-level branching. Agents.mdL26-L30
Useful? React with 👍 / 👎.
| bodmer/TFT_eSPI @ ^2.5.43 | ||
| ricmoo/QRCode @ ^0.0.1 |
There was a problem hiding this comment.
Both newly introduced libraries use caret ranges, allowing future compatible releases to alter the firmware build without a repository change. Pin the exact TFT_eSPI and QRCode versions validated by the three-environment build, as required by the repository dependency policy. Agents.mdL172-L175
Useful? React with 👍 / 👎.
1 participant
Summary
Hardware direction
Compatibility and power verification
Verification
Note: ctest --test-dir test/native/build --output-on-failure reported no registered tests in the existing build directory, so the native test runner was executed directly.