search

kbialek / deye-inverter-mqtt

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

Missing return value #185

Closed CarstenGrohmann closed 2 hours ago

CarstenGrohmann commented 1 week ago

Describe the bug

‎DeyeSetTimeProcessor.__set_time() returns a boolean value based on the signature.

https://github.com/kbialek/deye-inverter-mqtt/blob/ef72887383e3a2e0bc7a3e4a55b5574ebcf65bb5/src/deye_set_time_processor.py#L57-L61

What do you think about returning False here?

Currently the time is set once every 5 minutes.

2024-06-26 14:28:49,220 - DeyeSetTimeProcessor - INFO - Logger time set to 2024-06-26 14:28:49.142388
2024-06-26 14:35:00,590 - DeyeSetTimeProcessor - INFO - Logger time set to 2024-06-26 14:35:00.429899
2024-06-26 14:40:49,231 - DeyeSetTimeProcessor - INFO - Logger time set to 2024-06-26 14:40:49.160204
2024-06-26 14:46:49,308 - DeyeSetTimeProcessor - INFO - Logger time set to 2024-06-26 14:46:49.238482
2024-06-26 14:52:49,243 - DeyeSetTimeProcessor - INFO - Logger time set to 2024-06-26 14:52:49.165816
2024-06-26 14:57:51,270 - DeyeSetTimeProcessor - INFO - Logger time set to 2024-06-26 14:57:51.178168
2024-06-26 15:02:55,457 - DeyeSetTimeProcessor - INFO - Logger time set to 2024-06-26 15:02:55.301019

This is not always necessary. What is your opinion on setting the logger time only once a day or when it changes from unavailable to available?

kbialek commented 1 week ago

What do you think about returning False here?

No, False indicates an unsuccessful write. Rather this code should be moved to process method

     now = datetime.now() 
     delta = now - self.__last_update_ts 
     if delta.total_seconds() < 5 * 60: 
         return

This is not always necessary. What is your opinion on setting the logger time only once a day or when it changes from unavailable to available?

The main purpose of this feature is to let the microinverter reset the energy meter on a daily basis. In the initial implementation the time was set only when the logger became online. Later, I discovered, that some microinverters ignore the set-time command despite the write was successful, so I decided to change the implementation to the current one. It greatly increases the reliability of this feature for the price of a bit more modbus traffic, which is in my opinion acceptable. So I'm against the change that you propose.

CarstenGrohmann commented 1 week ago

Sound reasonable.

Does this also reset the daily energy counter every 5 minutes? Where is this reset implemented?

kbialek commented 1 week ago

Does this also reset the daily energy counter every 5 minutes? Where is this reset implemented?

This is inverter's internal energy counter. The reset functionality is implemented inside of the inverter's firmware. When an inverter is connected to the internet then it synchronizes its internal clock with the solarman servers. This is especially important for an inverter that shuts down itself for the night. However, if you disconnect the inverter from the internet, then it's no longer capable to sync its internal clock. That is why this feature is needed. We update the clock, and the inverter resets its energy counter once it detects a date (not time) difference.

CarstenGrohmann commented 6 days ago

Thanks for the explanation. I now understand why the values are not zeroed at every set time.