Skip to content

add support for EcoFlow River 3 Plus - #2837

Merged
jimklimov merged 15 commits into
networkupstools:masterfrom
greyltc:ecoflow
Mar 13, 2025
Merged

jimklimov merged 15 commits into
networkupstools:masterfrom
greyltc:ecoflow

Conversation

@greyltc

@greyltc greyltc commented Mar 7, 2025 •

Copy link
Copy Markdown
Contributor

Here's a pull request to address #2735 (MAINTAINER NOTE: initially an alternative to PR #2740, now assimilated that work - so became a successor to that draft PR)

I know there's another PR out there to address this same issue, but I felt like going through the process myself just for fun/learning.

I've only been able to test it on my River 3 Plus (with the latest firmware V1.32.76.52), but I think the changes here might work as is for some of EcoFlow's other new battery banks with UPS support like their Delta 3 Plus.

The hardware feature set seems pretty anemic at this point (or at least gen-usbhid-subdriver.sh didn't find that many variables), but I think I've got pretty much everything the hardware currently supports working here. I've done my best to remap the nut variable names as close to the spec and to what other drivers do as I could. The ones I couldn't find a good mapping for, I put as experimental. so what you see in the attached nut monitor screenshot below is everything that gen-usbhid-subdriver.sh came up with.

Screenshot From 2025-03-06 23-43-04
Screenshot From 2025-03-06 23-43-56

General points

  • Described the changes in the PR submission or a separate issue, e.g.
    known published or discovered protocols, applicable hardware (expected
    compatible and actually tested/developed against), limitations, etc.

  • There may be multiple commits in the PR, aligned and commented with
    a functional change. Notably, coding style changes better belong in a
    separate PR, but certainly in a dedicated commit to simplify reviews
    of "real" changes in the other commits. Similarly for typo fixes in
    comments or text documents.

  • Please star NUT on GitHub, this helps with sponsorships! ;)

Frequent "underwater rocks" for driver addition/update PRs

  • Revised existing driver families and added a sub-driver if applicable
    (nutdrv_qx, usbhid-ups...) or added a brand new driver in the other
    case.

  • Did not extend obsoleted drivers with new hardware support features
    (notably blazer and other single-device family drivers for Qx protocols,
    except the new nutdrv_qx which should cover them all).

  • For updated existing device drivers, bumped the DRIVER_VERSION macro
    or its equivalent.

  • For USB devices (HID or not), revised that the driver uses unique
    VID/PID combinations, or raised discussions when this is not the case
    (several vendors do use same interface chips for unrelated protocols).

  • For new USB devices, built and committed the changes for the
    scripts/upower/95-upower-hid.hwdb file

  • Proposed NUT data mapping is aligned with existing docs/nut-names.txt
    file. If the device exposes useful data points not listed in the file, the
    experimental.* namespace can be used as documented there, and discussion
    should be raised on the NUT Developers mailing list to standardize the new
    concept.

  • Updated data/driver.list.in if applicable (new tested device info)

Frequent "underwater rocks" for general C code PRs

  • Did not "blindly assume" default integer type sizes and value ranges,
    structure layout and alignment in memory, endianness (layout of bytes and
    bits in memory for multi-byte numeric types), or use of generic int where
    language or libraries dictate the use of size_t (or ssize_t sometimes).
  • Progress and errors are handled with upsdebugx(), upslogx(),
    fatalx() and related methods, not with direct printf() or exit().
    Similarly, NUT helpers are used for error-checked memory allocation and
    string operations (except where customized error handling is needed,
    such as unlocking device ports, etc.)

  • Coding style (including whitespace for indentations) follows precedent
    in the code of the file, and examples/guide in docs/developers.txt file.

  • For newly added files, the Makefile.am recipes were updated and the
    make distcheck target passes.

General documentation updates

  • Updated docs/acknowledgements.txt (for vendor-backed device support)

  • Added or updated manual page information in docs/man/*.txt files
    and corresponding recipe lists in docs/man/Makefile.am for new pages

  • Passed make spellcheck, updated spell-checking dictionary in the
    docs/nut.dict file if needed (did not remove any words -- the make
    rule printout in case of changes suggests how to maintain it).

Additional work may be needed after posting this PR

  • Propose a PR for NUT DDL with detailed device data dumps from tests
    against real hardware (the more models, the better).

  • Address NUT CI farm build failures for the PR: testing on numerous
    platforms and toolkits can expose issues not seen on just one system.

  • Revise suggestions from LGTM.COM analysis about "new issues" with
    the changed codebase.

ykuksenko and others added 4 commits December 29, 2024 21:13
Basic status works (charging, discharging, online, onbattery, ...).
Charge level works.
Discharge level from app shows correctly.
Any sort of control does not seem to work for load or beeper/light.
See code comments for details.

Signed-off-by: Yevgeniy Kuksenko <2882631+ykuksenko@users.noreply.github.com>
@jimklimov

Copy link
Copy Markdown
Member

Thanks! I wonder about merging the two PRs, yours and #2740 (they luckily deliver same-named files) - gotta check if that one has any unique mapping content (at least detailed comments pop out, and a generated hwdb file). In particular, that would lead to both of you scoring a contribution for the effort done :) WDYT?

@greyltc

greyltc commented Mar 7, 2025 •

Copy link
Copy Markdown
Contributor Author

WDYT?

I've merged in the hwdb file from @ykuksenko's branch. I'm unsure exactly how to handle their comments in drivers/ecoflow-hid.c, so I left those out. It also seems to me that at least some of the variables there do not match with what gen-usbhid-subdriver.sh gave me, which I assume is due to changes in EcoFlow's firmware since December '24.

…dd (C) of contributors [issue networkupstools#2735, PRs networkupstools#2740, networkupstools#2837]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
… "#if WITH_UNMAPPED_DATA_POINTS" [networkupstools#2837]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…ksenko variant [networkupstools#2837]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…etworkupstools#2740, networkupstools#2837]

* import back comments about findings in PR networkupstools#2740,
* apply reasonable number precision and HU_FLAG* where two
  codebases differed,
* clean up "unmapped.*" entries that are actually exposed as
  standard or experimental data points, or in some cases drop
  the "experimental.*" ones (it is easy to build code with
  settings which would show the same values as "unmapped.*")

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
@jimklimov

Copy link
Copy Markdown
Member

Thanks, I've reviewed the rest - hopefully reasonably de-duplicated some, picked the better flags for others...

@greyltc @ykuksenko : testing of this resulting PR branch would be very welcome, to make sure conversion methods in mapping tables are listed correctly, at least...

@jimklimov jimklimov mentioned this pull request Mar 7, 2025
2 of 20 tasks
@jimklimov jimklimov added the need testing Code looks reasonable, but the feature would better be tested against hardware or OSes label Mar 7, 2025
@greyltc

greyltc commented Mar 8, 2025

Copy link
Copy Markdown
Contributor Author

fde62e5 means now that if the build is done with WITH_UNMAPPED_DATA_POINTS defined, there's 100% 1:1 duplication between the mapped and unmapped variables, FYI.

988910f merges in a bunch of variables and notes that only apply to an an obsolete firmware from last December.
Three easy examples that have jumped out at me:

  1. I know the comment https://github.com/greyltc/nut/blob/988910f3588a710ab39678908853dc408f8d025f/drivers/ecoflow-hid.c#L88
    is wrong (i.e. the capacity reported matches the datasheet).

  2. UPS.PowerSummary.PresentStatus.CommunicationLost in https://github.com/greyltc/nut/blob/988910f3588a710ab39678908853dc408f8d025f/drivers/ecoflow-hid.c#L117 is not a variable that exists today.

  3. UPS.PowerSummary.PresentStatus.Overload does not exist today...

Many of the comments look like they're documenting struggles with buggy firmware that has been superseded or are commented out duplicated code.

988910f needs to be reverted imop.

@jimklimov

jimklimov commented Mar 8, 2025 •

Copy link
Copy Markdown
Member

Thanks. Not sure I have an opinion here, just... some thoughts:

  • Not all users run identical firmware, and probably the hardware composition in nominally same model also varies over time. Not sure if everyone can easily upgrade to latest FW either.
  • Mapping tables with values not seen on a particular device are not a big problem - those data points should just not get populated in that run. Same-name points also (we can get ups.xxx from 3 places for the vendor/family, whichever is served by a particular device is used in that run).
  • "Production" (e.g. packaged) builds do not happen WITH_UNMAPPED_DATA_POINTS, those are for investigations so duplicates are also not a big deal.
  • From my reading, unless I've botched something, I've actually dropped many unmapped's which are deciphered above by either source branch, and dropped a few experimental's. The few duplicates which do remain use very different modifiers in mapping (e.g. report the numeric reading or pass it through conversion functions). Items that were different but de-duplicated mainly concerned presence/absence of HU_* flags (static/semi-static data, etc.) or floating-point precision (0/1 digits) with generally same data type.
  • Comments can surely be updated to highlight whatever misbehaved in 2024 firmwares and was fixed by 2025's, no?

@dorner365

dorner365 commented Mar 10, 2025 •

Copy link
Copy Markdown

I just compiled NUT with the Ecoflow modification. It works, however, the "battery.runtime" is in minutes:
Screenshot 2025-03-10 004957
and does not line up with the Ecoflow app remaining time. There is a note to that effect at line 153 of ecoflow-hid.c: * FIXME: convert to seconds? */.

I converted to seconds, recompiled and now it shows the same as the app:
Screenshot 2025-03-10 010957

Here's the code that worked for me:
Insert at line 62 of ecoflow-hid.c:

/* ----------------------------------------------------------------------------- */
/* Runtime conversion function to match NUT HID driver's conversion signature  */
/* minutes -> seconds   */
/* ----------------------------------------------------------------------------- */
static const char *ecoflow_battery_runtime_conversion(double hid_value) {
    char *converted_value;
    double converted_runtime;

    /* Handle runtime values, including those over 24 hours */
    if (hid_value >= 0) {
        /* Convert minutes to seconds */
        converted_runtime = hid_value * 60.0;

        /* Allocate memory for string representation */
        converted_value = xmalloc(HU_INFOSIZE);
        snprintf(converted_value, HU_INFOSIZE, "%.0f", converted_runtime);
        return converted_value;
    }
    return NULL;
}
/* Lookup table for runtime conversion */
static info_lkp_t ecoflow_runtime_conversion_lkp[] = {
    { 0, NULL, ecoflow_battery_runtime_conversion, NULL },
    { -1, NULL, NULL, NULL }
};

and then replace line 154 with:

         { "battery.runtime", 0, 0, "UPS.PowerSummary.RunTimeToEmpty", NULL, "%.0f", HU_FLAG_QUICK_POLL, ecoflow_runtime_conversion_lkp },

@jimklimov

Copy link
Copy Markdown
Member

@dorner365 : thanks for the test and suggestions.

One thing that looks concerning to me here, are you sure the conversion is actually the correct thing to do? What is the capacity and workload on the UPS, is one-and-two-thirds of a day of runtime really more correct than ~40 minutes?

I don't know your setup, a 10kVA UPS feeding a Raspberry Pi and a WiFi router for it might fit the bill, but realistic workloads are usually in tens of minutes of protection - just enough to slide over short outages, or switch to a GenSet or other sources, or safely shut down...

@dorner365

Copy link
Copy Markdown

@jimklimov, please take a look at the screenshots and consider the following:

WhatsApp Image 2025-03-10 at 09 39 34_03768095

Screenshot 2025-03-10 090338

  • The battery.runtime has different meaning/value while the power station is ONLINE (in UPS, or pass-through mode) and when it is DISCHARGING.

  • I have Anker and Ecoflow models and their respective smartphone application, while ONLINE shows somewhere around one-and-two-thirds of a day running time. I asked them why? Why show a value instead of OnGrid, or ONLINE. Here is the answer I received from Ecoflow:
    "When the device is fully charged to 100%, the battery will stop charging, but it will continue to power the device's internal systems, such as the Battery Management System (BMS) and the cooling fan. These systems will gradually consume the battery's power, and the time listed is the duration the battery can sustain this power in its current state. However, once the battery level drops to 97%, it will automatically start charging again."

  • The current Ecoflow firmware does not return an UPS Load value. Without that you can't (even programmatically) have a Run Time value. So you get the battery.runtime instead :).

  • Working with the information at hand you can either show the values consistent with their application, or just show a value while DISCHARGING.

In any case, would it be "more correct" showing ~40 minutes runtime while ONLINE (because it's typically expected) than showing what the batery.runtime value returns?

@jimklimov

jimklimov commented Mar 10, 2025 •

Copy link
Copy Markdown
Member

In any case, would it be "more correct" showing ~40 minutes runtime while ONLINE (because it's typically expected) than showing what the batery.runtime value returns?

If you mean hard-coded, then no - the time is something shutdown scripts, admin planning, etc. can depend on, and in turn the time depends on the UPS model, its battery size (and battery age), and the workload. So there's no "one size fits all" case here.

If a value can be "guesstimated" - derived from others reasonably reliably, and/or user-provided calibration info (like runtimecal in some drivers) - then it might be useful even if not fully correct.

@dorner365

Copy link
Copy Markdown

I rest my case :)

…nversion_lkp

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
…ve differently in various tested devices [networkupstools#2837]

Signed-off-by: Jim Klimov <jimklimov+nut@gmail.com>
@jimklimov
jimklimov merged commit e9a48c9 into networkupstools:master Mar 13, 2025
@jimklimov jimklimov added the EcoFlow Issues and PRs about EcoFlow River 3, Delta 3 UPS series label Jul 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

EcoFlow Issues and PRs about EcoFlow River 3, Delta 3 UPS series enhancement need testing Code looks reasonable, but the feature would better be tested against hardware or OSes USB

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants