Timesync improvements - part 2 #32849
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
pullapprove
bot
requested review from
andy31415,
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,
kpschoedel,
ksperling-apple,
lazarkov,
lpbeliveau-silabs,
LuDuda and
mhazley
|
PR #32849: Size comparison from e1d93fa to cffdbbd Increases (7 builds for cyw30739, esp32, nrfconnect, telink)
Decreases (9 builds for efr32, 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)
|
andy31415
reviewed
Apr 4, 2024
src/app/clusters/time-synchronization-server/time-synchronization-server.cpp
Outdated
Show resolved
Hide resolved
andy31415
reviewed
Apr 4, 2024
src/app/clusters/time-synchronization-server/time-synchronization-server.cpp
Show resolved
Hide resolved
|
PR #32849: Size comparison from e1d93fa to e5ee87b Increases (9 builds for cyw30739, esp32, linux, nrfconnect, telink)
Decreases (7 builds for efr32, esp32, linux, nrfconnect, psoc6)
Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
|
PR #32849: Size comparison from e1d93fa to 80cf879 Increases (28 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, mbed, nrfconnect, nxp, psoc6, qpg, stm32)
Decreases (2 builds for nrfconnect, psoc6)
Full report (38 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32)
|
cecille
reviewed
Apr 10, 2024
| @@ -564,6 +560,12 @@ CHIP_ERROR TimeSynchronizationServer::SetTimeZone(const DataModel::DecodableList | |||
| uint8_t i = 0; | |||
| InitTimeZone(); | |||
|
|
|||
| if (items > mTimeZoneObj.timeZoneList.size()) | |||
There was a problem hiding this comment.
Does this change mean that you're going to get changes to the time zone list even if there's an error returned?
There was a problem hiding this comment.
Since we do the load in the error case, the time zone won't be changing.
There was a problem hiding this comment.
Functionally, what does this change do then? ie, why move this down here rather than having the early return at the top?
| if (CHIP_NO_ERROR != CopyCharSpanToMutableCharSpan(newTz.name.Value(), tempSpan)) | ||
| { | ||
| ReturnErrorOnFailure(LoadTimeZone()); | ||
| return CHIP_IM_GLOBAL_STATUS(InvalidCommand); | ||
| return CHIP_IM_GLOBAL_STATUS(ConstraintError); |
There was a problem hiding this comment.
did we have a test failure here previously? If not, is there a test gap here?
There was a problem hiding this comment.
This code was not working as expected and the tests never reached this corner case. However, with the string copying mechanism changes it was able to return InvalidCommand but that was wrong - it should have been ConstraintError.
There was a problem hiding this comment.
So this happens when the tz.name is longer than the max length? Can you add a test for this?
| if (CHIP_NO_ERROR != CopyCharSpanToMutableCharSpan(newTz.name.Value(), tempSpan)) | ||
| { | ||
| ReturnErrorOnFailure(LoadTimeZone()); | ||
| return CHIP_IM_GLOBAL_STATUS(InvalidCommand); | ||
| return CHIP_IM_GLOBAL_STATUS(ConstraintError); |
There was a problem hiding this comment.
So at this point we have already wiped out the value of tzStore.name, right? Is that ok?
There was a problem hiding this comment.
We have the LoadTimeZone() to undo that. Or do you mean something else?
src/app/clusters/time-synchronization-server/time-synchronization-server.cpp
Show resolved
Hide resolved
| { | ||
| commandObj->AddStatus(commandPath, Status::InvalidCommand); | ||
| return true; | ||
| } | ||
|
|
||
| if (granularity != GranularityEnum::kNoTimeGranularity && | ||
| (currentGranularity == GranularityEnum::kNoTimeGranularity || granularity >= currentGranularity) && | ||
| if (granularity > currentGranularity && |
There was a problem hiding this comment.
So if granularity == currentGranularity we will ignore this call? Why is that? This code could use a comment explaining why the check is the way it is.
There was a problem hiding this comment.
Since we set currentGranularity to one lower than what is provided in the SetUTCTime command, if we let granularity == currentGranularity be valid then we will be lowering currentGranularity gradually. I don't think that is intended here.
What is missing from this PR is setting Granularity to one lower value which is in the last PR of this series #32869 - a bad consequence of my attempt at breaking down the initial PR.
There was a problem hiding this comment.
@bzbarsky-apple any feedback?
- encode mutablecharspan instead of creating a new charspan - encode null when GetLocalTime() fails - simplify granularity validity check - change size comparison to compare against actual buffer size instead of attribute list size macro - use unknown enum to check for invalid enums - minor logging changes
fessehaeve
force-pushed
the
pull-request/timesync-fix-27575-2
branch
from
80cf879 to
283b174
Compare
|
PR #32849: Size comparison from f6b3594 to 283b174 Increases (9 builds for cyw30739, esp32, linux, nrfconnect, telink)
Decreases (8 builds for efr32, esp32, linux, nrfconnect, psoc6)
Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
cecille
reviewed
May 7, 2024
| @@ -564,6 +560,12 @@ CHIP_ERROR TimeSynchronizationServer::SetTimeZone(const DataModel::DecodableList | |||
| uint8_t i = 0; | |||
| InitTimeZone(); | |||
|
|
|||
| if (items > mTimeZoneObj.timeZoneList.size()) | |||
There was a problem hiding this comment.
Functionally, what does this change do then? ie, why move this down here rather than having the early return at the top?
| if (CHIP_NO_ERROR != CopyCharSpanToMutableCharSpan(newTz.name.Value(), tempSpan)) | ||
| { | ||
| ReturnErrorOnFailure(LoadTimeZone()); | ||
| return CHIP_IM_GLOBAL_STATUS(InvalidCommand); | ||
| return CHIP_IM_GLOBAL_STATUS(ConstraintError); |
There was a problem hiding this comment.
So this happens when the tz.name is longer than the max length? Can you add a test for this?
| @@ -679,6 +670,12 @@ CHIP_ERROR TimeSynchronizationServer::SetDSTOffset(const DataModel::DecodableLis | |||
| size_t i = 0; | |||
| InitDSTOffset(); | |||
|
|
|||
| if (items > mDstOffsetObj.dstOffsetList.size()) | |||
There was a problem hiding this comment.
same Q here - what's the purpose of moving this down and then un-doing the changes made up until this point rather than having the early return at the top?
There was a problem hiding this comment.
We used the macro of the size element for size checking in the past. Changed it here because of a feedback on the original PR. The size check moved down there because the Init* functions reset the size to default capacity and during operation the size changes because we pop the expired elements from the list. So we can't do this check before init* is called.
We already have a test for the long name, not a cert test, and it used to pass because we never hit this part of the code. It started failing when the code was fixed and the test failed because it expected ConstraintError and not InvalidCommand and hence this change.
There was a problem hiding this comment.
OK, I get how that happened. It's definitely more complex. I suppose it's a more direct comparison, but messing with the size of the list, to check the incoming against the new size of the list, to then back out the changes seems odd.
@bzbarsky-apple - original comment was from you - WDYT here?
Does mDstOffsetObj always start from the first element of the mDST array?
There was a problem hiding this comment.
also where is the long-name test that was previously failing? Is that running in the CI?
|
PR seems to have no author feedback on comments in a while now. Assuming stale and closing. |
|
This still needs to happen; this is addressing previous review issues... |
|
PR #32849: Size comparison from 7287041 to 283b174 Full report (81 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
|
PR #32849: Size comparison from f2b362a to d3a3ae1 Full report (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
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 :