add epoch timestamp in IM Event #12185
Conversation
pullapprove
bot
requested review from
anush-apple,
austinh0,
balducci-apple,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chulspro,
Damian-Nordic,
electrocucaracha,
emargolis,
erjiaqing,
franck-apple,
hawk248,
holbrookt,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kpschoedel,
LuDuda,
lzgrablic02,
mrjerryjohns,
msandstedt,
mspang,
pan-apple,
sagar-apple and
saurabhst
There was a problem hiding this comment.
@yunhanw-google It looks like we're disabling UTC_TIMESTAMPS for every platform (unless I read these diffs wrong) Should we remove that code too?
Fast tracking, given there's been enough time for review.
|
Thanks! My main concern here is that this PR claims it's adding timestamp (epoch) support, but also removes other UTC timestamp support. • If we don't need UTC timestamp support, let's remove it entirely. If some platforms require it, how do we make this more abstract? If we're doing cleanup like this (which is great!) let's do this cleanup completely. |
yunhanw-google
force-pushed
the
feature/add_im_event_epoch
branch
from
b9875c1 to
9ec5bd3
Compare
Thanks! Just remove those CHIP_CONFIG_EVENT_LOGGING_UTC_TIMESTAMPS for platform which currently don't use it. Also update the PR description, "Before this PR there is no epoch timestamp support for all platform, CHIP_CONFIG_EVENT_LOGGING_UTC_TIMESTAMPS has not yet been used. This PR add Epoch timestamp in IM event for freertos platform, and create abstract API in system/Clock.h" |
|
PR #12185: Size comparison from 46fbb06 to 9ec5bd3 Increases (37 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (4 builds for nrfconnect)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
|
Per discussion with @woody-apple , would file several tickets on epoch timestamp for different platform |
Problem
--For IM Events, it needs support either system timestamp or utc timestamp by spec, platform would pick up either one kind timestamp for IM event logging. which means some platfrom would not have utc event logging. Before this PR, CHIP_CONFIG_EVENT_LOGGING_UTC_TIMESTAMPS flag is useless. utc im timestamp is disabled by all example application currently, CHIP_CONFIG_EVENT_LOGGING_UTC_TIMESTAMPS = 0 is at default configuration
this PR pick up freertos platform to enable this utc timestamp feature for IM event for reference where it has #if CHIP_CONFIG_EVENT_LOGGING_UTC_TIMESTAMPS & CHIP_SYSTEM_CONFIG_PLATFORM_PROVIDES_TIME set, for other platform, if they wanna use utc timestamp for IM event, they can refer the freertos implementation
--Remove systemTimestamp's system prefix to suite for both usage for epoch and system.
--Create abstract API GetClock_RealTime, GetClock_RealTimeMS, SetClock_RealTime IN system/Clock.h, implemented it in freertos platform
Change overview
See above
Testing
Compilation and presubmit tests.