Reconnect without notification#345
Reconnect without notification#345larena1 wants to merge 1 commit intohass-agent:development-2.2.0-beta1from
Conversation
amadeo-alex
left a comment
amadeo-alex
left a comment
There was a problem hiding this comment.
had a few minutes to run down through the code and added comments on what caught my eye
| return; | ||
| } | ||
|
|
||
| if (Variables.AppSettings.MqttIgnoreGracePeriod) |
There was a problem hiding this comment.
there doesn't seem to be any reference to this configuration option within new code and there are not UI modifications proposed with this PR so implementing it as is would introduce an UI option that does nothing
| return; | ||
| } | ||
|
|
||
| if (Variables.AppSettings.MqttIgnoreGracePeriod) |
There was a problem hiding this comment.
same comment as line 159
| await Task.Delay(2000); | ||
|
|
||
| await AnnounceAvailabilityAsync(); | ||
| _isReady = true; |
There was a problem hiding this comment.
setting _isReady explicitly after awaiting for availability announcement was done on purpose as without it there was a race condition that was messing with the readiness to send the mqtt messages and sensors/commands config being published, with the proposed - lines 181 and 182 - the availability announcement is just spun of as a separate task (completed "at some point") and then _isReady set immediately - this will cause issues and cannot remain like that
There was a problem hiding this comment.
Would you prefer _isReady being set from AnnounceAvailabilityAsync then?
There was a problem hiding this comment.
Good idea, of course we'll need to test it since I remember it was quite a headache when I fiddled with it. Right after the last availability message is sent should be good.
| Variables.MainForm?.SetMqttStatus(ComponentStatus.Connecting); | ||
| _status = MqttStatus.Connecting; | ||
|
|
||
| Variables.MainForm?.ShowToolTip(Languages.MqttManager_ToolTip_Disconnected, true); |
There was a problem hiding this comment.
in the proposed code there is no logic that informs user of the connection being dropped, only that new connection failed to be established, what is the reason for this decision?
There was a problem hiding this comment.
Both handlers are being called every 5 seconds on every failed reconnect attempt and the connecting failed notification implies being disconnected so I thought it doesn't make sense to have 2 separate notifications for that or what do you think?
There was a problem hiding this comment.
Since I've "inherited" HASS.Agent codebase I'm always leaning towards leaving features/functions as they are and generally avoiding big code changes even though I see a lot of room for improvement - that will be done with v3 complete rewrite project. On top of that I don't like when software I'm using removes something just like that, without some kind of bigger reason (unless of course that specific change is strictly required for a bugfix)
I'd like to keep it if it doesn't break anything not to impact the "userspace".
There was a problem hiding this comment.
IMHO you are overthinking this but if you wish I can also post both notifications at the same time
|
Hi @amadeo-alex |
|
? |
|
I was waiting for your response to that and if you want to proceed with this general overhaul of the reconnect logic after all your feedback is addressed or if you want to take another approach on it. |
|
I'd prefer to leave both notifications, this is however not a big issue/discussion as worst case scenario the notification can be unified. The bigger issue is the UI option that would do nothing. |
2 participants
No description provided.