victronenergy / venus

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

dbus_generator: faulty "Generator not detected at AC-In" notification #1235

Closed ReinvdZee closed 4 months ago

ReinvdZee commented 4 months ago

A system with a generator faulty reports the "Generator not detected at AC-In" alert when the generator detects a high battery voltage.

See screenshots below and also discussion on slack

bildschirmfoto_2024-02-28_um_17 29 09_720

bildschirmfoto_2024-02-28_um_17 28 03_720

bildschirmfoto_2024-02-28_um_17 27 46_720

bildschirmfoto_2024-02-28_um_17 27 21_720

bildschirmfoto_2024-03-05_um_09 43 17_720

ReinvdZee commented 4 months ago

The NoGeneratorAtAcIn alarm is raised whenever there is no AC at the input whenever the state of dbus-generator is unequal to STOPPED, COOLDOWN or WARMUP. However, in this case, the state is set to 10: ERROR (see last screenshot) because the generator is in error, so the alarm is raised because there is no AC at the input.

This notification is only valid when the generator was running before the error was raised. So, preventing the alarm conditions from being evaluated whenever state = ERROR will not give desired behavior.

ReinvdZee commented 4 months ago

@jepefe, What would you think of the following?

The enum runningConditions contains the conditions on which the generator is started but it also contains Stopped. dbus path /RunningByConditionCode is always set to a value of this enum, so it essentially reflects the state of the generator.

We could add a condition StoppedOnError to represent the case when the generator was running and has been stopped on error. In case of error, the generator is stopped and /RunningByConditionCode is set to StoppedOnError.

If the error occurs when the generator is not running, /RunningByConditionCode was, and remains 0 (Stopped).

Next, we can use /RunningByConditioncode to only evaluate the alarm when the state = ERROR and the generator was stopped because of the error.

kwindrem commented 4 months ago

How would the system transition from StoppedOnError state to Stopped state?

What impact would StoppedOnError have on starting the generator either manually or automatically?

Would you not want to prevent start attempts (auto or manual) if an error condition exists?

I think all logic would still need to look at the error state regardless of whether the generator was running at the time of the error or not. If I recall, that's how it is handled now.

StoppedOnError as a separate state would possibly provide the abnormal reason the generator transitioned to stop but the gui-v1 reports the error status. So I'm not sure there is an advantage to a separate StoppedOnError state.

Just my two cents.

jepefe commented 4 months ago

@ReinvdZee I think that best option is to add a new alarm for these managed generator. It will triggered when start stop service can't start the generator because it is in error state.

philipptrenz commented 4 months ago

Please note that, depending on generator type, there are several types of errors. Some are only warnings, some are actual errors causing a shutdown or failing startup. This will become clearer with the new error scheme.

@ReinvdZee I think that best option is to add a new alarm for these managed generator. It will triggered when start stop service can't start the generator because it is in error state.

I don't think it's a good idea to conclude that a generator error results in an operating error. Also, we have generator integrations, which do not report errors. But otherwise I agree with @jepefe. Therefore I would suggest

jepefe commented 4 months ago

Currently the generator start stop will not start the generator if the generator is in error state so it actually results into operating error. Start/stop needs switch to the new error scheme to differentiate between errors and warnings.

Yes the idea was always to keep it.

The "Generator not detected on AC-IN" alert should not be raised at all, it should be independent from any generator error state

That might be a good idea but perhaps not necessary? AC input detection mechanism will inform both, generator not running but also a tripped circuit breaker.

For "managed" generators, we have better methods to detect if it could not be started or was shutdown: I would suggest to compare the reported generator state to the expected state of dbus_generator and raise a new alert, if it deviates for a certain amount of time – Only relevant if generator is expected to startup/run, of course

This should be handled somewhere else but not in generator start/stop. Venus can just trigger an alarm when the generator reports an error.

Additionally, a new "Generator in error state" alert could be added, which is independent of any start/stop functionality and only informs customers that the generator is in an error state

ReinvdZee commented 4 months ago

@jepefe

@ReinvdZee I think that best option is to add a new alarm for these managed generator. It will triggered when start stop service can't start the generator because it is in error state.

I agree, this is useful, but we still would need some way to prevent the NoGeneratorAtAcIn alert from being raised when the generator is not running and should not be running?

Anyway, should we transition to the new error scheme first and see how it goes from there?

jepefe commented 4 months ago

@ReinvdZee I'd fix the false NoGeneratorAtAcIn alarm issue by taking the error state into account when evaluating it, and then open a new issue to implement the new error codes scheme.

ReinvdZee commented 4 months ago

@jepefe,

So just prevent the alarm NoGeneratorAtAcIn from being evaluated whenever state = ERROR?

jepefe commented 4 months ago

Yes, that will fix one of the issues, which is raising this alarm when there is no reason for the generator to start.

The other issue that needs to be fixed is that non critical alarms prevents the generator from start. That one can be solved by implementing the new error codes scheme that differentiate between errors and warnings.

ReinvdZee commented 4 months ago

@jepefe,

Yes, that will fix one of the issues, which is raising this alarm when there is no reason for the generator to start.

It also leads to the alarm not being raised when the error occurs when the generator was running. But I suppose that's ok for now, since dbus_generator does report the generator being in an error state.

Lets fix this issue as agreed and proceed with the implementation of the new error scheme.

jepefe commented 4 months ago

@ReinvdZee You are right, then is better to keep it as it is now and then fix it by implementing the new error codes.

ReinvdZee commented 4 months ago

Agreed. For now, this issue will not be fixed. We prefer to have the user over-notified rather than under-notified. This particular issue will also be fixed when implementing the new error scheme: https://github.com/victronenergy/venus/issues/1238