Suggested filter for RWSensors should be round_robin

[Ivan-L]

Issue related to

Sunsynk Home Assistant Dev Add-On

Describe the issue/bug

Currently if the user adds a sensor and does not explicitly choose a filter, the same set of filters get applied to read-only and writable sensors through the suggested_filter function. For example, for RW voltage sensors, an avg filter is used, which results in odd behaviour when the value is changed either on the inverter side or in the add-on itself in that when there is a change, the sensor value will gradually increase (with several decimal places) towards the new value but these intermediate average values are undesirable.

Expected behavior It is proposed that the suggested filter for all RW sensors is now or last. I cannot think of a use case for a RW sensor where the filter should be different. Would there be any undesirable consequences of using the now or last filters for all RW sensors?

If there is agreement on the above, I will put up a PR.

Current implementation:

def suggested_filter(sensor: Sensor) -> str:
    """Default filters."""
    if sensor.id.startswith("prog"):
        return "round_robin"
    f_id = {
        "battery_soc": "last",
        "fault": "round_robin",
        "grid_connected_status": "last",
        "overall_state": "step",
        "priority_mode": "round_robin",
        "sd_status": "step",
        "serial": "round_robin",
    }
    assert all(s in ALL_SENSORS for s in f_id)

    f_unit = {
        AMPS: "step",
        VOLT: "avg",
        WATT: "step",
        KWH: "last",
        CELSIUS: "avg",
    }
    res = f_id.get(sensor.id) or f_unit.get(sensor.unit) or "step"
    _LOGGER.debug("%s unit:%s, id:%s", res, sensor.unit, sensor.id)
    return res

Proposed change:

def suggested_filter(sensor: Sensor) -> str:
    """Default filters."""
    if isinstance(sensor, RWSensor):
        return "now" # or "last"
    f_id = {
        "battery_soc": "last",
        "fault": "round_robin",
        "grid_connected_status": "last",
        "overall_state": "step",
        "sd_status": "step",
        "serial": "round_robin",
    }
    assert all(s in ALL_SENSORS for s in f_id)

    f_unit = {
        AMPS: "step",
        VOLT: "avg",
        WATT: "step",
        KWH: "last",
        CELSIUS: "avg",
    }
    res = f_id.get(sensor.id) or f_unit.get(sensor.unit) or "step"
    _LOGGER.debug("%s unit:%s, id:%s", res, sensor.unit, sensor.id)
    return res

[Ivan-L]

Or alternatively getfilter can always return a customised filter for RWSensors, just the interval, sample and filter parameters need to be agreed upon.

Also when RWSensors are updated through the HA UI, they could force a filter update?

def getfilter(filter_def: str, sensor: Sensor) -> Filter:
    """Get a filter from a filterstring."""
    fff = filter_def.split(":")

    if fff[0] == "round_robin":
        return RoundRobinFilter(sensor=sensor)

    funcs = {"min": min, "max": max, "mean": mean, "avg": mean}
    if fff[0] in funcs:
        res = Filter(interval=10, samples=6, filter=funcs[fff[0]], sensor=sensor)
        return res

    if fff[0] == "last":

        def last(val: Sequence[int]) -> int:
            return val[-1]

        res = Filter(interval=60, samples=1, filter=last, sensor=sensor)
        return res

    if fff[0] == "now":
        res = Filter(interval=2, samples=1, filter=max, sensor=sensor)
        return res

    if fff and fff[0] != "step":
        _LOGGER.warning("Unknown filter: %s", fff)

    thr = 80
    try:
        thr = int(fff[1])
    except IndexError:
        pass
    except ValueError as err:
        _LOGGER.error("Bad threshold: %s - %s", fff[1], err)

    return SCFilter(interval=1, samples=60, filter=mean, sensor=sensor, threshold=thr)

[kellerza]

Should all config not be round_robin?

It might be that we have to speed up RR a bit (maybe read more than 1 per cycle), but reading them every second seems too much

[Ivan-L]

Looking at round_robin in detail, you are right.

The main issue is the logic for selecting the default filter for a RWSensor. Currently only if a sensor name starts with prog does it get the round_robin filter by default, or if it is explicitly added to the suggested filter dictionary. The other RWSensors do not meet those criteria and therefore end up being treated as regular sensors where their unit of measurement is used to determine the default filter. The examples would be non-prog voltage RWSensors which get the avg filter due to their unit or non-prog battery capacity / amp sensors which get the step filter by default, which ultimately also causes averaging. So switching to a filter for all RWSensors which does not do averaging at all will solve the issue.

The secondary issue which is less of a concern is that when a RWSensor value is updated, the force parameter is set to true when publishing which causes the filter to not update its list of values, therefore the updated value is missed by the filter and we are dependant on the filter's interval to pick up the new changes even though we knew about the updated value outside of the filter (the default filter is 60 seconds for round_robin). Here is the current implementation:

async def publish_sensors(sensors: List[Filter], *, force: bool = False) -> None:
    """Publish sensors."""
    for fsen in sensors:
        res = fsen.sensor.value
        if not force:
            res = fsen.update(res)
            if res is None:
                continue
        if isinstance(res, float):
            if modf(res)[0] == 0:
                res = int(res)
            else:
                res = f"{res:.2f}".rstrip("0")

        await MQTT.connect(OPT)
        await MQTT.publish(
            topic=f"{SS_TOPIC}/{OPT.sunsynk_id}/{fsen.sensor.id}", payload=str(res)
        )

Perhaps fsen.update(res) should always be done but if res is None: continue should only be done if not force? If this is done then the only scenario where the RR speed would be an issue is when the user walks to the inverter and updates a RWSensor there - it may take up to 60 seconds for the add-on to pick up the changes, but perhaps that is fine because is a the alternative is to speed up the RR filter for all cases just to make this edge case quicker which might not be the right thing to do.

Proposed change:

async def publish_sensors(sensors: List[Filter], *, force: bool = False) -> None:
    """Publish sensors."""
    for fsen in sensors:
        res = fsen.sensor.value
        res = fsen.update(res)
        if not force and res is None:
             continue
        ...

So after the above ramblings, the suggested changes are as follows:

1. Use round_robin as the suggested filter for all RWSensors. This eliminates averaging that is currently experienced.
2. Always call the update() method on a Filter when the value of an RWSensor changes via the add-on so that the Filter has the most up to date value
3. Keep the rest of the logic of the publish_sensors method the same - if force is false, we do not publish MQTT updates if the filter says we shouldn't, but if force is true, we do.
4. Don't worry about the up to 60 second delay of the RR filter for now because presumably the user would rather use the add-on to write sensor values than walk to the inverter, but even if the latter occurs we would eventually get the updated values anyway?

Maybe a last question - should we allow for anything but the default (proposed round_robin) filter to be selected by the user for RWSensors? What if a user adds prog1_capacity:avg which would not make sense?

[Ivan-L]

@kellerza I've put up #62 which implements points 1 and 2 above.