Add optional EcoFlow CDC telemetry to usbhid-ups - #3569
Conversation
Signed-off-by: user01010111 <lapses.50.booster@icloud.com>
|
A ZIP file with standard source tarball and another tarball with pre-built docs for commit 55bf78d is temporarily available: NUT-tarballs-PR-3569.zip. |
|
❌ Build nut 2.8.5.5051-master failed (commit 637bf29682 by @) |
|
❌ Build nut 2.8.5.5051-master failed (commit 637bf29682 by @) |
Signed-off-by: user01010111 <lapses.50.booster@icloud.com>
Signed-off-by: user01010111 <lapses.50.booster@icloud.com>
|
❌ Build nut 2.8.5.5052-master failed (commit 55d99ae42f by @) |
Signed-off-by: user01010111 <lapses.50.booster@icloud.com>
|
❌ Build nut 2.8.5.5053-master failed (commit bb35bca927 by @) |
|
✅ Build nut 2.8.5.5054-master completed (commit 6506ebb503 by @)
|
|
✅ Build nut 2.8.5.5054-master completed (commit 6506ebb503 by @) |
jimklimov
left a comment
There was a problem hiding this comment.
Quite solid, thanks. Commented on a couple of stylistic nits, but neither is something that requires an immediate code change - more as hints for future contributions.
| "system temperature parses"); | ||
| check(metrics.has_battery_temperature && metrics.battery_temperature == 28, | ||
| "battery temperature parses"); | ||
| check(metrics.has_output_power && fabs(metrics.output_power - 258.768) < 0.01, |
There was a problem hiding this comment.
One minor suggestion, not a blocker: in include/nut_float.h we have #define f_equal(x, y) ( fabsf((float)(x) - (float)(y)) <= FLT_EPSILON ) (and other similar macros) to make code like this simpler to read. In a test this is probably not even worth fixing, so just to let you know if this pattern would appear elsewhere.
| upsdebugx(1, "upsdrv_cleanup..."); | ||
|
|
||
| if (subdriver != NULL && subdriver->aux != NULL && | ||
| subdriver->aux->cleanup != NULL) { |
There was a problem hiding this comment.
A little style nit, I think applies to various spots in both of your PRs, and is loosely documented at docs/developers.txt section Indenting with tabs vs. spaces (maybe should be rewritten more explicitly, so I'll update) - we find it easier to read indented code with a visually clear boundary of where the opening clause ends and a scoped code section starts. So for multi-line clauses, the final parentheses tend to go onto their own line with original indent level:
if (A &&
B) {
...
}
becomes more like
if (A
&& B
) {
...
}
or
if (A
&& B
) {
...
}
A single-line clause remains indented as it was:
if (A && B) {
...
}
I think there's no immediate need to change this PR, unless you plan more work on the feature. There are similar "offenders" elsewhere in the code base and I hope would be eventually covered with some code styling automation, although our particular set of tweaks about convenient C mark-up does not seem common (for .editorconfig, .clang-format et al) so such automation is still waiting for its maker :)
jimklimov
left a comment
There was a problem hiding this comment.
Ah, my earlier review missed an aspect that does need a change: bump driver versions (usbhid-ups.c itself, and the *-hid.c subdrivers, due to structure format change), at least if nothing else bumped them since v2.8.5 release.
I wonder also if ecoflow-cdc{,-protocol}.{c,h} might stand as a driver code base on its own merit, or is strictly meant to be part of usbhid-ups? Maybe introducing a structured naming for the new aux tweaks should be useful (e.g. we have ecoflow-hid for usbhid-ups, so AUX tweaks could be ecoflow-hid-aux-something - WDYT?)
|
Thanks for the review. I’ve updated the branch to bump I retained |
Signed-off-by: user01010111 <lapses.50.booster@icloud.com>
eb4e00f to
55bf78d
Compare
|
✅ Build nut 2.8.5.5057-master completed (commit d83200915e by @)
|
|
✅ Build nut 2.8.5.5058-master completed (commit adc52e30d2 by @)
|
Summary
ecoflow_cdc_port.experimental.ecoflow.*.Rationale
The EcoFlow RIVER 3 Plus HID interface provides the data needed for UPS monitoring and shutdown, but it does not provide live power and temperature telemetry. The same USB device exposes richer read-only data through a CDC ACM interface.
This change keeps HID authoritative and enriches the same NUT device instead of creating a second partial UPS. CDC support is opt-in. A missing or failed CDC connection reconnects independently and never marks the HID data stale.
The protocol framing and field work builds on the public r3pcomms project and was independently verified against a RIVER 3 Plus.
Related to #2735.
Safety
ecoflow_cdc_portis configured.Testing
make -j2 check: 7 passed, 0 failed.usbhid-upsbuild with serial support enabled.usbhid-upsbuild with serial support disabled.reconnect.tryingwhile HID remained responsive and continued reporting status, charge and runtime.Contributor note
I used an AI coding assistant during implementation. I reviewed the resulting changes and validated them with the tests and hardware checks listed above.
The commit includes the required DCO sign-off.