time sync notification #1442
time sync notification #1442
Conversation
probot-esphome
bot
added
core
has-tests
integration: ds1307
integration: time
new-integration
labels
|
Hey there @OttoWinter, mind taking a look at this pull request as its been labeled with an integration ( |
badbadc0ffee
force-pushed
the
on_time_sync
branch
from
fc7cb89
to
7f417cf
Compare
|
this is a follow-up to #1441 |
|
Would it make sense to have the on_time_sync be part of the base time component, so that all time sources fire this event? |
|
Ah, I think this is exactly what was done in this PR. I just was confused by the extra changes for DS1307 |
Right - that's just because I based the branch on the one from the previous PR. Once that one is merged, this one will look a bit cleaner... |
badbadc0ffee
force-pushed
the
on_time_sync
branch
from
7f417cf
to
6d8cf84
Compare
|
Is SNTP is forgotten here? |
It should not. The automation should work for all time sources, I don't see a reason why it shouldn't work with SNTP. |
|
Are you sure? I don't see a call to the base method from sntp |
And you are right! There was none. I was fooled because sntp logged the exact same debug output "Synchronized time: [...]" |
| @@ -54,6 +54,7 @@ void SNTPComponent::loop() { | |||
| char buf[128]; | |||
| time.strftime(buf, sizeof(buf), "%c"); | |||
| ESP_LOGD(TAG, "Synchronized time: %s", buf); | |||
| this->time_sync_callback_.call(); | |||
There was a problem hiding this comment.
This is going to report false positives, i.e. if the clock is actually synchronized because of e.g. home assistant, there is no way this thing is going to realize that.
This code makes sense if this is the only use of a time component in your whole yaml, but as your intention is to have many I'm pointing this out.
There was a problem hiding this comment.
Your remark is totally valid. but I am fine with this: the 'false positive' in this case is OK.
It may lead to a double write to the RTC in a set-up with DS1307, HA and SNTP enabled for the first synchronization if that happens to be from the HA. If I don't get things wrong, after the initial sync, the SNTP will never sync again, so this is really not a dramatic thing.
Actually, the SNTP component does not care if its own library is responsible for the sync. Independent of this PR, it would write a duplicate "Synchronized time: ..."-log in a configuration with HA and SNTP even it the initial sync was from HA. But as said: I'm OK with that.
There was a problem hiding this comment.
I'm ok with that. I'm adding a quick note in the docs so people does not believe is a bug
* add on_time_sync trigger * cleanup lint * fix review remark (sntp didn't trigger callbacks)
* add on_time_sync trigger * cleanup lint * fix review remark (sntp didn't trigger callbacks)
Description:
RFC: is this a valid approach to get notified of time synchronizations that are initiated by other clock sources?
Checklist:
tests/folder).If user exposed functionality or configuration variables are added/changed: