Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connected genset: service & runtime counters #1315

Open
1 of 2 tasks
Tracked by #1287
ReinvdZee opened this issue Jul 16, 2024 · 27 comments
Open
1 of 2 tasks
Tracked by #1287

Connected genset: service & runtime counters #1315

ReinvdZee opened this issue Jul 16, 2024 · 27 comments
Assignees
Milestone

Comments

@ReinvdZee
Copy link
Collaborator

ReinvdZee commented Jul 16, 2024

  • 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.
  • We need to make sure that runtime + service interval as visible on VRM uses the DSE one.
@ReinvdZee ReinvdZee added this to the v3.50 milestone Jul 16, 2024
@ReinvdZee ReinvdZee self-assigned this Jul 16, 2024
@ReinvdZee ReinvdZee changed the title Connected genset: runtime counters Connected genset: service & runtime counters Jul 16, 2024
@ReinvdZee
Copy link
Collaborator Author

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
Copy link
Collaborator

@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
Copy link
Collaborator Author

@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
Copy link
Collaborator

philipptrenz commented Jul 16, 2024

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

  • We remove "Operating time" from the "Engine" page
  • We sync "Total run time" of dbus_generator with /Engine/OperatingHours from the genset controller
  • We introduce a new top-level entry, so that all runtime-related metrics are at one place
    • By this, we can move "Settings" to the top level and get rid of "Auto start/stop" → One level less in the hierarchy 🎉
  • We rename "Run time" to "Current run time" to prevent confusion
  • We show "Total run time" and "Time to service" always in full hours → Often requested by customers
    • Only for "Daily run time" and "Current run time" more resolution makes sense

@philipptrenz
Copy link
Collaborator

Mockup:
Frame 1023

@philipptrenz
Copy link
Collaborator

philipptrenz commented Jul 16, 2024

@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
Copy link
Contributor

mpvader commented Jul 16, 2024

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

@ReinvdZee
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

@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
Copy link
Collaborator

philipptrenz commented Jul 19, 2024

@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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

@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.

ReinvdZee added a commit to victronenergy/dbus_generator that referenced this issue Jul 19, 2024
@kwindrem
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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

ReinvdZee added a commit to victronenergy/dbus_generator that referenced this issue Jul 23, 2024
ReinvdZee added a commit to victronenergy/dbus_generator that referenced this issue Jul 23, 2024
@ReinvdZee
Copy link
Collaborator Author

ReinvdZee commented Jul 31, 2024

@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
Copy link
Collaborator Author

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
Copy link
Collaborator

@ReinvdZee Looks great!

ReinvdZee added a commit to victronenergy/dbus_generator that referenced this issue Jul 31, 2024
@ReinvdZee
Copy link
Collaborator Author

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

@ReinvdZee
Copy link
Collaborator Author

Changes are also documented here

@ReinvdZee
Copy link
Collaborator Author

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

@ReinvdZee
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

@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
Copy link
Collaborator

@ReinvdZee Sure, sounds good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants