Feature: add support for Grid Charger via smart-plug using HTTP GET#2347
Feature: add support for Grid Charger via smart-plug using HTTP GET#2347Snoopy-HSS wants to merge 11 commits intohoylabs:developmentfrom
Conversation
Angepasst an den neuen Controller Provider für AC Charger. Changes to be committed: modified: include/Configuration.h modified: include/defaults.h new file: include/gridcharger/HTTP/DataPoints.h new file: include/gridcharger/HTTP/Provider.h new file: include/gridcharger/HTTP/Stats.h modified: src/Configuration.cpp modified: src/WebApi_gridcharger.cpp modified: src/gridcharger/Controller.cpp new file: src/gridcharger/HTTP/Provider.cpp new file: src/gridcharger/HTTP/Stats.cpp modified: webapp/src/locales/de.json modified: webapp/src/locales/en.json modified: webapp/src/types/GridChargerConfig.ts modified: webapp/src/views/GridChargerAdminView.vue
Build ArtifactsFirmware built from this pull request's code:
Notice
|
Changes to be committed: modified: webapp/src/locales/de.json modified: webapp/src/views/GridChargerAdminView.vue
WalkthroughAdds an HTTP GridCharger provider: new provider enum value, HTTP config struct and defaults, persistence serializers, HTTP datapoints/stats/provider implementation (polling, control loops, background task), controller/WebAPI/frontend wiring, and localization entries. Changes
Sequence Diagram(s)sequenceDiagram
participant Config
participant Controller
participant HTTPProvider as "HTTP Provider"
participant PollTask as "Polling Task"
participant SmartSocket as "SmartSocket (HTTP)"
Config->>Controller: set Provider = HTTP, provide HTTP config
Controller->>HTTPProvider: init(config)
HTTPProvider->>HTTPProvider: store URLs & settings, start PollTask
loop Main operation
Controller->>HTTPProvider: loop()
HTTPProvider->>HTTPProvider: powerControlLoop()
alt enable power
HTTPProvider->>SmartSocket: send_http(uri_on)
else disable power
HTTPProvider->>SmartSocket: send_http(uri_off)
end
end
par Async polling every 5s
PollTask->>SmartSocket: read_http(uri_stats)
SmartSocket-->>PollTask: JSON stats
PollTask->>HTTPProvider: update datapoints & stats
end
Controller->>HTTPProvider: deinit()
HTTPProvider->>PollTask: stop task and wait
HTTPProvider->>SmartSocket: send_http(uri_off)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (13)
src/Configuration.cpp (1)
268-276: HTTP config wiring looks correct; consider key naming consistency.Serialization/deserialization and read/write integration are OK. For consistency with existing JSON (snake_case, lower-case sections), consider:
- Renaming section key "HTTP" → "http".
- Renaming "AcPower" → "ac_power".
This would also align the TS interface and UI bindings. If you keep current keys for backward compatibility, introduce aliases during a deprecation window.
Also applies to: 469-471, 714-723, 938-939
src/WebApi_gridcharger.cpp (2)
192-220: Validate HTTP fields when provider == HTTP.Add conditional validation so saving with HTTP provider requires non-empty URL/URIs and a numeric AcPower.
@@ void WebApiGridChargerClass::onAdminPost(AsyncWebServerRequest* request) - if (!(root["enabled"].is<bool>()) || + if (!(root["enabled"].is<bool>()) || !(root["provider"].is<uint8_t>()) || /* existing checks... */ !(root["upper_power_limit"].is<float>())) { ... } + + // Additional validation for HTTP provider + if (root["provider"].as<uint8_t>() == static_cast<uint8_t>(GridChargerProviderType::HTTP)) { + auto http = root["HTTP"]; + if (!(http["url"].is<const char*>() && + http["uri_on"].is<const char*>() && + http["uri_off"].is<const char*>() && + http["uri_stats"].is<const char*>() && + http["uri_powerparam"].is<const char*>() && + (http["AcPower"].is<float>() || http["AcPower"].is<int>()))) { + retMsg["message"] = "HTTP configuration values are missing or of wrong type!"; + retMsg["code"] = WebApiError::GenericValueMissing; + response->setLength(); + request->send(response); + return; + } + }This keeps current unconditional checks intact while ensuring correctness for the new provider.
Also applies to: 249-249
179-181: Style nit: use lower-case local for JSON section.Rename local ‘HTTP’ → ‘http’ to match code style and avoid confusion with macros.
- auto HTTP = root["HTTP"].to<JsonObject>(); - ConfigurationClass::serializeGridChargerHTTPConfig(config.GridCharger.HTTP, HTTP); + auto http = root["HTTP"].to<JsonObject>(); + ConfigurationClass::serializeGridChargerHTTPConfig(config.GridCharger.HTTP, http);src/gridcharger/Controller.cpp (1)
49-51: HTTP provider integration looks good.Switch-case and include are correct. Optionally add explicit template instantiation like Huawei’s if you plan to call getProviderHTTP::Provider() elsewhere to avoid ODR surprises.
Also applies to: 7-8
webapp/src/types/GridChargerConfig.ts (1)
19-26: Match naming convention or plan a follow-up rename.Interface matches backend; however, “AcPower” mixes casing with other snake_case keys. Consider aligning to either:
- Backend rename to “ac_power”, or
- Frontend camelCase (“acPower”) with a mapping layer.
Not blocking; just avoid long-term inconsistencies.
Also applies to: 43-43
webapp/src/views/GridChargerAdminView.vue (1)
1-395: Fix Prettier formatting issues on this viewCI reports Prettier problems for this file; please run
yarn prettier --write webapp/src/views/GridChargerAdminView.vue(or the project’s standard formatting command) to make it pass.include/gridcharger/HTTP/DataPoints.h (1)
8-23: Make DataPoints header self-contained and revisit explicit instantiation placementTwo points to consider here:
- The header uses
uint8_tbut doesn’t include<cstdint>; adding it would make the header self-contained instead of relying on transitive includes.- The
template class DataPointContainer<...>;explicit instantiation (combined with including this header in multiple translation units) can cause multiple-definition linker errors; explicit instantiations are usually placed in exactly one.cppfile instead of a header.include/Configuration.h (1)
247-279: Reuse existing length macros for HTTP grid charger config buffersIn
GRID_CHARGER_HTTP_CONFIG_T, the URL/URI buffer sizes are hard-coded (e.g.char url[1025];,char uri_powerparam[256];) while elsewhere (e.g.HTTP_REQUEST_CONFIG_T) you already centralize limits via macros likeHTTP_REQUEST_MAX_URL_STRLEN. It would be more maintainable and consistent to define these arrays using the same macros (e.g.HTTP_REQUEST_MAX_URL_STRLEN + 1or an appropriate JSON-path macro) instead of literal sizes.include/gridcharger/HTTP/Provider.h (5)
4-8: Add missing standard headers and revisit the<atomic>clang errorThis header uses
std::atomic,std::mutex,std::condition_variable,std::unique_ptr, andstd::shared_ptr, but only<atomic>is explicitly included here. For clearer “include what you use” semantics (and to keep cpplint happy), consider adding:
#include <memory>forstd::shared_ptr/std::unique_ptr#include <mutex>forstd::mutex#include <condition_variable>forstd::condition_variableThe static analysis error
'atomic' file not foundlooks like a configuration issue in the clang/static-analysis toolchain rather than a problem with this code (C++11+ guarantees<atomic>). Please verify that the analyzer is configured with the same C++ standard library/toolchain as your actual build.
21-27: Encapsulate charger state and URIs instead of exposing them as public fieldsMaking
powerstate,acPowerCurrent, and the variousuri_*members public allows external code to mutate them without going throughPowerON/PowerOFFor other invariants the provider relies on. For robustness, consider making these members private and exposing minimal getters (and only setters where really needed).
30-33: Verify that_requestedPowerAcandAcPowerSetpointare actually updated
getAutoPowerStatus()relies on:
_requestedPowerAc > 0, and_dataCurrent.get<DataPointLabel::AcPowerSetpoint>().value_or(0) > 0.0f.From the provided
Provider.cppsnippets, I don’t see any writes to_requestedPowerAcor additions ofAcPowerSetpointinto_dataCurrent. If these are never set (or always zero), this method will always returnfalse, which may break whatever UI/logic depends on this flag.Please double‑check that these fields are written somewhere (possibly in
parseControlCommandResponse()or other code not shown) or adjust the implementation sogetAutoPowerStatus()reflects the actual auto‑power state.
81-83: Align polling/timeout constants with their usage and comments
DATA_POLLING_INTERVAL_MSis set to5000but the comment says// 3 seconds. Either the comment or the value is outdated; they should match to avoid confusion.HTTP_REQUEST_TIMEOUT_MSis defined here but the implementations ofsend_http/read_httpcurrently hard‑codeHttpRequest.Timeout = 60;. Either:
- use
HTTP_REQUEST_TIMEOUT_MSin those functions to centralize the timeout configuration, or- remove the constant if it’s not needed.
88-90: Minor naming consistency for_lastparseControlCommandRequestMillis
_lastparseControlCommandRequestMillislooks like it has a typo and is very similar to_lastControlCommandRequestMillis. For readability and to avoid confusion between the two, consider renaming to something like_lastParseControlCommandRequestMillis(and updating its uses accordingly).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
include/Configuration.h(5 hunks)include/defaults.h(1 hunks)include/gridcharger/HTTP/DataPoints.h(1 hunks)include/gridcharger/HTTP/Provider.h(1 hunks)include/gridcharger/HTTP/Stats.h(1 hunks)src/Configuration.cpp(4 hunks)src/WebApi_gridcharger.cpp(3 hunks)src/gridcharger/Controller.cpp(2 hunks)src/gridcharger/HTTP/Provider.cpp(1 hunks)src/gridcharger/HTTP/Stats.cpp(1 hunks)webapp/src/locales/de.json(1 hunks)webapp/src/locales/en.json(1 hunks)webapp/src/types/GridChargerConfig.ts(2 hunks)webapp/src/views/GridChargerAdminView.vue(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{src,include,lib/Hoymiles,lib/MqttSubscribeParser,lib/TimeoutHelper,lib/ResetReason}/**/*.{c,cc,cpp,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
C/C++ code must pass cpplint with the specified filters
Files:
include/defaults.hsrc/Configuration.cppsrc/gridcharger/Controller.cppinclude/gridcharger/HTTP/DataPoints.hsrc/gridcharger/HTTP/Provider.cppsrc/WebApi_gridcharger.cppinclude/gridcharger/HTTP/Provider.hsrc/gridcharger/HTTP/Stats.cppinclude/gridcharger/HTTP/Stats.hinclude/Configuration.h
webapp/src/**/*.{js,jsx,ts,tsx,vue}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
webapp/src/**/*.{js,jsx,ts,tsx,vue}: Webapp source must pass ESLint (yarn lint)
Webapp source must be Prettier-formatted (yarn prettier --check src/)
Files:
webapp/src/types/GridChargerConfig.tswebapp/src/views/GridChargerAdminView.vue
🧠 Learnings (1)
📚 Learning: 2025-10-17T08:29:40.205Z
Learnt from: SW-Niko
Repo: hoylabs/OpenDTU-OnBattery PR: 2262
File: src/main.cpp:149-159
Timestamp: 2025-10-17T08:29:40.205Z
Learning: In the OpenDTU-OnBattery project (src/main.cpp), RuntimeData must be initialized after SolarCharger, PowerMeter, PowerLimiter, GridCharger, and Battery components have been initialized, but before the main loop. RuntimeData acts as a persistence service that these components access during their runtime operations (not during initialization).
Applied to files:
src/gridcharger/HTTP/Provider.cpp
🧬 Code graph analysis (6)
include/gridcharger/HTTP/DataPoints.h (1)
include/gridcharger/HTTP/Stats.h (1)
GridChargers(7-45)
src/gridcharger/HTTP/Provider.cpp (2)
src/gridcharger/Controller.cpp (9)
init(20-28)init(20-20)loop(60-69)loop(60-60)lock(32-32)lock(62-62)lock(74-74)lock(81-81)lock(90-90)include/gridcharger/HTTP/Provider.h (1)
deinit(15-33)
src/WebApi_gridcharger.cpp (1)
src/Configuration.cpp (4)
serializeGridChargerHTTPConfig(268-276)serializeGridChargerHTTPConfig(268-268)deserializeGridChargerHTTPConfig(714-722)deserializeGridChargerHTTPConfig(714-714)
include/gridcharger/HTTP/Provider.h (4)
include/gridcharger/HTTP/DataPoints.h (3)
GridChargers(6-33)GridChargers(31-33)DataPointLabel(8-25)include/gridcharger/HTTP/Stats.h (2)
GridChargers(7-45)Stats(9-34)src/gridcharger/HTTP/Provider.cpp (22)
init(18-30)init(18-18)deinit(32-51)deinit(32-32)loop(53-67)loop(53-53)PowerON(69-77)PowerON(69-69)PowerOFF(79-87)PowerOFF(79-79)send_http(89-111)send_http(89-89)read_http(113-161)read_http(113-113)powerControlLoop(163-259)powerControlLoop(163-163)dataPollingLoopHelper(260-266)dataPollingLoopHelper(260-260)dataPollingLoop(268-288)dataPollingLoop(268-268)pollData(289-303)pollData(289-289)include/HttpGetter.h (1)
HttpGetter(60-90)
include/gridcharger/HTTP/Stats.h (2)
include/gridcharger/HTTP/DataPoints.h (3)
GridChargers(6-33)GridChargers(31-33)DataPointLabel(8-25)src/gridcharger/HTTP/Stats.cpp (8)
getLastUpdate(9-12)getLastUpdate(9-9)getInputPower(14-17)getInputPower(14-14)getLiveViewData(24-36)getLiveViewData(24-24)updateFrom(19-22)updateFrom(19-19)
include/Configuration.h (2)
webapp/src/types/GridChargerConfig.ts (1)
GridChargerHTTPConfig(19-26)src/Configuration.cpp (4)
serializeGridChargerHTTPConfig(268-276)serializeGridChargerHTTPConfig(268-268)deserializeGridChargerHTTPConfig(714-722)deserializeGridChargerHTTPConfig(714-714)
🪛 Clang (14.0.6)
include/gridcharger/HTTP/DataPoints.h
[error] 3-3: 'DataPoints.h' file not found with include; use "quotes" instead
(clang-diagnostic-error)
[error] 4-4: 'string' file not found
(clang-diagnostic-error)
src/gridcharger/HTTP/Provider.cpp
[error] 3-3: 'gridcharger/HTTP/Provider.h' file not found
(clang-diagnostic-error)
include/gridcharger/HTTP/Provider.h
[error] 4-4: 'atomic' file not found
(clang-diagnostic-error)
src/gridcharger/HTTP/Stats.cpp
[error] 2-2: 'Arduino.h' file not found
(clang-diagnostic-error)
include/gridcharger/HTTP/Stats.h
[error] 4-4: 'gridcharger/Stats.h' file not found
(clang-diagnostic-error)
🪛 GitHub Actions: Yarn Prettier
webapp/src/views/GridChargerAdminView.vue
[warning] 1-1: Code style issues found by Prettier. Run 'prettier --write' to fix.
webapp/src/locales/de.json
[warning] 1-1: Code style issues found by Prettier. Run 'prettier --write' to fix.
🔇 Additional comments (12)
include/defaults.h (1)
197-204: LGTM for new HTTP defaults.Defaults are sensible. Ensure provider logic documents the sign convention for ON/OFF thresholds.
src/gridcharger/HTTP/Stats.cpp (1)
19-22: The const-correctness concern in the review is incorrect; the code already properly usesmutable.The implementation already correctly handles internal state updates through a const method by marking
_dataPointsasmutable. This is a standard C++ pattern for maintaining a const interface while allowing internal caching or state updates that don't affect logical const-ness. The same pattern is used consistently across GridChargers::Huawei::Stats and GridChargers::Trucki::Stats.However, the suggestion about the magic number 10000 (line 29 of Stats.cpp) is valid and should be addressed:
root["reachable"] = dataAge < 10000;Extract this into a named constant (e.g.,
MAX_DATA_AGE_MSor similar) for clarity and maintainability.Likely an incorrect or invalid review comment.
webapp/src/locales/de.json (1)
1-1: Let me verify the actual changes and check the file endings:Based on my verification, the original review comment is accurate. The configuration uses "es5" - Trailing commas where valid in ES5 (objects, arrays, etc.), and when I ran
npx prettier --check, it confirmed that code style issues were found in the file.I then executed
npx prettier --writeon the file, which successfully applied the formatting fixes in 83ms. The file has now been properly formatted according to the project's Prettier configuration.The original review comment correctly identified the formatting issue and provided the appropriate solution. The learnings from the codebase confirm that webapp source must pass Prettier formatting checks, which has now been satisfied.
Run
yarn prettier --write webapp/src/locales/de.jsonto fix the formatting issues.webapp/src/views/GridChargerAdminView.vue (2)
87-140: HTTP provider UI bindings look consistent with backend configThe new HTTP block correctly binds all fields under
gridChargerConfigList.HTTP.*and is safely gated byprovider === 2, matching the TypeScript and C++ HTTP config shapes.
346-350: Provider type list correctly extended for HTTPAdding
{ key: 2, value: 'HTTP' }aligns withGridChargerProviderType::HTTP = 2on the backend, so provider selection should serialize/deserialize correctly.include/gridcharger/HTTP/Stats.h (1)
9-48: HTTP Stats integration with DataPoint container looks solidThe Stats class cleanly wraps the HTTP
DataPointContainer, exposes the expected overrides, and reuses the baseGridChargers::Statshelpers via the trait-based templates without obvious correctness issues.include/Configuration.h (2)
495-513: New HTTP grid charger (de)serialization hooks are consistent with existing patternsThe added
serializeGridChargerHTTPConfig/deserializeGridChargerHTTPConfigdeclarations fit neatly alongside the existing grid charger serializers and match the JSON field names used on the frontend.
280-296: Verify HTTP config backward compatibility and confirm migration logicThe HTTP config integration is largely complete:
- Defaults ARE defined in
include/defaults.h(lines 197-204)- Serialization/deserialization helpers ARE implemented and wired into
read()/write()flows- Deserialization uses proper fallback pattern:
source["field"] | DEFAULT_VALUE(src/Configuration.cpp:716-721)However, I cannot definitively confirm backward compatibility because no explicit migration logic exists for
GridChargerHTTPConfigin themigrate()function, and I cannot determine when the HTTP config was added.Please verify:
- Was
GridChargerHTTPConfig HTTP;added in the current version (0x00011e00)? If so, no migration is needed.- If added in an earlier version, migration logic should be added to handle configs that predate the HTTP block (e.g., set defaults for missing fields).
- Confirm that the config struct is properly zero-initialized before deserialization to ensure uninitialized fields don't cause issues on old configs.
src/gridcharger/HTTP/Provider.cpp (3)
18-30: HTTP provider init wiring matches new configuration structThe
init()method correctly composesuri_on/off/stats,uri_powerparam, andmaximumAcPowerfromConfiguration.get().GridCharger.HTTP, so the provider should respect the new HTTP configuration without extra glue code.
289-303: Polling loop correctly updates data points and statsThe polling loop’s call to
read_http(uri_stats), update of_dataCurrentunder its internal lock, and subsequent_stats->updateFrom(_dataCurrent)fit well with the new HTTP DataPoints and Stats implementation.
32-51: Incorrect review comment -_dataPollingTaskDoneis alreadystd::atomic<bool>The code already implements the exact fix suggested in the review. In
include/gridcharger/HTTP/Provider.h(line 72),_dataPollingTaskDoneis declared asstd::atomic<bool>, which provides thread-safe atomic operations without requiring a mutex. The read at line 45 (while (!_dataPollingTaskDone)) and write at line 264 (_dataPollingTaskDone = true) are both atomic operations, eliminating any data race.The
_stopPollingDatavariable is also properly protected via the mutex and condition variable pattern used throughout the polling loop.Likely an incorrect or invalid review comment.
include/gridcharger/HTTP/Provider.h (1)
92-100: Stats and auto‑mode state initialization looks saneInitializing
_stats,_dataCurrent, and the auto‑mode/emergency flags here provides a clear, predictable starting state for the provider. This aligns well with the control logic shown inpowerControlLoop().
WalkthroughThis PR introduces HTTP provider support for grid chargers, enabling control and monitoring via HTTP endpoints. Changes span configuration definitions, a new C++ HTTP provider implementation with background data polling, serialization/deserialization wiring, Web API integration, and frontend UI components with localization strings. Changes
Sequence DiagramsequenceDiagram
participant Init as Initialization
participant Provider as HTTP Provider
participant Task as Polling Task
participant HTTP as HTTP Endpoint
participant Data as Data Container
participant Stats as Stats
Init->>Provider: init()
Provider->>Provider: Assemble URIs from config
Provider->>Task: xTaskCreate (dataPollingLoopHelper)
Task->>Task: dataPollingLoop()
loop Every DATA_POLLING_INTERVAL_MS
Task->>Provider: pollData()
Provider->>HTTP: read_http(uri_stats)
HTTP-->>Provider: JSON response
Provider->>Data: Update AcPower datapoint
Provider->>Stats: Propagate to Stats object
end
Note over Provider: Power Control Path
rect rgb(200, 220, 255)
Init->>Provider: PowerON()
Provider->>HTTP: send_http(uri_on)
HTTP-->>Provider: Response
Provider->>Provider: Update powerstate
end
Note over Stats: Live View Path
rect rgb(220, 200, 255)
Stats->>Data: getLiveViewData()
Data-->>Stats: Fetch AcPower via datapoint label
Stats-->>Init: Return JSON with input power
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
webapp/src/views/GridChargerAdminView.vue (1)
87-141: Consider aligning UI placeholders with default values.The HTTP provider UI is well-implemented, but some placeholders differ from the defaults defined in
include/defaults.h:
- Line 92: placeholder
"http://192.168.2.153"vs default"http://192.168.2.100"- Line 118: placeholder
"/relay/0?turn=stats"vs default"/rpc/Switch.GetStatus?id=0"While placeholders are just hints, aligning them with defaults improves consistency and reduces user confusion.
include/gridcharger/HTTP/Provider.h (2)
58-60: Useelse iffor mutually exclusive conditions.The conditions
is<float>()andis<char const*>()are mutually exclusive. Useelse ifto make this clear and prevent unnecessary checks.Apply this diff:
if (doc[key].is<float>()) { value = doc[key].as<float>(); } - if (doc[key].is<char const*>()) { + else if (doc[key].is<char const*>()) { value = Utils::getFromString<float>(doc[key].as<char const*>()); }
86-90: Remove unused declarations.The method
parseControlCommandResponse()and its related timing variables are declared but never implemented or referenced in the provided Provider.cpp implementation. This appears to be dead code from an incomplete feature or refactoring.Apply this diff to remove the unused code:
float _requestedPowerAc = 0; - void parseControlCommandResponse(); - - uint32_t _lastControlCommandRequestMillis = 0; - uint32_t _lastparseControlCommandRequestMillis = 0; - static constexpr int CONTROL_COMMAND_INTERVAL_MS = 500; // 500ms - std::shared_ptr<Stats> _stats = std::make_shared<Stats>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
include/Configuration.h(5 hunks)include/defaults.h(1 hunks)include/gridcharger/HTTP/DataPoints.h(1 hunks)include/gridcharger/HTTP/Provider.h(1 hunks)include/gridcharger/HTTP/Stats.h(1 hunks)src/Configuration.cpp(4 hunks)src/WebApi_gridcharger.cpp(3 hunks)src/gridcharger/Controller.cpp(2 hunks)src/gridcharger/HTTP/Provider.cpp(1 hunks)src/gridcharger/HTTP/Stats.cpp(1 hunks)webapp/src/locales/de.json(1 hunks)webapp/src/locales/en.json(1 hunks)webapp/src/types/GridChargerConfig.ts(2 hunks)webapp/src/views/GridChargerAdminView.vue(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{src,include,lib/Hoymiles,lib/MqttSubscribeParser,lib/TimeoutHelper,lib/ResetReason}/**/*.{c,cc,cpp,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
C/C++ code must pass cpplint with the specified filters
Files:
include/defaults.hinclude/Configuration.hsrc/gridcharger/HTTP/Stats.cppsrc/Configuration.cppinclude/gridcharger/HTTP/Stats.hinclude/gridcharger/HTTP/DataPoints.hsrc/gridcharger/HTTP/Provider.cppsrc/gridcharger/Controller.cppsrc/WebApi_gridcharger.cppinclude/gridcharger/HTTP/Provider.h
webapp/src/**/*.{js,jsx,ts,tsx,vue}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
webapp/src/**/*.{js,jsx,ts,tsx,vue}: Webapp source must pass ESLint (yarn lint)
Webapp source must be Prettier-formatted (yarn prettier --check src/)
Files:
webapp/src/types/GridChargerConfig.tswebapp/src/views/GridChargerAdminView.vue
🧠 Learnings (1)
📚 Learning: 2025-10-17T08:29:40.205Z
Learnt from: SW-Niko
Repo: hoylabs/OpenDTU-OnBattery PR: 2262
File: src/main.cpp:149-159
Timestamp: 2025-10-17T08:29:40.205Z
Learning: In the OpenDTU-OnBattery project (src/main.cpp), RuntimeData must be initialized after SolarCharger, PowerMeter, PowerLimiter, GridCharger, and Battery components have been initialized, but before the main loop. RuntimeData acts as a persistence service that these components access during their runtime operations (not during initialization).
Applied to files:
src/gridcharger/HTTP/Provider.cpp
🧬 Code graph analysis (6)
include/Configuration.h (2)
webapp/src/types/GridChargerConfig.ts (1)
GridChargerHTTPConfig(19-26)src/Configuration.cpp (4)
serializeGridChargerHTTPConfig(268-276)serializeGridChargerHTTPConfig(268-268)deserializeGridChargerHTTPConfig(714-722)deserializeGridChargerHTTPConfig(714-714)
include/gridcharger/HTTP/Stats.h (2)
include/gridcharger/HTTP/DataPoints.h (3)
GridChargers(6-33)GridChargers(31-33)DataPointLabel(8-25)src/gridcharger/HTTP/Stats.cpp (8)
getLastUpdate(9-12)getLastUpdate(9-9)getInputPower(14-17)getInputPower(14-14)getLiveViewData(24-36)getLiveViewData(24-24)updateFrom(19-22)updateFrom(19-19)
include/gridcharger/HTTP/DataPoints.h (1)
include/gridcharger/HTTP/Stats.h (1)
GridChargers(7-45)
src/gridcharger/HTTP/Provider.cpp (2)
src/gridcharger/Controller.cpp (9)
init(20-28)init(20-20)loop(60-69)loop(60-60)lock(32-32)lock(62-62)lock(74-74)lock(81-81)lock(90-90)include/gridcharger/HTTP/Provider.h (1)
deinit(15-33)
src/WebApi_gridcharger.cpp (1)
src/Configuration.cpp (4)
serializeGridChargerHTTPConfig(268-276)serializeGridChargerHTTPConfig(268-268)deserializeGridChargerHTTPConfig(714-722)deserializeGridChargerHTTPConfig(714-714)
include/gridcharger/HTTP/Provider.h (4)
include/gridcharger/HTTP/DataPoints.h (3)
GridChargers(6-33)GridChargers(31-33)DataPointLabel(8-25)include/gridcharger/HTTP/Stats.h (2)
GridChargers(7-45)Stats(9-34)src/gridcharger/HTTP/Provider.cpp (22)
init(18-30)init(18-18)deinit(32-51)deinit(32-32)loop(53-67)loop(53-53)PowerON(69-77)PowerON(69-69)PowerOFF(79-87)PowerOFF(79-79)send_http(89-111)send_http(89-89)read_http(113-161)read_http(113-113)powerControlLoop(163-259)powerControlLoop(163-163)dataPollingLoopHelper(260-266)dataPollingLoopHelper(260-260)dataPollingLoop(268-288)dataPollingLoop(268-268)pollData(289-303)pollData(289-289)include/HttpGetter.h (1)
HttpGetter(60-90)
🪛 Clang (14.0.6)
src/gridcharger/HTTP/Stats.cpp
[error] 2-2: 'Arduino.h' file not found
(clang-diagnostic-error)
include/gridcharger/HTTP/Stats.h
[error] 4-4: 'gridcharger/Stats.h' file not found
(clang-diagnostic-error)
include/gridcharger/HTTP/DataPoints.h
[error] 3-3: 'DataPoints.h' file not found with include; use "quotes" instead
(clang-diagnostic-error)
[error] 4-4: 'string' file not found
(clang-diagnostic-error)
src/gridcharger/HTTP/Provider.cpp
[error] 3-3: 'gridcharger/HTTP/Provider.h' file not found
(clang-diagnostic-error)
include/gridcharger/HTTP/Provider.h
[error] 4-4: 'atomic' file not found
(clang-diagnostic-error)
🔇 Additional comments (10)
include/defaults.h (1)
197-204: LGTM! Well-chosen defaults for HTTP-based grid charger control.The default values are appropriate for smart plug control via HTTP (e.g., Shelly devices). The URI patterns and power thresholds align with typical use cases.
webapp/src/locales/de.json (1)
1136-1149: LGTM! Complete and accurate German translations.The localization strings for the HTTP provider UI are well-formed and cover all necessary fields.
src/gridcharger/Controller.cpp (1)
7-7: LGTM! HTTP provider integration follows established pattern.The implementation correctly integrates the HTTP provider following the same pattern as existing Huawei and Trucki providers.
Also applies to: 49-51
src/WebApi_gridcharger.cpp (1)
8-8: LGTM! HTTP configuration serialization correctly implemented.The serialization and deserialization of HTTP configuration follows the established pattern used for other providers.
Also applies to: 179-180, 249-249
include/Configuration.h (2)
247-247: LGTM! HTTP provider type correctly added to enum.The new HTTP provider type is properly defined following the existing pattern.
295-295: LGTM! HTTP configuration properly integrated.The HTTP member and serialization function declarations are correctly added following the existing pattern.
Also applies to: 512-512, 531-531
webapp/src/views/GridChargerAdminView.vue (1)
348-348: LGTM! HTTP provider option correctly added.The HTTP provider is properly added to the provider list with the correct key value.
webapp/src/types/GridChargerConfig.ts (1)
19-26: LGTM! TypeScript interface correctly defined.The
GridChargerHTTPConfiginterface properly mirrors the C++ structure and is correctly integrated into the main configuration.Also applies to: 43-43
src/gridcharger/HTTP/Stats.cpp (1)
1-36: LGTM! Stats implementation is clean and follows established patterns.The implementation correctly delegates to the
_dataPointsmember and provides appropriate live view data for the HTTP provider. The 10-second reachability threshold is reasonable for HTTP polling.Note: The static analysis error about
Arduino.hnot found is a false positive—this header is available in the PlatformIO build environment.include/gridcharger/HTTP/Provider.h (1)
4-4: Verify clang configuration for standard library headers.The static analysis tool reports that the
<atomic>header cannot be found, yetstd::atomic<bool>is used on line 72. This is likely a false positive due to misconfigured include paths in the static analysis environment, but please verify that the code compiles successfully in CI.
little change in Provider.cpp to set powerstate to false when not true Changes to be committed: modified: src/gridcharger/HTTP/Provider.cpp modified: webapp/src/locales/de.json modified: webapp/src/locales/en.json
Changes to be committed: modified: src/gridcharger/HTTP/Provider.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/gridcharger/HTTP/Provider.cpp (3)
89-111: Shared_httpGetterissue already flagged.The thread safety concern with
_httpGetterbeing shared betweensend_httpandread_httpwas identified in previous reviews. Use a localstd::unique_ptr<HttpGetter>instead of the shared member.
113-161: Unsafe optional dereference and shared_httpGetteralready flagged.Previous reviews identified the critical issues with unsafe
*oAcPowerdereference on error paths and the shared_httpGettermember. Additionally, lines 155-157 log an error when path resolution fails but still returnpathResolutionResult.firstrather than a safe fallback—this should also useoAcPower.value_or(0.0f)for consistency.
194-262:powerstatedata race already flagged.Previous reviews identified the race condition on
powerstateaccessed from bothpowerControlLoop()(main thread) andpollData()(polling task) without synchronization. Usestd::atomic<bool>forpowerstate.
🧹 Nitpick comments (1)
src/gridcharger/HTTP/Provider.cpp (1)
84-84: Remove stray semicolon.Unnecessary semicolon after closing brace.
- }; + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/gridcharger/HTTP/Provider.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{src,include,lib/Hoymiles,lib/MqttSubscribeParser,lib/TimeoutHelper,lib/ResetReason}/**/*.{c,cc,cpp,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
C/C++ code must pass cpplint with the specified filters
Files:
src/gridcharger/HTTP/Provider.cpp
🧠 Learnings (1)
📚 Learning: 2025-10-17T08:29:40.205Z
Learnt from: SW-Niko
Repo: hoylabs/OpenDTU-OnBattery PR: 2262
File: src/main.cpp:149-159
Timestamp: 2025-10-17T08:29:40.205Z
Learning: In the OpenDTU-OnBattery project (src/main.cpp), RuntimeData must be initialized after SolarCharger, PowerMeter, PowerLimiter, GridCharger, and Battery components have been initialized, but before the main loop. RuntimeData acts as a persistence service that these components access during their runtime operations (not during initialization).
Applied to files:
src/gridcharger/HTTP/Provider.cpp
🧬 Code graph analysis (1)
src/gridcharger/HTTP/Provider.cpp (1)
src/gridcharger/Controller.cpp (9)
init(20-28)init(20-20)loop(60-69)loop(60-60)lock(32-32)lock(62-62)lock(74-74)lock(81-81)lock(90-90)
🔇 Additional comments (5)
src/gridcharger/HTTP/Provider.cpp (5)
18-30: LGTM – consider validating config inputs.The initialization logic is straightforward. For robustness, you might consider returning
falseifurlis empty or if required config fields are missing, but this is optional.
53-67: LGTM!The polling task creation is properly guarded by the mutex, and the task is only started once.
264-270: Task helper structure looks correct.The
_dataPollingTaskDone = trueassignment at line 268 is part of the data race flagged earlier—ensure it uses atomic operations when_dataPollingTaskDoneis converted tostd::atomic<bool>.
272-291: LGTM!The polling loop correctly uses
wait_forwith a predicate for interruptible sleep, and releases the mutex during the potentially longpollData()call.
293-311: Data update synchronization is correct;powerstaterace already flagged.The
_dataCurrentupdate (lines 306-309) correctly uses scoped locking. Thepowerstatewrites at lines 299/303 are part of the data race flagged in previous reviews—convert to atomic store when fixing.
Changes to be committed: modified: src/gridcharger/HTTP/Provider.cpp
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/gridcharger/HTTP/Provider.cpp (1)
190-193:⚠️ Potential issue | 🟠 MajorClear
_batteryEmergencyChargingwhen the emergency condition ends.Once
getImmediateChargingRequest()flips back to false, this branch powers the charger off and returns, but it never resets_batteryEmergencyCharging. That leaves the provider stuck in the emergency path on every later loop and normal auto control never resumes.Suggested fix
if (_batteryEmergencyCharging && !stats->getImmediateChargingRequest()) { DTU_LOGI("Emergency Charge OFF %.02f", *oAcPower); PowerOFF(); + _batteryEmergencyCharging = false; return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gridcharger/HTTP/Provider.cpp` around lines 190 - 193, The branch that handles emergency charging (the if checking _batteryEmergencyCharging && !stats->getImmediateChargingRequest()) powers the charger off but never clears the _batteryEmergencyCharging flag, so the provider remains stuck in emergency mode; modify that branch in Provider.cpp to set _batteryEmergencyCharging = false (after calling PowerOFF() or just before returning) so normal auto control can resume once getImmediateChargingRequest() is no longer true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gridcharger/HTTP/Provider.cpp`:
- Around line 207-209: The code currently sets powerstate=false immediately
after calling PowerOFF(), which can incorrectly mark the plug off even if the
HTTP request failed; remove the unconditional assignments (the powerstate=false
lines after PowerOFF() at occurrences near PowerLimiter and elsewhere) and
instead only update powerstate when PowerOFF reports success (e.g., check
PowerOFF's return status or have PowerOFF update powerstate internally) so the
controller state reflects the actual outcome; update calls around PowerOFF() and
references to powerstate accordingly.
- Around line 18-29: Provider::init currently sets up URLs and power config but
does not reset the polling lifecycle flag _stopPollingData that deinit() sets to
true, causing Provider::loop to forever skip creating the polling task after a
disable/re-enable; modify Provider::init to explicitly reset lifecycle flags
(e.g., set _stopPollingData = false and any related
_pollingTaskCreated/_pollingStopped flags) so that loop() will proceed to
create/start the stats polling task again after re-initialization.
- Around line 228-247: Enable auto mode whenever battery support is available
and the inverter isn't producing, but only apply the SoC-stop behavior when
AutoPowerBatterySoCLimitsEnabled is true: change the condition that sets
_autoPowerEnabled to check config.Battery.Enabled and
PowerLimiter.isGovernedBatteryPoweredInverterProducing() (remove the dependency
on config.GridCharger.AutoPowerBatterySoCLimitsEnabled), then keep a separate if
that checks config.GridCharger.AutoPowerBatterySoCLimitsEnabled and _batterySoC
>= config.GridCharger.AutoPowerStopBatterySoCThreshold to call PowerOFF(), clear
_autoPowerEnabled and powerstate; remove or adjust the current else if
(powerstate) branch so it only turns the charger off when battery support is
disabled or inverter rules require it (not simply because the SoC-stop flag is
off), and preserve updating _autoModeBlockedTillMillis = millis() + 19900 after
enabling auto mode.
- Around line 91-93: The HttpRequest.Timeout is currently set to 60 (interpreted
as 60 ms) in send_http() and is also used by read_http(); update the timeout to
60000 (milliseconds) to represent 60 seconds and adjust the inline comment
accordingly. Locate the HttpRequest struct initialization (symbol: HttpRequest)
in send_http() and any usage in read_http(), change Timeout from 60 to 60000,
and ensure any comments referencing "60 seconds" match the millisecond value;
this aligns with HTTPClient::setTimeout() and setConnectTimeout() which expect
milliseconds.
---
Duplicate comments:
In `@src/gridcharger/HTTP/Provider.cpp`:
- Around line 190-193: The branch that handles emergency charging (the if
checking _batteryEmergencyCharging && !stats->getImmediateChargingRequest())
powers the charger off but never clears the _batteryEmergencyCharging flag, so
the provider remains stuck in emergency mode; modify that branch in Provider.cpp
to set _batteryEmergencyCharging = false (after calling PowerOFF() or just
before returning) so normal auto control can resume once
getImmediateChargingRequest() is no longer true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9fd484d5-c215-41e3-9888-173dc5a8a61a
📒 Files selected for processing (1)
src/gridcharger/HTTP/Provider.cpp
AndreasBoehm
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
include/Configuration.h (1)
271-278:⚠️ Potential issue | 🟡 MinorNormalize HTTP config declaration style and remove magic buffer sizes.
Line 271/Line 278 formatting is inconsistent with cpplint-friendly style, and Lines 272-275 hardcode
1025instead of the shared URL-length constant.Suggested patch
-struct GRID_CHARGER_HTTP_CONFIG_T{ - char url[1025]; - char uri_on[1025]; - char uri_off[1025]; - char uri_stats[1025]; +struct GRID_CHARGER_HTTP_CONFIG_T { + char url[HTTP_REQUEST_MAX_URL_STRLEN + 1]; + char uri_on[HTTP_REQUEST_MAX_URL_STRLEN + 1]; + char uri_off[HTTP_REQUEST_MAX_URL_STRLEN + 1]; + char uri_stats[HTTP_REQUEST_MAX_URL_STRLEN + 1]; char uri_powerparam[256]; float AcPower; -};using GridChargerHTTPConfig = struct GRID_CHARGER_HTTP_CONFIG_T; +}; +using GridChargerHTTPConfig = struct GRID_CHARGER_HTTP_CONFIG_T;As per coding guidelines, C/C++ code must pass cpplint with the specified filters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/Configuration.h` around lines 271 - 278, The struct GRID_CHARGER_HTTP_CONFIG_T definition uses inconsistent formatting and hardcoded 1025 buffer sizes; update the declaration to a cpplint-friendly style (one member per line, consistent spacing) and replace the magic sizes for url, uri_on, uri_off, and uri_stats with the shared URL-length constant (e.g., URL_MAX_LEN or the existing project constant) rather than literal 1025, keeping AcPower as float; preserve the alias GridChargerHTTPConfig and ensure the typedef/using placement follows the project's preferred pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@include/Configuration.h`:
- Around line 271-278: The struct GRID_CHARGER_HTTP_CONFIG_T definition uses
inconsistent formatting and hardcoded 1025 buffer sizes; update the declaration
to a cpplint-friendly style (one member per line, consistent spacing) and
replace the magic sizes for url, uri_on, uri_off, and uri_stats with the shared
URL-length constant (e.g., URL_MAX_LEN or the existing project constant) rather
than literal 1025, keeping AcPower as float; preserve the alias
GridChargerHTTPConfig and ensure the typedef/using placement follows the
project's preferred pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32c76540-cc76-493a-8389-86030f8f9190
📒 Files selected for processing (6)
include/Configuration.hinclude/defaults.hsrc/Configuration.cppsrc/WebApi_gridcharger.cppwebapp/src/locales/de.jsonwebapp/src/locales/en.json
✅ Files skipped from review due to trivial changes (3)
- webapp/src/locales/en.json
- include/defaults.h
- webapp/src/locales/de.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/WebApi_gridcharger.cpp
- src/Configuration.cpp
|
@Snoopy-HSS bitte beachte die Kommentare von @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 102269
🔴 Kritische Bugs (
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
include/Configuration.h (1)
271-279:⚠️ Potential issue | 🟡 MinorFix cpplint/style and replace hardcoded URL buffer sizes.
Line 271 and Line 278 still contain style issues (
struct ...{and};using ...), and1025is hardcoded for URL-like fields instead of reusingHTTP_REQUEST_MAX_URL_STRLEN + 1.♻️ Proposed fix
-struct GRID_CHARGER_HTTP_CONFIG_T{ - char url[1025]; - char uri_on[1025]; - char uri_off[1025]; - char uri_stats[1025]; +struct GRID_CHARGER_HTTP_CONFIG_T { + char url[HTTP_REQUEST_MAX_URL_STRLEN + 1]; + char uri_on[HTTP_REQUEST_MAX_URL_STRLEN + 1]; + char uri_off[HTTP_REQUEST_MAX_URL_STRLEN + 1]; + char uri_stats[HTTP_REQUEST_MAX_URL_STRLEN + 1]; char uri_powerparam[256]; float AcPower; -};using GridChargerHTTPConfig = struct GRID_CHARGER_HTTP_CONFIG_T; +}; +using GridChargerHTTPConfig = struct GRID_CHARGER_HTTP_CONFIG_T;As per coding guidelines,
C/C++ code must pass cpplint with the specified filters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/Configuration.h` around lines 271 - 279, The struct declaration GRID_CHARGER_HTTP_CONFIG_T and alias need cpplint fixes and replace hardcoded sizes: change the opening to "struct GRID_CHARGER_HTTP_CONFIG_T {" (space before '{'), replace the fixed 1025 and 256 sizes with the canonical constant (e.g. use HTTP_REQUEST_MAX_URL_STRLEN + 1 for url, uri_on, uri_off, uri_stats and for uri_powerparam per guideline), close with "};" on its own line and put "using GridChargerHTTPConfig = GRID_CHARGER_HTTP_CONFIG_T;" on a separate line; this ensures cpplint-style spacing and avoids hardcoded buffer lengths.webapp/src/locales/en.json (1)
1146-1146:⚠️ Potential issue | 🟡 MinorFix typo in user-facing hint text.
Line 1146 still has
Postitiveinstead ofPositive.✏️ Suggested fix
- "targetPowerConsumptionHint": "Postitive values use grid power to charge the battery. Negative values result in early shutdown", + "targetPowerConsumptionHint": "Positive values use grid power to charge the battery. Negative values result in early shutdown",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/locales/en.json` at line 1146, The user-facing string for the JSON key "targetPowerConsumptionHint" contains a typo ("Postitive"); update the value to correct the spelling to "Positive" while preserving the rest of the message ("Positive values use grid power to charge the battery. Negative values result in early shutdown") so the key targetPowerConsumptionHint reads the corrected sentence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webapp/src/locales/de.json`:
- Line 1149: The German locale uses the wrong key "HTTPAddressHint" while
GridChargerAdminView.vue expects "gridchargeradmin.HTTPipAddressHint"; add or
rename the entry in de.json to match the lookup (use key
"gridchargeradmin.HTTPipAddressHint" with the existing German string) so the
tooltip resolves correctly in GridChargerAdminView.vue.
In `@webapp/src/locales/en.json`:
- Line 1153: The locale key in en.json is currently "HTTPAddressHint" but the
admin component GridChargerAdminView.vue expects
"gridchargeradmin.HTTPipAddressHint"; update the locales to include the matching
key (add or rename "gridchargeradmin.HTTPipAddressHint" with the same string
value) so the tooltip for the HTTP IP/address field is available to
gridchargeradmin.HTTPipAddressHint.
---
Duplicate comments:
In `@include/Configuration.h`:
- Around line 271-279: The struct declaration GRID_CHARGER_HTTP_CONFIG_T and
alias need cpplint fixes and replace hardcoded sizes: change the opening to
"struct GRID_CHARGER_HTTP_CONFIG_T {" (space before '{'), replace the fixed 1025
and 256 sizes with the canonical constant (e.g. use HTTP_REQUEST_MAX_URL_STRLEN
+ 1 for url, uri_on, uri_off, uri_stats and for uri_powerparam per guideline),
close with "};" on its own line and put "using GridChargerHTTPConfig =
GRID_CHARGER_HTTP_CONFIG_T;" on a separate line; this ensures cpplint-style
spacing and avoids hardcoded buffer lengths.
In `@webapp/src/locales/en.json`:
- Line 1146: The user-facing string for the JSON key
"targetPowerConsumptionHint" contains a typo ("Postitive"); update the value to
correct the spelling to "Positive" while preserving the rest of the message
("Positive values use grid power to charge the battery. Negative values result
in early shutdown") so the key targetPowerConsumptionHint reads the corrected
sentence.
🪄 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
Run ID: fae20ed4-0d81-4d13-812e-b6f1153ed597
📒 Files selected for processing (5)
include/Configuration.hinclude/defaults.hsrc/Configuration.cppwebapp/src/locales/de.jsonwebapp/src/locales/en.json
✅ Files skipped from review due to trivial changes (1)
- include/defaults.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Configuration.cpp
Changes to be committed: modified: include/Configuration.h modified: include/gridcharger/HTTP/DataPoints.h modified: include/gridcharger/HTTP/Provider.h new file: src/gridcharger/HTTP/DataPoints.cpp modified: src/gridcharger/HTTP/Provider.cpp modified: webapp/src/locales/de.json modified: webapp/src/locales/en.json
|
@coderabbitai alles gut jetzt? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 23878
✅ Behoben
❌ Noch offen1. // src/gridcharger/HTTP/Provider.cpp
if (_batteryEmergencyCharging && !stats->getImmediateChargingRequest()) {
DTU_LOGI("Emergency Charge OFF %.02f", oAcPower.value_or(0.0f));
PowerOFF();
+ _batteryEmergencyCharging = false; // ← fehlt noch!
return;
}3. bool Provider::init()
{
+ _stopPollingData = false; // ← hinzufügen, sonst startet Polling-Task nach Re-enable nie
DTU_LOGI("Initialize HTTP AC charger interface...");
...
}7. Hardcodierte Puffergrößen in struct GRID_CHARGER_HTTP_CONFIG_T {
- char url[1025];
- char uri_on[1025];
- char uri_off[1025];
- char uri_stats[1025];
+ char url[HTTP_REQUEST_MAX_URL_STRLEN + 1];
+ char uri_on[HTTP_REQUEST_MAX_URL_STRLEN + 1];
+ char uri_off[HTTP_REQUEST_MAX_URL_STRLEN + 1];
+ char uri_stats[HTTP_REQUEST_MAX_URL_STRLEN + 1];8d. Fehlende `#include` <memory>
`#include` <mutex>
`#include` <condition_variable>9. Fehlende Validierung in // Nach der bestehenden Validierung hinzufügen:
if (root["provider"].as<uint8_t>() == static_cast<uint8_t>(GridChargerProviderType::HTTP)) {
if (!(root["HTTP"]["url"].is<const char*>()) ||
!(root["HTTP"]["AcPower"].is<float>())) {
retMsg["message"] = "HTTP values are missing or of wrong type!";
retMsg["code"] = WebApiError::GenericValueMissing;
response->setLength();
request->send(response);
return;
}
}10. UI-Platzhalter stimmt nicht mit -placeholder="http://192.168.2.153"
+placeholder="http://192.168.2.100"(Default in |
Changes to be committed:
modified: include/Configuration.h
modified: include/gridcharger/HTTP/Provider.h
modified: src/WebApi_gridcharger.cpp
modified: src/gridcharger/HTTP/Provider.cpp
modified: webapp/src/views/GridChargerAdminView.vue
|
@coderabbitai jetzt besser? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5273
Alle 10 Punkte sind vollständig adressiert. Der Code sieht gut aus! ✨ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
@AndreasBoehm Änderungen durchgeführt und getestet |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
include/Configuration.h (1)
271-278: Inconsistent field naming style inGRID_CHARGER_HTTP_CONFIG_T.This struct mixes
snake_case(url,uri_on,uri_off,uri_stats,uri_powerparam) withPascalCase(AcPower), and diverges from the naming convention used by every other*_CONFIG_Tstruct in this file (e.g.,HTTP_REQUEST_CONFIG_TusesUrl,AuthType,Username,Timeout; the nearbyGRID_CHARGER_HUAWEI_CONFIG_TusesOfflineVoltage,InputCurrentLimit, etc.). Also consider replacing the magic256foruri_powerparamwith a named constant for consistency with the other URL fields.♻️ Proposed rename for consistency
struct GRID_CHARGER_HTTP_CONFIG_T { - char url[HTTP_REQUEST_MAX_URL_STRLEN + 1]; - char uri_on[HTTP_REQUEST_MAX_URL_STRLEN + 1]; - char uri_off[HTTP_REQUEST_MAX_URL_STRLEN + 1]; - char uri_stats[HTTP_REQUEST_MAX_URL_STRLEN + 1]; - char uri_powerparam[256]; - float AcPower; + char Url[HTTP_REQUEST_MAX_URL_STRLEN + 1]; + char UriOn[HTTP_REQUEST_MAX_URL_STRLEN + 1]; + char UriOff[HTTP_REQUEST_MAX_URL_STRLEN + 1]; + char UriStats[HTTP_REQUEST_MAX_URL_STRLEN + 1]; + char UriPowerParam[HTTP_REQUEST_MAX_JSON_PATH_STRLEN + 1]; + float AcPower; };Requires corresponding updates in
src/Configuration.cpp(serialize/deserialize),src/gridcharger/HTTP/Provider.cpp(init()), and the JSON keys insrc/WebApi_gridcharger.cpp/webapp/src/types/GridChargerConfig.tsif you want the on-wire schema renamed too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/Configuration.h` around lines 271 - 278, The struct GRID_CHARGER_HTTP_CONFIG_T mixes naming styles and uses a magic 256; rename fields to match the other *_CONFIG_T PascalCase convention (Url, UriOn, UriOff, UriStats, UriPowerParam) and replace the hardcoded 256 with the existing URL-length constant (e.g., HTTP_REQUEST_MAX_URL_STRLEN + 1) or a new named constant; update all usages/serialization in Configuration.cpp (serialize/deserialize for GRID_CHARGER_HTTP_CONFIG_T), the init() and any references in src/gridcharger/HTTP/Provider.cpp, and the JSON/web API keys and types in src/WebApi_gridcharger.cpp and webapp/src/types/GridChargerConfig.ts to keep on-wire schema consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@include/Configuration.h`:
- Around line 271-278: The struct GRID_CHARGER_HTTP_CONFIG_T mixes naming styles
and uses a magic 256; rename fields to match the other *_CONFIG_T PascalCase
convention (Url, UriOn, UriOff, UriStats, UriPowerParam) and replace the
hardcoded 256 with the existing URL-length constant (e.g.,
HTTP_REQUEST_MAX_URL_STRLEN + 1) or a new named constant; update all
usages/serialization in Configuration.cpp (serialize/deserialize for
GRID_CHARGER_HTTP_CONFIG_T), the init() and any references in
src/gridcharger/HTTP/Provider.cpp, and the JSON/web API keys and types in
src/WebApi_gridcharger.cpp and webapp/src/types/GridChargerConfig.ts to keep
on-wire schema consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3d92187-e51d-445d-bfc6-0ccea581df19
📒 Files selected for processing (9)
include/Configuration.hinclude/gridcharger/HTTP/DataPoints.hinclude/gridcharger/HTTP/Provider.hsrc/WebApi_gridcharger.cppsrc/gridcharger/HTTP/DataPoints.cppsrc/gridcharger/HTTP/Provider.cppwebapp/src/locales/de.jsonwebapp/src/locales/en.jsonwebapp/src/views/GridChargerAdminView.vue
✅ Files skipped from review due to trivial changes (1)
- webapp/src/locales/de.json
🚧 Files skipped from review as they are similar to previous changes (3)
- src/WebApi_gridcharger.cpp
- webapp/src/views/GridChargerAdminView.vue
- src/gridcharger/HTTP/Provider.cpp
…n den Dateien gemacht, die du mir genannt hast. Bitte überprüfe sie noch einmal, bevor du den Commit machst. Changes to be committed: modified: include/Configuration.h modified: src/Configuration.cpp modified: src/WebApi_gridcharger.cpp modified: src/gridcharger/HTTP/Provider.cpp modified: webapp/src/types/GridChargerConfig.ts modified: webapp/src/views/GridChargerAdminView.vue
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Configuration.cpp (1)
272-280: JSON key naming is inconsistent with the rest of the config schema.All other sections in this file use
snake_caseJSON keys (e.g."ip_address","offline_voltage","auto_power_enabled"), but this serializer writesCamelCasekeys ("Url","UriOn","UriOff","UriStats","UriPowerParam","AcPower"). The matching section key is also"HTTP"(line 472) rather than"http"like the sibling"can","huawei","trucki"objects. Since this is a new on-disk format with no prior config to be backward-compatible with, please align to the project-wide convention so tooling, documentation, and manual JSON edits stay consistent. Don't forget to updatedeserializeGridChargerHTTPConfig(Lines 718–726), the"HTTP"object key inwrite()(Lines 472–473) /read()(Line 941), and the webapp type/view accordingly.♻️ Proposed change
void ConfigurationClass::serializeGridChargerHTTPConfig(GridChargerHTTPConfig const& source, JsonObject& target) { - target["Url"] = source.Url; - target["UriOn"] = source.UriOn; - target["UriOff"] = source.UriOff; - target["UriStats"] = source.UriStats; - target["UriPowerParam"] = source.UriPowerParam; - target["AcPower"] = source.AcPower; + target["url"] = source.Url; + target["uri_on"] = source.UriOn; + target["uri_off"] = source.UriOff; + target["uri_stats"] = source.UriStats; + target["uri_power_param"] = source.UriPowerParam; + target["ac_power"] = source.AcPower; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Configuration.cpp` around lines 272 - 280, The serializer serializeGridChargerHTTPConfig currently emits CamelCase keys ("Url","UriOn","UriOff","UriStats","UriPowerParam","AcPower") and the section key is "HTTP", which is inconsistent with the project's snake_case convention; change serializeGridChargerHTTPConfig to emit snake_case keys (e.g. "url","uri_on","uri_off","uri_stats","uri_power_param","ac_power"), update the corresponding deserializeGridChargerHTTPConfig to read the new snake_case names, rename the object key used in write() and read() from "HTTP" to "http" to match sibling sections, and adjust any webapp type/view references that expect the old CamelCase/"HTTP" names so everything uses snake_case consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gridcharger/HTTP/Provider.cpp`:
- Around line 24-29: The code concatenates config.GridCharger.HTTP.Url with
UriOn/UriOff/UriStats into _uriOn/_uriOff/_uriStats which can exceed
HTTP_REQUEST_MAX_URL_STRLEN and later gets silently truncated when copied into
HttpRequest.Url in send_http/read_http; in init() validate the combined length
(e.g., check Url.length() + UriOn.length() < HTTP_REQUEST_MAX_URL_STRLEN) and if
any combined URL would exceed the limit log a clear error via the existing
logger and refuse to enable the provider (return failure from init()), or
alternatively stop pre-concatenating and instead store base
config.GridCharger.HTTP.Url and the Uri* separately and build the final URL
inside send_http/read_http while enforcing the same length check before copying
into HttpRequest.Url.
- Around line 285-297: The current Provider::pollData() incorrectly sets
_powerState from measured acPowerCurrent; change this so measured power only
updates the datapoint/stat (_dataCurrent / _stats) and does not flip the
commanded plug state. Instead, set _powerState only from explicit
PowerON()/PowerOFF() success paths (or introduce a separate variable e.g.
_commandedPowerState) and, if you want a confirmation from measurement, only
treat acPowerCurrent > 0 as a one-way confirmation to set _powerState = true
(never set it false based on acPowerCurrent). Update pollData() to stop
assigning _powerState = (acPowerCurrent > 0) and adjust
powerControlLoop/PowerON/PowerOFF success handling to maintain the commanded
state accordingly.
---
Nitpick comments:
In `@src/Configuration.cpp`:
- Around line 272-280: The serializer serializeGridChargerHTTPConfig currently
emits CamelCase keys
("Url","UriOn","UriOff","UriStats","UriPowerParam","AcPower") and the section
key is "HTTP", which is inconsistent with the project's snake_case convention;
change serializeGridChargerHTTPConfig to emit snake_case keys (e.g.
"url","uri_on","uri_off","uri_stats","uri_power_param","ac_power"), update the
corresponding deserializeGridChargerHTTPConfig to read the new snake_case names,
rename the object key used in write() and read() from "HTTP" to "http" to match
sibling sections, and adjust any webapp type/view references that expect the old
CamelCase/"HTTP" names so everything uses snake_case consistently.
🪄 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
Run ID: 6a1253b6-dffd-4097-8d54-9e95bba10c6c
📒 Files selected for processing (6)
include/Configuration.hsrc/Configuration.cppsrc/WebApi_gridcharger.cppsrc/gridcharger/HTTP/Provider.cppwebapp/src/types/GridChargerConfig.tswebapp/src/views/GridChargerAdminView.vue
🚧 Files skipped from review as they are similar to previous changes (3)
- webapp/src/types/GridChargerConfig.ts
- src/WebApi_gridcharger.cpp
- webapp/src/views/GridChargerAdminView.vue
| String url = config.GridCharger.HTTP.Url; | ||
| _uriOn = url + config.GridCharger.HTTP.UriOn; | ||
| _uriOff = url + config.GridCharger.HTTP.UriOff; | ||
| _uriStats = url + config.GridCharger.HTTP.UriStats; | ||
| _uriPowerparam = config.GridCharger.HTTP.UriPowerParam; | ||
| _maximumAcPower = config.GridCharger.HTTP.AcPower; |
There was a problem hiding this comment.
Silent URL truncation risk on concatenation.
config.GridCharger.HTTP.Url and the Uri* fields are each sized HTTP_REQUEST_MAX_URL_STRLEN + 1 (1025). Their concatenation into _uriOn/_uriOff/_uriStats can reach ~2048 chars. Later strlcpy(HttpRequest.Url, Url.c_str(), sizeof(HttpRequest.Url)) in send_http/read_http silently truncates to 1024, which can produce a malformed-but-plausible URL (and a confusing failure mode for users who supply long base URLs or paths). Consider either (a) validating combined length in init() and logging a clear error / refusing to enable the provider, or (b) splitting base URL and URI into separate HttpRequestConfig fields rather than pre-concatenating into one 1025-byte buffer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gridcharger/HTTP/Provider.cpp` around lines 24 - 29, The code
concatenates config.GridCharger.HTTP.Url with UriOn/UriOff/UriStats into
_uriOn/_uriOff/_uriStats which can exceed HTTP_REQUEST_MAX_URL_STRLEN and later
gets silently truncated when copied into HttpRequest.Url in send_http/read_http;
in init() validate the combined length (e.g., check Url.length() +
UriOn.length() < HTTP_REQUEST_MAX_URL_STRLEN) and if any combined URL would
exceed the limit log a clear error via the existing logger and refuse to enable
the provider (return failure from init()), or alternatively stop
pre-concatenating and instead store base config.GridCharger.HTTP.Url and the
Uri* separately and build the final URL inside send_http/read_http while
enforcing the same length check before copying into HttpRequest.Url.
| void Provider::pollData() | ||
| { | ||
| float acPowerCurrent = read_http(_uriStats); | ||
| DTU_LOGV("acPowerCurrent: %f", acPowerCurrent); | ||
| _powerState = (acPowerCurrent > 0); | ||
|
|
||
| // Update data points | ||
| { | ||
| auto scopedLock = _dataCurrent.lock(); | ||
| _dataCurrent.add<DataPointLabel::AcPower>(acPowerCurrent); | ||
| } | ||
| _stats->updateFrom(_dataCurrent); | ||
| } |
There was a problem hiding this comment.
_powerState = (acPowerCurrent > 0) conflates "plug on" with "charger drawing power".
In pollData() the plug state is inferred from measured AC power. A charger that is plugged into a smart-plug that is physically ON may still report ~0 W when idle (e.g. between CV/CC transitions, after the battery is near full, or briefly after switching on). This will flip _powerState to false in the polling task, and on the next powerControlLoop() iteration the auto-control branch at Line 247 (!_powerState) may then re-issue PowerON(), oscillating between ON commands, and the emergency-OFF and inverter-active branches will also no longer fire because they guard on _powerState.
Consider tracking the commanded plug state separately from the observed AC power (e.g. only update _powerState from PowerON()/PowerOFF() success paths, and expose acPowerCurrent purely as a datapoint / stat), or at minimum only treat acPowerCurrent > 0 as a one-way confirmation of ON and not as evidence of OFF.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gridcharger/HTTP/Provider.cpp` around lines 285 - 297, The current
Provider::pollData() incorrectly sets _powerState from measured acPowerCurrent;
change this so measured power only updates the datapoint/stat (_dataCurrent /
_stats) and does not flip the commanded plug state. Instead, set _powerState
only from explicit PowerON()/PowerOFF() success paths (or introduce a separate
variable e.g. _commandedPowerState) and, if you want a confirmation from
measurement, only treat acPowerCurrent > 0 as a one-way confirmation to set
_powerState = true (never set it false based on acPowerCurrent). Update
pollData() to stop assigning _powerState = (acPowerCurrent > 0) and adjust
powerControlLoop/PowerON/PowerOFF success handling to maintain the commanded
state accordingly.
2 participants
Löst den Push Request #1330 ab.
Angepasst an den neuen Controller Provider für AC Charger.
Changes to be committed:
modified: include/Configuration.h
modified: include/defaults.h
new file: include/gridcharger/HTTP/DataPoints.h
new file: include/gridcharger/HTTP/Provider.h
new file: include/gridcharger/HTTP/Stats.h
modified: src/Configuration.cpp
modified: src/WebApi_gridcharger.cpp
modified: src/gridcharger/Controller.cpp
new file: src/gridcharger/HTTP/Provider.cpp
new file: src/gridcharger/HTTP/Stats.cpp
modified: webapp/src/locales/de.json
modified: webapp/src/locales/en.json
modified: webapp/src/types/GridChargerConfig.ts
modified: webapp/src/views/GridChargerAdminView.vue