Conversation
There was a problem hiding this comment.
Copilot reviewed 7 out of 10 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
- webapp/src/locales/de.json: Language not supported
- webapp/src/locales/en.json: Language not supported
- webapp/src/locales/fr.json: Language not supported
|
Update, System 2 days and 12h online Logging: |
Build ArtifactsFirmware built from this pull request's code:
Notice
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds functionality to log and display VE.Direct transmission errors in greater detail for both the MPPT charge controllers and the Victron SmartShunt. Key changes include:
- Adding transmission error values in the JSON output for MPPT and SmartShunt.
- Introducing new error counter and logging functions in the VeDirect frame handler.
- Updating the VE.Direct data structure to map error codes to human‐readable strings.
Reviewed Changes
Copilot reviewed 8 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/solarcharger/victron/Stats.cpp | Adds JSON entries for MPPT transmission errors. |
| src/battery/victronsmartshunt/Stats.cpp | Updates Live View data to include transmission error value. |
| lib/VeDirectFrameHandler/VeDirectFrameHexHandler.cpp | Updates error handling to include HEX_CHECKSUM errors. |
| lib/VeDirectFrameHandler/VeDirectFrameHandler.h | Declares new error counter and logging function members. |
| lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp | Implements error counting and logging, including transmission error averaging. |
| lib/VeDirectFrameHandler/VeDirectData.h and VeDirectData.cpp | Adds mapping for transmission error codes to readable strings. |
| include/battery/victronsmartshunt/Stats.h | Adds a new member to hold transmission error data for the SmartShunt. |
Files not reviewed (3)
- webapp/src/locales/de.json: Language not supported
- webapp/src/locales/en.json: Language not supported
- webapp/src/locales/fr.json: Language not supported
Comments suppressed due to low confidence (1)
src/solarcharger/victron/Stats.cpp:255
- [nitpick] Consider aligning the naming for transmission error keys across device types. For consistency, either use 'MpptTransmitError' or a similar naming convention as used in the SmartShunt ('transmitError').
device["MpptTransmitError"]["v"] = mpptData.transmissionErrors_Day;
881f70a to
c68f9c2
Compare
|
Now using DTU_LOGI to log the transmission errors. |
d196439 to
12fa025
Compare
|
Small improvement: Do not print the transmission faults in the logging if we do not have any. |
e2985bd to
f6946ca
Compare
f6946ca to
7a3f960
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds transmit-error tracking across VE-Direct parsing: new Error enum and per-error counters in the frame handler, periodic conversion to a daily rate, propagation into veStruct, battery/charger stats, and UI localization keys. Changes
Sequence DiagramsequenceDiagram
participant Frame as FrameHandler
participant Data as VeDirectData
participant Stats as StatsModule
participant UI as WebUI
Frame->>Frame: receive bytes / parse
Frame->>Frame: detect error (TIMEOUT, CHECKSUM, BUFFER, etc.)
Frame->>Frame: call setErrorCounter(Error) -> increment per-type counter and _errorSumSinceStartup
Note over Frame: every 60s
Frame->>Frame: compute transmitErrors_Day from _errorSumSinceStartup
Frame->>Data: assign transmitErrors_Day
Data->>Stats: updateFrom(veStruct)
Stats->>Stats: store _transmitErrors and add to live/JSON payloads
Stats->>UI: publish localized payload
UI->>UI: render label and value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
include/battery/victronsmartshunt/Stats.h (1)
34-34: Initialize the new member explicitly.Please default-initialize
_transmissionErrorsto avoid undefined startup values in any non-zero-initialized construction path.♻️ Proposed change
- float _transmissionErrors; + float _transmissionErrors {0.0f};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/battery/victronsmartshunt/Stats.h` at line 34, The new member float _transmissionErrors is not explicitly initialized; initialize it to a known value (e.g., 0.0f) to avoid undefined startup values by either giving it an in-class default (float _transmissionErrors = 0.0f;) or adding it to the Stats constructor initializer list (Stats(...) : ..., _transmissionErrors(0.0f) { }). Target the _transmissionErrors field in the Stats class/constructor when making this change.lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp (2)
336-337: Consider whetherUNKNOWN_TEXT_DATAshould count as a transmission error.Unknown fields may not indicate transmission errors but rather unsupported device features. Counting these as errors could inflate the transmission error metric for devices with extra fields that this code doesn't parse yet.
If this is intentional for monitoring purposes, consider documenting that this counter includes both actual errors and unsupported fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp` around lines 336 - 337, The code currently treats unknown text fields as transmission errors by calling setErrorCounter(veStruct::Error::UNKNOWN_TEXT_DATA) alongside DTU_LOGI in VeDirectFrameHandler.cpp; change this so unsupported/unknown fields do not inflate the transmission error metric — either remove the setErrorCounter call for veStruct::Error::UNKNOWN_TEXT_DATA, replace it with a non-error counter (e.g., a warning/unsupported-field metric), or gate the increment behind a configurable flag, and add a brief comment documenting the chosen behavior so future readers know whether UNKNOWN_TEXT_DATA is considered a true transmission error or just an unsupported field.
122-134: Day 1 metric may be misleading to users.When
errorDays <= 1.0f,transmissionErrors_Dayequals the raw error sum rather than a rate. Since the UI displays this with unit "1/d", users may interpret it as a daily rate when it's actually just the total count during the first day.Consider either:
- Setting
transmissionErrors_Dayto 0 or a sentinel value until day 1 passes- Computing the rate even on day 1 (e.g.,
errors / errorDaysregardless of value)- Documenting this behavior in the UI or logging
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp` around lines 122 - 134, The current logic sets _tmpFrame.transmissionErrors_Day to the raw error sum when errorDays <= 1.0f which mislabels a count as "1/d"; change the calculation so transmissionErrors_Day is always a rate: compute errorDays = esp_timer_get_time() / (24.0f*60*60*1000*1000) and set _tmpFrame.transmissionErrors_Day = _errorCounter.at(static_cast<size_t>(veStruct::Error::SUM)) / errorDays (handle the unlikely zero case defensively), rather than skipping the division when errorDays <= 1.0f; update any related logging/printing (printErrorCounter() call) if you want to avoid noisy output on very small errorDays.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp`:
- Around line 392-408: The SUM-based halving in
VeDirectFrameHandler<T>::setErrorCounter (which updates _errorCounter and checks
veStruct::Error::SUM > 50000) skews transmissionErrors_Day because the numerator
is reduced but the elapsed-time denominator is not; fix by tracking extra state
(e.g., add a lastScaleTime or cumulativeScaleFactor member) and update how
transmissionErrors_Day is computed to account for the scale: either record the
timestamp when you halve counters and use that as the start time for rate
calculation, or maintain a cumulative scaling divisor applied when computing the
effective total so the daily rate reflects pre-scale counts; update
setErrorCounter to adjust the new state (lastScaleTime or cumulativeScaleFactor)
whenever you downscale so transmissionErrors_Day uses those values to compute an
accurate rate.
---
Nitpick comments:
In `@include/battery/victronsmartshunt/Stats.h`:
- Line 34: The new member float _transmissionErrors is not explicitly
initialized; initialize it to a known value (e.g., 0.0f) to avoid undefined
startup values by either giving it an in-class default (float
_transmissionErrors = 0.0f;) or adding it to the Stats constructor initializer
list (Stats(...) : ..., _transmissionErrors(0.0f) { }). Target the
_transmissionErrors field in the Stats class/constructor when making this
change.
In `@lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp`:
- Around line 336-337: The code currently treats unknown text fields as
transmission errors by calling
setErrorCounter(veStruct::Error::UNKNOWN_TEXT_DATA) alongside DTU_LOGI in
VeDirectFrameHandler.cpp; change this so unsupported/unknown fields do not
inflate the transmission error metric — either remove the setErrorCounter call
for veStruct::Error::UNKNOWN_TEXT_DATA, replace it with a non-error counter
(e.g., a warning/unsupported-field metric), or gate the increment behind a
configurable flag, and add a brief comment documenting the chosen behavior so
future readers know whether UNKNOWN_TEXT_DATA is considered a true transmission
error or just an unsupported field.
- Around line 122-134: The current logic sets _tmpFrame.transmissionErrors_Day
to the raw error sum when errorDays <= 1.0f which mislabels a count as "1/d";
change the calculation so transmissionErrors_Day is always a rate: compute
errorDays = esp_timer_get_time() / (24.0f*60*60*1000*1000) and set
_tmpFrame.transmissionErrors_Day =
_errorCounter.at(static_cast<size_t>(veStruct::Error::SUM)) / errorDays (handle
the unlikely zero case defensively), rather than skipping the division when
errorDays <= 1.0f; update any related logging/printing (printErrorCounter()
call) if you want to avoid noisy output on very small errorDays.
🪄 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: 6e2ebcd5-87e6-4e9d-a8c4-b9857152eea3
📒 Files selected for processing (10)
include/battery/victronsmartshunt/Stats.hlib/VeDirectFrameHandler/VeDirectData.cpplib/VeDirectFrameHandler/VeDirectData.hlib/VeDirectFrameHandler/VeDirectFrameHandler.cpplib/VeDirectFrameHandler/VeDirectFrameHandler.hlib/VeDirectFrameHandler/VeDirectFrameHexHandler.cppsrc/battery/victronsmartshunt/Stats.cppsrc/solarcharger/victron/Stats.cppwebapp/src/locales/de.jsonwebapp/src/locales/en.json
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/VeDirectFrameHandler/VeDirectData.cpp`:
- Around line 367-379: The code lacks a veStruct::Error value for the "Unknown
Data" bucket and so getTransmitErrorAsString cannot return a string for it; add
a new enum member (e.g., UNKNOWN_DATA) to veStruct::Error in the header and add
a corresponding entry in the frozen::map inside getTransmitErrorAsString (map
key UNKNOWN_DATA -> "Unknown Data"), then update any places that classify frames
as unknown-data to set that enum value so logs/UI can display it (refer to
getTransmitErrorAsString and the veStruct::Error enum).
In `@lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp`:
- Line 398: printErrorCounter() never emits the accumulated raw total stored in
_errorSumSinceStartup, so update the function(s) that format and log error
statistics (e.g., printErrorCounter and any related summary formatter used
around the block that handles entries at lines 414-431) to append a "Total Sum"
line showing _errorSumSinceStartup; specifically, locate printErrorCounter()
(and the summary code that formats _errorCounter entries) and add a formatted
output that includes the numeric value of _errorSumSinceStartup in the same log
output so the sample "Total Sum" line can be emitted.
- Around line 175-182: Initialize the member _prevState to a known safe value in
the constructor (e.g., State::IDLE) and change the nested-hex detection in
VeDirectFrameHandler:: (the block that currently checks if ((inbyte == ':') &&
(_state != State::CHECKSUM))) to test the current state (_state ==
State::RECORD_HEX) instead of _prevState when deciding to call
setErrorCounter(veStruct::Error::NESTED_HEX); keep the existing _prevState
update logic around the RECORD_HEX transition so the previous state is still
recorded for other uses.
🪄 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: 78ff47e8-0fdd-4977-b071-79be659dd57f
📒 Files selected for processing (10)
include/battery/victronsmartshunt/Stats.hlib/VeDirectFrameHandler/VeDirectData.cpplib/VeDirectFrameHandler/VeDirectData.hlib/VeDirectFrameHandler/VeDirectFrameHandler.cpplib/VeDirectFrameHandler/VeDirectFrameHandler.hlib/VeDirectFrameHandler/VeDirectFrameHexHandler.cppsrc/battery/victronsmartshunt/Stats.cppsrc/solarcharger/victron/Stats.cppwebapp/src/locales/de.jsonwebapp/src/locales/en.json
✅ Files skipped from review due to trivial changes (2)
- src/solarcharger/victron/Stats.cpp
- src/battery/victronsmartshunt/Stats.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- include/battery/victronsmartshunt/Stats.h
- webapp/src/locales/de.json
- webapp/src/locales/en.json
- lib/VeDirectFrameHandler/VeDirectFrameHexHandler.cpp
- lib/VeDirectFrameHandler/VeDirectData.h
- lib/VeDirectFrameHandler/VeDirectFrameHandler.h
- detect and count different transmit errors - logging of errors - can be used to qualify the transmit quality
- add error counter to MPPT - add error counter to Smart Shunt
7fd97ad to
252a3e0
Compare

I've implemented a transmission error log for the VE.Direct connections (charge controller, shunt). The Live View displays the recorded transmission errors for each device per day. This allows you to check the quality of your VE.Direct connection.
Additionally, the log displays transmission errors in more detail every 60 seconds. You can see which errors (checksum, buffer overflow, etc.) were detected.
23:13:09.079 > [VE.Direct MPPT 10/9] Average transmission errors per day: 0.0
23:13:09.079 > [VE.Direct MPPT 10/9] Total Sum: 0, Frame Timeout: 0, Text Checksum: 0
23:13:09.133 > [VE.Direct MPPT 10/9] Hex Checksum: 0, Hex Buffer: 0, Nested Hex: 0
23:13:09.133 > [VE.Direct MPPT 10/9] Debug Buffer: 0, Unknown Data: 0, Invalid Char: 0
For the sake of completeness, I should also mention: