victronenergy / venus

Victron Energy Unix/Linux OS
https://github.com/victronenergy/venus/wiki
568 stars 70 forks source link

Connected genset: service & runtime counters #1315

Open ReinvdZee opened 1 month ago

ReinvdZee commented 1 month ago
ReinvdZee commented 1 month ago

There are now two runtime counters in the menu, (and on VRM etc.). The one by the genset panel is the one we want - the counter by dbus-generator needs to be hidden or otherwise not used.

The "Engine" menu of the connected genset shows the operating time as reported by the genset controller: image

The "Auto start/stop" menu shows "Run time" "Total run time" and "Daily run time" metrics: image

When the operating time of the genset is available, the "Runtime" and "Total runtime" fields should be hidden because the values can be different (and then the operating time as reported by the genset is the most useful).

"Daily runtime" contains historical data of the start/stop service: image

AFAIK, connected gensets do not report this data (maybe some do, not sure), so it may still be useful for the user to display this, even though its data from a different source (startstop instead of genset). What do we do with this @mpvader ?

@philipptrenz Do you know if there are connected gensets which do report this? And does dbus-modbus-client also fetch this data? If so, we can also display historical data from the genset if present, or display it from startstop if not.

philipptrenz commented 1 month ago

@ReinvdZee There's one more time-related page under "Auto start/stop" > "Settings" > "Run time and service":

Bildschirmfoto 2024-07-16 um 16 53 44
ReinvdZee commented 1 month ago

@philipptrenz Oh thanks, We should definitely hide the "Generator total runtime" field there. And depending on what we do with the daily run time counter, the top one can be hidden as well.

philipptrenz commented 1 month ago

In earlier discussions we came to the point, that VRM should always have only one point of truth. Therefore, I would recommend:

philipptrenz commented 1 month ago

Mockup: Frame 1023

philipptrenz commented 1 month ago

@philipptrenz Do you know if there are connected gensets which do report this? And does dbus-modbus-client also fetch this data? If so, we can also display historical data from the genset if present, or display it from startstop if not.

Maybe some do, but definitely not all. Therefore, I wouldn't rely on that. I think it's fine if "Current run time" and "Daily run time" are counted by dbus_generator, as long as "Total run time" is accurate and in sync with the reading from the genset controller.

mpvader commented 1 month ago

My two cents: daily run time counters are very useful; otherwise it seems all to be going in the good direction

ReinvdZee commented 1 month ago

@mpvader

My two cents: daily run time counters are very useful; otherwise it seems all to be going in the good direction

Thanks, I agree. We'll keep the daily runtime counter and the current runtime counter. Both use dbus_generator as source. The total runtime will use the genset as source if possible.

@philipptrenz Thanks for you mockup, looks good!

Maybe some do, but definitely not all. Therefore, I wouldn't rely on that. I think it's fine if "Current run time" and "Daily run time" are counted by dbus_generator, as long as "Total run time" is accurate and in sync with the reading from the genset controller.

Ok, we'll just use the dbus_generator counters for this then.

ReinvdZee commented 1 month ago

@philipptrenz See screenshot:

image

The "Generator service interval" is not present in your mockup, would you expect it somewhere else? In settings perhaps? I think that if you're able to reset the service timer here, you should also be able to configure it.

philipptrenz commented 1 month ago

@ReinvdZee Yes, my idea was to have it in the settings. And actually, it's the same. I'm not sure how often service intervals have to be adjusted. Initially I thought the interval adjustment and resetting it should be located at the same place, but VRM also has the interval counter and reset option at the same place, so I moved only the reset functionality over to the counter to be consistent. But I'm not too set in my ways.

Bildschirmfoto 2024-07-19 um 15 49 28 Bildschirmfoto 2024-07-19 um 15 51 06
ReinvdZee commented 1 month ago

@philipptrenz

Yes, my idea was to have it in the settings.

Ok, I can agree with that. Settings will then look like this: image

ReinvdZee commented 1 month ago

@philipptrenz

We rename "Run time" to "Current run time" to prevent confusion

For connected gensets, this field is currently not displayed. Its on PageGenerator.qml, only used for the Relay controlled generator start/stop in the main settings menu.

Would the Run time field be useful to show in the connected genset page? It will then hold the run time of the start/stop instance. This value is also used in the daily runtime, which is displayed for connected gensets.

kwindrem commented 1 month ago

Resetting the service time is done frequently, say when the oil is changed. The service interval is typically set on generator install and remains unchanged. It's a small point and having both in the same menu isn't a big deal.

kwindrem commented 1 month ago

You might take a look at the generator overview I did for GuiMods for some ideas. I display three times in the run times area: daily, accumulated and time till next service (if service interval is not 0). The current run time is shown in status along with why the run started. If it's a timed manual run the time till end is also shown.

For the main overview page, the generator tile should show if the generator is running or not. Maybe showing if it was manually started would be valuable to help avoid a generator running forever.

philipptrenz commented 1 month ago

Thanks for your inputย @kwindrem. Looks like we're slowly catching up ๐Ÿ˜‰

@ReinvdZee I just checked in gui-v2 and the current run time is already available from the generator tile as well as the manual generator control. I think this is sufficient. Otherwise, I would also see the current run time next to the generator status and errors instead of the "Run time and service", like Kevin described.

ReinvdZee commented 1 month ago

@philipptrenz @kwindrem thanks for your input. I'll add the current runtime to the run time and service menu for completeness.

ReinvdZee commented 1 month ago

@philipptrenz

@ReinvdZee Yes, my idea was to have it in the settings.

I've moved the service interval setting over the the run time & service menu because it directs the user to the reset button after setting it, so it makes sense to have it in the same menu.

Screenshot from 2024-07-31 10-38-39

I also modified the toast message when a value of 0 is entered: image

ReinvdZee commented 1 month ago

And I also added "Current run time" to the main menu of the genset page:

image

It is hidden when startstop is not "Running", "Warm-up" or "Cooldown".

philipptrenz commented 1 month ago

@ReinvdZee Looks great!

ReinvdZee commented 1 month ago

The "Run time and service" menu of the relay-controlled genset is also moved to the toplevel (was part of "Settings"): image

ReinvdZee commented 1 month ago

Changes are also documented here

ReinvdZee commented 3 weeks ago

VRM does not display the correct runtime yet. VRM always looks at startstop instance 0 for its data. This is a known issue

ReinvdZee commented 2 weeks ago

I came across the menu entry "Accumulated running time since last test run". It was in the setting menu, I relocated it to the "Run time and service menu":

image

ReinvdZee commented 2 weeks ago

We moved the "Settings" submenu of auto start/stop to the toplevel. However, for DC gensets, there is already a settings menu which holds settings related to the charging behavior:

image image

Since the two settings menu's are meant to configure completely different things; I'd suggest to rename the settings menu for the DC genset to: "DC genset settings"

@philipptrenz, @martinbosma, agreed?

philipptrenz commented 2 weeks ago

How about moving the DC-related genset settings as a, for example "DC charging parameters" named, entry into the general "Settings" menu โ€“ hidden for non-DC gensets, of course?

ReinvdZee commented 2 weeks ago

@philipptrenz Sure we can do that. It does introduce a level of hierarchy, but keeps the top-level cleaner. The same menu is also used for relay-controlled gensets, but the settings can be hidden there as well.

martinbosma commented 1 week ago

@ReinvdZee Sure, sounds good to me