Timesync improvements - part 3 #32869
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fessehaeve
force-pushed
the
pull-request/timesync-fix-27575-3
branch
from
6501048 to
715a998
Compare
pullapprove
bot
requested review from
andyg-apple,
anush-apple,
arkq,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
harimau-qirex,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple,
kkasperczyk-no,
ksperling-apple,
lazarkov,
lpbeliveau-silabs and
LuDuda
|
PR #32869: Size comparison from 9b9ed65 to f3195f5 Increases above 0.2%:
Increases (62 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
Decreases (10 builds for cyw30739, linux, nrfconnect, psoc6, telink)
Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
fessehaeve
force-pushed
the
pull-request/timesync-fix-27575-3
branch
from
4278579 to
7b4e84f
Compare
The code can be tested right now with the changes I made. However, I still don't know what to test that is not covered by cert or yaml tests. Any tips would be appreciated. |
|
PR #32869: Size comparison from bcebbc6 to 7b4e84f Increases (4 builds for cyw30739, linux, nrfconnect)
Decreases (9 builds for esp32, linux, nrfconnect, psoc6, telink)
Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
|
There are a couple of gaps in the integration tests that happen because of the static feature set of the examples. The changes made here are good - passing in the storage delegate to the init function is helpful for sure. There are some parts that just ARE hard to test with unit testing. Ex. the read client integration was tested using an orchestration script to start up the app in different modes. The other tests don't run against various combos like that. The two things that are going to really help on the test side are the mock clock and the persistent storage delegate. https://project-chip.github.io/connectedhomeip-doc/testing/unit_testing.html#utilities. Then you can test stuff like no clock, and ensure the values are properly persisted. The integration tests right now don't do a power-off, re-read of persistent attributes. Maybe they should but it's a big ask for cert tests to be constantly rebooting. For unit testing, just getting the thing building in a test scenario is the first step - that's going to tell you more about what needs to be injected than a visual inspection will. The link there has a description of how to make this work for nlUnitTest. But nlUnitTest is being deprecated, pw_unit_test is the new hotness. See https://github.com/project-chip/connectedhomeip/pull/29682/files for an example. The author there promised a documentation update, but hasn't arrived yet. OTOH, it's not too old so I'm still holding out hope. Here's the proposal we had at the MM for this: Basically, everything touching the event generation subsystem or other clusters of system layer pieces gets extracted out so it can be injected, the driver layer gets pulled out (which I think you already have with the delegate) and the server parts get extracted from the cluster logic (again, mostly there, I think). |
cecille
reviewed
Apr 16, 2024
| if (tzStore.timeZone.name.HasValue()) | ||
| const TimeSyncDataProvider::TimeZoneStore & tzStore = GetTimeZone()[0]; | ||
| lastTz.offset = tzStore.timeZone.offset; | ||
| if (tzStore.timeZone.name.HasValue() && sizeof(name) >= sizeof(tzStore.name)) |
There was a problem hiding this comment.
ok, so there's a resize(?) line in GetTimeZone()
mTimeZoneObj.timeZoneList = mTimeZoneObj.timeZoneList.SubSpan(0, mTimeZoneObj.validSize);
Why not just move that re-size into where the list(s) are actually re-sized? Then the list is properly resized after the call to UpdateTimeZoneState and the getter function has no side effects. At that point, you're just directly using a class-scoped object with (I think) a class-owned buffer that won't go out of scope. And you can just directly use it as lastTz without making a copy.
Reasoning here - 1) Getters with side effects are misleading and 2) memcpy is a big bug source and makes me nervous.
If that's too much for this change, then can this at least be swapped over to use CopyCharSpanToMutableCharSpan?
There was a problem hiding this comment.
- I considered your suggestion. However, the list is resized in multiple circumstances and I believe making those changes doesn't give the code more clarity - it would even be more confusing. Since the list is a complex type, doing the resize in the getter is completely acceptable to me - i.e. it feels right that the getter is relying on the current valid size to return the valid item.
- will be replaced as suggested
There was a problem hiding this comment.
why would you need to re-size in multiple places if you move the resize into the update?
src/app/clusters/time-synchronization-server/time-synchronization-server.cpp
Show resolved
Hide resolved
| @@ -647,7 +648,9 @@ CHIP_ERROR TimeSynchronizationServer::LoadTimeZone() | |||
| CHIP_ERROR TimeSynchronizationServer::ClearTimeZone() | |||
| { | |||
| InitTimeZone(); | |||
| return mTimeSyncDataProvider.StoreTimeZone(GetTimeZone()); | |||
| ReturnErrorOnFailure(mTimeSyncDataProvider.StoreTimeZone(GetTimeZone())); | |||
| emitTimeZoneStatusEvent(GetDelegate()->GetEndpoint()); | |||
There was a problem hiding this comment.
I think the only place this function gets called right now is on Init() if the time zone fails to load. Is the intent to have that emit an event there? That seems odd. If that IS the intent, would it make more sense to move that up into the init?
This is a public method - can the app call this? Under what circumstances? If the intent is to allow this to be called by the application code, then it should probably push this though the SetTimeZone function so it can check if the current time zone is set.
There was a problem hiding this comment.
Agree that emitting event during init is odd. Will remove it.
My intention was that apps could decide to clear time zone for whatever reason. SetTimeZone can only be helpful if the app has the means to get a valid time zone setting without using timesync.
- simplify redundant code with loading TZ and DST
- changed mechanism by which time zone and dst tables are maintained - event generation is now part of maintaining time zone and dst tables. In some cases, event generation is handled by the caller of the functions that are updating the tables - updated test that is checking validity of dst offset items
- improved time zone name copying buffer size comparison
suitable for unit testing
- do not emit TimeZoneStatus event during init - use raw list instead of getter
fessehaeve
force-pushed
the
pull-request/timesync-fix-27575-3
branch
from
1979551 to
3fc8487
Compare
|
PR #32869: Size comparison from f6b3594 to 3fc8487 Increases (9 builds for cyw30739, esp32, linux, nrfconnect, telink)
Decreases (7 builds for efr32, esp32, nrfconnect, psoc6)
Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
|
This is a 6-month old PR that has merge conflicts. Closing as stale. |
|
We still need to do this; it has been blocked on review from me because I could not figure out a sane way to review it, but I will get past that. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes for issues form #27575 :