kbialek / deye-inverter-mqtt

Reads Deye solar inverter metrics and posts them over mqtt
Apache License 2.0
215 stars 50 forks source link

Publish on change #105

Closed palto42 closed 1 year ago

palto42 commented 1 year ago

Since the Deye inverter I'm using is only updating the measurements every 5 minutes, any faster reading interval doesn't seem to make much sense. On the other hand, reading only every 5 minutes would impose the risk to miss one reading due to some intermediate connectivity issue. My idea to solve this "issue" is to read more frequently, but only publish a new MQTT message if any of the values changed. This is based on the assumption that it's extremely unlikely that all logger values are unchanged between 2 updates.

I have already implemented a solution for testing this idea in my branch palto42/deye-inverter-mqtt/publish_on_change. It implements storing the last published reading in a class variable and functions to compare a new reading with the last published. Only in case of a diff it will publish the reading and update the internal variable.

It might look a bit over-engineering a simple issue, but it avoids storing duplicate reading and also improved the data visualization (Grafana) and aggregation in my InfluxDB. With 1 min reading interval Grafana was showing a curve with steps every 5 min, now I get a smooth graph: image

If there is any interest, I can raise a PR.

kbialek commented 1 year ago

Hi @palto42

This is an interesting proposal. I took a look at your code and I have to say it is quite hacky :slightly_smiling_face: and thus hard to understand. Especially this part

        if not events[0]:  # offline
            self.__log.debug("No events received (offline)")
            return True
        if events[1:] == self.__last_observations and self.__event_updated + self.__event_expiry > time.time():
            self.__log.debug("Event data is unchanged and hasn't expired")
            return False

If I get it correctly, you rely on the fact that the events are always provided in the same order, don't you?

We could add it behind a feature flag. And imho, it would also require to start publishing the metrics messages as retained (another feature flag?), so the mqtt clients will always get the full set of metrics on subscription.

palto42 commented 1 year ago

Hi @kbialek your understanding is correct that I rely on the event order, as it seems to be the case for my model and it would make the comparison more complicated otherwise. If you think that the order may change for subsequent messages, the comparison function would need to change.

My current implementation always publishes the full set if any value has changed.

kbialek commented 1 year ago

hi @palto42

If you think that the order may change for subsequent messages, the comparison function would need to change.

At the moment the event order is consistent for subsequent calls, but this is absolutely not guaranteed to be kept working this way in the future. If I get it right, you discard the first event, which I guess represents inverter online status. This is unfortunately very fragile approach. Please consider filtering the events using isinstance() function.

My current implementation always publishes the full set if any value has changed.

Perfect :+1:

palto42 commented 1 year ago

Hi @kbialek, thanks for your feedback! I have implemented a new compare using sets so that the order may change. The state event is not removed by type and not hard coded as the first event in the list. The new function is enabled by the env variable DEYE_PUBLISH_ON_CHANGE=true which defaults to false. I rebased the code on your version 2023.07.3.

kbialek commented 1 year ago

@palto42 Could you open a PR, so I can add my comments?

github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 30 days with no activity.