Migrate Aurora_ABB_Powerone to DataUpdateCoordinator #72363
Conversation
davet2001
force-pushed
the
aurora_dataupdatecoordinator
branch
from
245b3cb
to
9c1aadf
Compare
davet2001
force-pushed
the
aurora_dataupdatecoordinator
branch
from
5bba16d
to
9b24350
Compare
davet2001
force-pushed
the
aurora_dataupdatecoordinator
branch
from
cfc384d
to
2d4e37f
Compare
|
|
||
| except AuroraTimeoutError: | ||
| self.data = {} | ||
| self.available = False |
There was a problem hiding this comment.
If you migrate to using CoordinatorEntity as a base class you don't need to worry about available anymore.
raise UpdateFailed if something goes wrong and everything will automatically go unavailable
There was a problem hiding this comment.
I have done some experiments with this, and I'm not happy with the result when raising UpdateFailed.
The problem is that UpdateFailed logs an error every time, but going offline during the night is not an error. I don't want to fill the user's logs with errors that are impossible to prevent. By explicitly detecting online/offline I can manage what generates an error and also only show log messages when the device comes back online, even though it is trying 100s of times in the night.
There was a problem hiding this comment.
Maybe it changed in all those months, but I just checked the code of the DataUpdateCoordinator and there is a mechanism for this:
except UpdateFailed as err:
self.last_exception = err
if self.last_update_success:
if log_failures:
self.logger.error("Error fetching %s data: %s", self.name, err)
self.last_update_success = FalseWhich is doing exactly what you're doing right now
There was a problem hiding this comment.
I get what you are saying, but I'm trying to do something very specific, which is to catch the case of a timeout error, then display a single warning message on the first instance.
I think the automatic route only gives me the possibility to suppress all failure logging which isn't what I want.
In addition, this particular device is sometimes unreliable particularly in low light, so I'm planning a follow up PR that does some auto-retrying, see davet2001@f745523 and #82983
There was a problem hiding this comment.
I am not talking about the log_failures but about the last_update_success. If you trigger an UpdateFailed, it will log an error, and then it will set last_update_success to False, which is checked before logging.
So in practice, if UpdateFailed is raised 5 times, 1 error is written. It won't log it again, until it actually does an update without a raised error (so it succeeded).
There was a problem hiding this comment.
Yes, they are doing very similar things. I did a quick experiment, changing the code to raise UpdateFailed on timeout. It works as you describe. But here is the log output:
Existing code:
2023-08-25 11:30:04.467 WARNING (SyncWorker_2) [homeassistant.components.aurora_abb_powerone] Communication with aurora_abb_powerone lost
2023-08-25 11:35:06.608 INFO (SyncWorker_4) [homeassistant.components.aurora_abb_powerone] Communication with aurora_abb_powerone back online
Using UpdateFailed to manage offline/online messages:
2023-08-25 11:26:35.351 ERROR (MainThread) [homeassistant.components.aurora_abb_powerone] Error fetching aurora_abb_powerone data: No response after 3 tries
2023-08-25 11:28:13.427 INFO (MainThread) [homeassistant.components.aurora_abb_powerone] Fetching aurora_abb_powerone data recovered
They are almost identical, but the key point: going offline due to sunset is not an error. So I want a way of this occurring without raising anything red in the logs (only info/warning). I can't see any other way of doing this.
There was a problem hiding this comment.
Okay you got my point, now I know we're on the same wavelength.
Is there a way in the code you can differentiate on if it's because of a failure or if it's because of sunset?
There was a problem hiding this comment.
Unfortunately none that I know of, unplugging the cable behaves the same as sunset (both cause a timeout).
There was a problem hiding this comment.
Right, I still think this is a difficult situation. imo, posting something in the logs doesn't have to be seen as something bad, just an FYI. I think we can just use the normal way like every integration does, but just have to teach the user a bit more about how it works via the documentation or the error raised. You could edit the raised error like, "Lost connection to inverter, could it be dark?". We have way more integrations that just log every time the service is responds with a 500 while the next call just succeeds.
I think it would be good to have a third opinion on this matter. Like I get where you come from, but I am just wondering what's the right solution here.
There was a problem hiding this comment.
@joostlek I don't agree. If there are warning or error prints users will create issues and complain about it. In this case, what could be considered is to log with info level since debug prints are usually very spammy.
I think the PR is good to go as is though, adjusting to info level can be done in a follow-up.
davet2001
force-pushed
the
aurora_dataupdatecoordinator
branch
from
d1ede77
to
35119c9
Compare
davet2001
force-pushed
the
aurora_dataupdatecoordinator
branch
2 times, most recently
from
b438de0
to
32cbb19
Compare
Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
|
|
||
| except AuroraTimeoutError: | ||
| self.data = {} | ||
| self.available = False |
There was a problem hiding this comment.
Right, I still think this is a difficult situation. imo, posting something in the logs doesn't have to be seen as something bad, just an FYI. I think we can just use the normal way like every integration does, but just have to teach the user a bit more about how it works via the documentation or the error raised. You could edit the raised error like, "Lost connection to inverter, could it be dark?". We have way more integrations that just log every time the service is responds with a 500 while the next call just succeeds.
I think it would be good to have a third opinion on this matter. Like I get where you come from, but I am just wondering what's the right solution here.
joostlek
added
the
second-opinion-wanted
There was a problem hiding this comment.
Looks good to me. Let's get this year+ PR over the finish line. Thanks for hanging in there @davet2001
|
@joostlek Thanks for the review. Points should all be addressed now. Regarding the logs, I do feel that logs and warnings should only occur if something is happening that shouldn't happen. If nothing else, unnecessary logs distract when debugging a real problem. e.g. my current setup is generating the following error approx twice per second continuously. I treat this as something that should be fixed rather than lived with. But definitely worth another opinion on this. |
Breaking change
Should be no breaking changes.
Proposed change
The existing integration makes separate calls to the serial port for each sensor on demand. This PR moves those to a DataUpdateCoordinator in line with other integrations. This should improve responsiveness of the integration.
Update 13/5/2023:
It seems that problem #92985 (comms failure) beginning in 2023.5.x can be resolved by this change.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: