search

victronenergy / venus

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

Generator start/stop alarms: add watching the Remote start mode of genset #1274

Closed mpvader closed 3 months ago

mpvader commented 4 months ago

Todo

To have automatically starting working correctly, there are two conditions:

We have an alarm feature, that monitors if the automatically starting and stopping of the generator works correctly. See below screenshot for the setting meant here.

Right now that only monitors the first, not the second. And monitoring the Remote start setting is quite wanted, so in the field its often disabled for maintenance for example, and then forgotten to enable it again. So this issue is about making it monitor also the second one.

We'll make a seperate alarm for this, so there will be two alarms:

  1. Alarm: Auto start/stop on the GX is disabled.
  2. Alarm: Remote start mode in the genset is disabled

Note that we're not adding a second user setting for this. There is one user setting, and the GX then monitors both paths.

Details:

image

ReinvdZee commented 4 months ago

I'm trying to get the expected functionality straight, do I understand this correctly (@mpvader)?

I'm asking because the alarm evaluation function was modified (@izak) to look at /RemoteStartModeEnabled, but its behavior is not what I'd expect.

Currently, an alarm will only be triggered when GX Auto start/stop is enabled AND the genset remote start mode is disabled.

izak commented 4 months ago

@ReinvdZee We only changed the name of the path, not any of the functionality, so behaviour should have remained the same. The function is supposed to remind you to turn autostart back on, in case you turned it off because you were working on the genset. For safety, a service man will always turn autostart off before working on the machine.

It should warn you, after a timeout, at least if you forgot to re-enable autostart on the panel (because that requires sending someone out back to the site again). That is what I recall at least. @jepefe originally designed this and will know better.

mpvader commented 4 months ago

@ReinvdZee , when that, already existing, setting is enabled, then:

And also:

And both of these alarms have a different text, to clearly tell the user what the problem is.

And now that I think of it, to have this work properly on VRM, both of them need to have their own path on dbus as well btw. With two states: 0 = OK and 2 = Alarm

And then we also need data attributes in VRM; Izak knows how that works.

jepefe commented 4 months ago

@ReinvdZee We only changed the name of the path, not any of the functionality, so behaviour should have remained the same. The function is supposed to remind you to turn autostart back on, in case you turned it off because you were working on the genset. For safety, a service man will always turn autostart off before working on the machine.

It should warn you, after a timeout, at least if you forgot to re-enable autostart on the panel (because that requires sending someone out back to the site again). That is what I recall at least. @jepefe originally designed this and will know better.

@izak I believe you are considering this from the perspective of generators with a panel. The functionality was originally designed for the GX generator start/stop autostart feature, not the remote panel's RemoteStartModeEnabled. However, after the change Rein referred to, this alarm mechanism has been modified to monitor the panel RemoteStartModeEnabled but not the GX autostart.

mpvader commented 4 months ago

So we have a bug in there since Q4 2023?

jepefe commented 4 months ago

Unfortunately, that appears to be the case.

mpvader commented 4 months ago

ok. That does explain some confusion that I've heard now and then.

What I'd like to be done is, aside from fixing this in venus master aka, v3.40 betas, also release a Venus OS v3.33 that fixes this bug. And then preferably include checking remote start mode as well: ie check both paths as how its discussed above.

Can you take care of that @ReinvdZee , and have Jesus review it?

Please before you start on this, consider a bit how you'll do it with regards to releases; if needed to keep it practical, its even ok for me to just force push to master of dbus_generator to remove all recent changes, ie. v1.6.12. Then do these fixes; test and release that carefully.

Thereafter re-add dcgenset service and other things again.

For now, I've removed the pending entry on the todo, to reduce work on Venus OS maintainer side.

ReinvdZee commented 4 months ago

@mpvader,

Venus v3.32 has dbus-generator v1.6.10.

Should I then branch off of commit 26ea1c.. (the 1.6.10 tag) and do the changes there?

izak commented 4 months ago

Should I then branch off of commit 26ea1c.. (the 1.6.10 tag) and do the changes there?

Yes. Make a branch that branches off at the tag that was in 3.30. I usually call it venus-3.3x according to the current stable cycle.

Then the idea is to first fix the issue on master, and then cherry-pick it across to this branch so you can easily make a patch.

Then when venus 3.33 is built, this is added as a patch to the build process and the release number (of the OE package) is incremented. Jeroen handles that.

A nice feature of github, once you have the changeset on the screen, add ".patch" to the end of the url. List this on the TODO, with the (backport candidate) tag. That makes it easy to pull a patch that can go right into the build tree.

ReinvdZee commented 4 months ago

@ReinvdZee , when that, already existing, setting is enabled, then:

About the setting "Alarm when generator is not in auto start mode", there is actually two (or more) ways to navigate to this setting, and both result in a different dbus path being set.

1: settings menu Device settings --> "Generator start/stop" --> "settings":

Screencast from 24-05-24 12:13:41.webm

This setting sets /Settings/Generator0/Alarms/AutoStartDisabled

2: Through genset menu genset --> "Auto start/stop" --> "Settings":

Screencast from 24-05-24 12:17:02.webm

This setting sets /Settings/Generator1/Alarms/AutoStartDisabled

So will the setting accessed through the genset menu be removed, or should we point it to the same dbus path? Because now, the alarm on the /RemoteStartModeEnabled will only trigger when the setting through the genset menu is enabled, because that instance of startstop is looking at the genset.

@mpvader, @jepefe

jepefe commented 4 months ago

@ReinvdZee

Via the two menus you are actually accessing to two independent instances of the generator start/stop:

/Settings/Generator0/Alarms/AutoStartDisabled applies to the Relay generator and /Settings/Generator1/Alarms/AutoStartDisabled applies to the Fischer Panda generator.

Both settings must be kept.

ReinvdZee commented 4 months ago

My plan for fixing the alarms:

Make the two separate alarms work and rename /AutoStart on a feature branch. Then merge onto master and cherry-pick only the separate alarms fix to a backporting branch (venus-v3.3x, branched off at tag v1.6.10).

The backport will then have the two alarms (and therefore fix the bug), but won't have the renaming part.

To have the new alarm actually show up, a change to venus-platform is needed as well. Same here, do changes (add 1 line here) on master and cherry-pick over to a backporting branch.

So the backport will have the two alarms and is two patches, ok? @mpvader

mpvader commented 4 months ago

hi Rein, yes, the fix can be as many patches as you want. 👍

mpvader commented 4 months ago

Here are the two alarms, really nice how they work now:

image

mpvader commented 4 months ago

I think there is still some work on vrmlogger, and actually another bug: the old alarm was never made available to VRM (!!)

Thats up for Izak. Pls do that as a separate patch @izak , and check that that patch applies to the vrmlogger version in Venus v3.32, which is vrmlogger-v2.312.

mpvader commented 4 months ago

@ReinvdZee / @izak see updated todo list at the top. And Izak pls confirm that my suspicion is correct, that we have never added this to VRM.

mpvader commented 4 months ago

@ReinvdZee can you add screenshots to this issue, of both alarms? Preferably each separate as well as the both in one like you did yesterday.

and then both gui-v1 and v2

thx

ReinvdZee commented 4 months ago

Alarms on gui-v1:

image

And on gui-v2: image