Add a "TimeSource" class #1153
Add a "TimeSource" class #1153
Conversation
andy31415
requested review from
anush-apple,
BroderickCarlin,
chrisdecenzo,
hawk248,
jelderton,
robszewczyk,
saurabhst and
woody-apple
as code owners
andy31415
force-pushed
the
01_add_time_source
branch
from
3884b35 to
391f2ef
Compare
|
Bloat report for job "Build Examples [ESP32]": |
|
Bloat report for job "Build Examples [nRF]": |
|
Bloat report for job "Build Examples [main-build]": |
There was a problem hiding this comment.
@andy31415, please sync up with @robszewczyk who has been in the process of open sourcing Nest's "Plaid" time manipulation framework for some time that will address this issue without propagating templates around.
Messaged Rob offline. One worry I would have is that seems to include both open source (seems time or laywer sensitive maybe ... moving from closed to open sometimes is involved) and inclusion into CHIP and that may be a long timeline. If this takes longer than a few days (which i find very likely) I would prefer to unblock transport layer and session management by including the template approach. I volunteer to switch to Plaid after that as soon as it lands as a first priority. |
It's already been fundamentally approved. Maybe you can help @robszewczyk make it happen. It's the right path here; let's make it happen. |
|
Bloat report for job "Build Examples [nRF]": |
|
Bloat report for job "Build Examples [ESP32]": |
|
Bloat report for job "Build Examples [main-build]": |
|
Bloat report for job "Build Examples [nRF]": |
|
Bloat report for job "Build Examples [ESP32]": |
Result of talking with Rob seems to be: Plaid is really suitable to integration and functional test, but has high overhead for unit tests (it requires and uses a network syncrhonization server). This PR proposes infrastructure suitable for unit tests (but not for integration tests as there is no sync), so I would still like to be considered. @gerickson what are your thoghts? |
|
Bloat report for job "Build Examples [main-build]": |
| public: | ||
| uint64_t GetCurrentMonotonicTimeMs() { return mCurrentTimeMs; } | ||
|
|
||
| void SetCurrentMonotonicTimeMs(uint64_t value) |
There was a problem hiding this comment.
Shouldn't there be a companion Set... for the kSystem that's a no-op?
There was a problem hiding this comment.
I was thinking of that, however it does not seem like the platform layer has a usable set time and I did not want to deceive users.
I could have a CHIP_ERROR return that returns error on system and success on test ... but then I wonder why bother if compiler can already catch correct vs incorrect usages.
There was a problem hiding this comment.
LMK if you think a CHIP_ERROR Set* method would be useful in all instances and I can add it. As it stands, I am leaning towards compile time error rather than runtime, but I agree that this may confuse some compilers (I was surprised it built myself .... but hey, I;ll take it)
There was a problem hiding this comment.
I'm generally for "fly what you test and test what you fly", so this seems a bit akimbo to that to begin with; however, I think, in this case, a compile-time error is probably better than a run-time error.
|
Bloat report for job "Build Examples [ESP32]": |
|
Bloat report for job "Build Examples [nRF]": |
|
Bloat report for job "Build Examples [main-build]": |
|
Bloat report for job "Build Examples [nRF]": |
|
Bloat report for job "Build Examples [ESP32]": |
|
Bloat report for job "Build Examples [main-build]": |
There was a problem hiding this comment.
/pullapprove-test
version: 3
groups:
############################################################
# Base Required Reviewers
############################################################
required-reviewers:
type: required
conditions:
- len(groups.approved.include('reviewer-groups-*')) >= 3
reviews:
required: 3
############################################################
# Reviewer Groups
############################################################
reviewer-groups-amazon:
type: optional
conditions:
- files.include('*')
reviewers:
teams:
- reviewers-amazon
reviewer-groups-apple:
type: optional
conditions:
- files.include('*')
reviewers:
teams:
- reviewers-apple
reviewer-groups-comcast:
type: optional
conditions:
- files.include('*')
reviewers:
teams:
- reviewers-comcast
reviewer-groups-google:
type: optional
conditions:
- files.include('*')
reviewers:
teams:
- reviewers-google
reviewers-samsung:
type: optional
conditions:
- files.include('*')
reviewers:
teams:
- reviewers-samsung
|
It seems that now with the pullapprove, I still get issues even though grant already approved. Does this make it harder for folks with key access to submit since their own ownership does not carry over? @woody-apple can we somehow update things for this odd situation? I cannot self-approve I think. |
Problem
System layer time source is a 'real' time source, making it hard to unit test time-dependent things. I would like to implement connection expiring logic, and controlling system time seems very useful in those instances.
Summary of Changes
Adds a templated 'time source' class which can be switched to either the test time source or keep using the system time.
This was my first idea, but it will start propagating templates around. if there are other suggestions to be able to control time for tests (maybe weak linkage or maybe our extern timesources could be swizzled at link time) please let me know. I find this should be clean enough, but if other methods are considered cleaner I would be interested to see them. This should have been a problem already in all other projects that handle time.
This is working towards #1088
fixes #1156 because I had qemu errors and I needed to see what failed.