Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Timesync improvements - part 2 #32849

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Timesync improvements - part 2 #32849

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ed1bbac
- simplify span usage
fessehaeve Apr 4, 2024
de52717
added error code on logs when generating events fails
fessehaeve Apr 4, 2024
283b174
fix error code - other errors in this file will be fixed in another PR
fessehaeve Apr 5, 2024
d3a3ae1
Merge branch 'master' into pull-request/timesync-fix-27575-2
fessehaeve Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 33 additions & 36 deletions src/app/clusters/time-synchronization-server/time-synchronization-server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,10 @@ static bool emitDSTTableEmptyEvent(EndpointId ep)

if (CHIP_NO_ERROR != error)
{
ChipLogError(Zcl, "Unable to emit DSTTableEmpty event [ep=%d]", ep);
ChipLogError(Zcl, "DSTTableEmptyEvent failed: %" CHIP_ERROR_FORMAT, error.Format());
return false;
}
ChipLogProgress(Zcl, "Emit DSTTableEmpty event [ep=%d]", ep);
ChipLogProgress(Zcl, "DSTTableEmptyEvent");

// TODO: re-schedule event for after min 1hr https://github.com/project-chip/connectedhomeip/issues/27200
// delegate->scheduleDSTTableEmptyEvent()
Expand All @@ -165,11 +165,11 @@ static bool emitDSTStatusEvent(EndpointId ep, bool dstOffsetActive)

if (CHIP_NO_ERROR != error)
{
ChipLogError(Zcl, "Unable to emit DSTStatus event [ep=%d]", ep);
ChipLogError(Zcl, "DSTStatusEvent failed: %" CHIP_ERROR_FORMAT, error.Format());
return false;
}

ChipLogProgress(Zcl, "Emit DSTStatus event [ep=%d]", ep);
ChipLogProgress(Zcl, "DSTStatusEvent active: %d", dstOffsetActive);
return true;
}

Expand All @@ -191,11 +191,11 @@ static bool emitTimeZoneStatusEvent(EndpointId ep)

if (CHIP_NO_ERROR != error)
{
ChipLogError(Zcl, "Unable to emit TimeZoneStatus event [ep=%d]", ep);
ChipLogError(Zcl, "TimeZoneStatusEvent failed: %" CHIP_ERROR_FORMAT, error.Format());
return false;
}

ChipLogProgress(Zcl, "Emit TimeZoneStatus event [ep=%d]", ep);
ChipLogProgress(Zcl, "TimeZoneStatusEvent offset: %d", static_cast<int>(tz.offset));
return true;
}

Expand All @@ -208,13 +208,13 @@ static bool emitTimeFailureEvent(EndpointId ep)

if (CHIP_NO_ERROR != error)
{
ChipLogError(Zcl, "Unable to emit TimeFailure event [ep=%d]", ep);
ChipLogError(Zcl, "TimeFailureEvent failed: %" CHIP_ERROR_FORMAT, error.Format());
return false;
}

// TODO: re-schedule event for after min 1hr if no time is still available
// https://github.com/project-chip/connectedhomeip/issues/27200
ChipLogProgress(Zcl, "Emit TimeFailure event [ep=%d]", ep);
ChipLogProgress(Zcl, "TimeFailureEvent");
GetDelegate()->NotifyTimeFailure();
return true;
}
Expand All @@ -228,13 +228,13 @@ static bool emitMissingTrustedTimeSourceEvent(EndpointId ep)

if (CHIP_NO_ERROR != error)
{
ChipLogError(Zcl, "Unable to emit MissingTrustedTimeSource event [ep=%d]", ep);
ChipLogError(Zcl, "Unable to emit MissingTrustedTimeSource event");
return false;
}

// TODO: re-schedule event for after min 1hr if TTS is null or cannot be reached
// https://github.com/project-chip/connectedhomeip/issues/27200
ChipLogProgress(Zcl, "Emit MissingTrustedTimeSource event [ep=%d]", ep);
ChipLogProgress(Zcl, "Emit MissingTrustedTimeSource event");
return true;
}

Expand Down Expand Up @@ -540,10 +540,6 @@ CHIP_ERROR TimeSynchronizationServer::SetTimeZone(const DataModel::DecodableList
size_t items;
VerifyOrReturnError(CHIP_NO_ERROR == tzL.ComputeSize(&items), CHIP_IM_GLOBAL_STATUS(InvalidCommand));

if (items > CHIP_CONFIG_TIME_ZONE_LIST_MAX_SIZE)
{
return CHIP_ERROR_BUFFER_TOO_SMALL;
}
if (items == 0)
{
return ClearTimeZone();
Expand All @@ -568,6 +564,12 @@ CHIP_ERROR TimeSynchronizationServer::SetTimeZone(const DataModel::DecodableList
uint8_t i = 0;
InitTimeZone();

if (items > mTimeZoneObj.timeZoneList.size())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this change mean that you're going to get changes to the time zone list even if there's an error returned?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we do the load in the error case, the time zone won't be changing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally, what does this change do then? ie, why move this down here rather than having the early return at the top?

{
LoadTimeZone();
return CHIP_ERROR_BUFFER_TOO_SMALL;
}

while (newTzL.Next())
{
auto & tzStore = mTimeZoneObj.timeZoneList[i];
Expand All @@ -593,20 +595,14 @@ CHIP_ERROR TimeSynchronizationServer::SetTimeZone(const DataModel::DecodableList
tzStore.timeZone.validAt = newTz.validAt;
if (newTz.name.HasValue() && newTz.name.Value().size() > 0)
{
size_t len = newTz.name.Value().size();
if (len > sizeof(tzStore.name))
{
ReturnErrorOnFailure(LoadTimeZone());
return CHIP_ERROR_IM_MALFORMED_COMMAND_DATA_IB;
}
memset(tzStore.name, 0, sizeof(tzStore.name));
chip::MutableCharSpan tempSpan(tzStore.name, len);
chip::MutableCharSpan tempSpan(tzStore.name);
if (CHIP_NO_ERROR != CopyCharSpanToMutableCharSpan(newTz.name.Value(), tempSpan))
{
ReturnErrorOnFailure(LoadTimeZone());
return CHIP_IM_GLOBAL_STATUS(InvalidCommand);
return CHIP_IM_GLOBAL_STATUS(ConstraintError);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did we have a test failure here previously? If not, is there a test gap here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this happens when the tz.name is longer than the max length? Can you add a test for this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So at this point we have already wiped out the value of tzStore.name, right? Is that ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have the LoadTimeZone() to undo that. Or do you mean something else?

}
tzStore.timeZone.name.SetValue(CharSpan(tzStore.name, len));
tzStore.timeZone.name.SetValue(tempSpan);
}
else
{
Expand Down Expand Up @@ -665,11 +661,6 @@ CHIP_ERROR TimeSynchronizationServer::SetDSTOffset(const DataModel::DecodableLis
size_t items;
VerifyOrReturnError(CHIP_NO_ERROR == dstL.ComputeSize(&items), CHIP_IM_GLOBAL_STATUS(InvalidCommand));

if (items > CHIP_CONFIG_DST_OFFSET_LIST_MAX_SIZE)
{
return CHIP_ERROR_BUFFER_TOO_SMALL;
}

if (items == 0)
{
return ClearDSTOffset();
Expand All @@ -679,6 +670,12 @@ CHIP_ERROR TimeSynchronizationServer::SetDSTOffset(const DataModel::DecodableLis
size_t i = 0;
InitDSTOffset();

if (items > mDstOffsetObj.dstOffsetList.size())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also where is the long-name test that was previously failing? Is that running in the CI?

{
LoadDSTOffset();
return CHIP_ERROR_BUFFER_TOO_SMALL;
}

while (newDstL.Next())
{
auto & dst = mDstOffsetObj.dstOffsetList[i];
Expand Down Expand Up @@ -979,7 +976,7 @@ CHIP_ERROR TimeSynchronizationAttrAccess::ReadDefaultNtp(EndpointId endpoint, At
err = TimeSynchronizationServer::Instance().GetDefaultNtp(dntp);
if (err == CHIP_NO_ERROR)
{
err = aEncoder.Encode(CharSpan(buffer, dntp.size()));
err = aEncoder.Encode(dntp);
}
else if (err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND)
{
Expand Down Expand Up @@ -1021,9 +1018,10 @@ CHIP_ERROR TimeSynchronizationAttrAccess::ReadDSTOffset(EndpointId endpoint, Att
CHIP_ERROR TimeSynchronizationAttrAccess::ReadLocalTime(EndpointId endpoint, AttributeValueEncoder & aEncoder)
{
DataModel::Nullable<uint64_t> localTime;
CHIP_ERROR err = TimeSynchronizationServer::Instance().GetLocalTime(endpoint, localTime);
err = aEncoder.Encode(localTime);
return err;
VerifyOrReturnError(CHIP_NO_ERROR == TimeSynchronizationServer::Instance().GetLocalTime(endpoint, localTime),
aEncoder.EncodeNull());
ReturnErrorOnFailure(aEncoder.Encode(localTime));
return CHIP_NO_ERROR;
}

CHIP_ERROR TimeSynchronizationAttrAccess::Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder)
Expand Down Expand Up @@ -1094,19 +1092,18 @@ bool emberAfTimeSynchronizationClusterSetUTCTimeCallback(
const auto & timeSource = commandData.timeSource;

auto currentGranularity = TimeSynchronizationServer::Instance().GetGranularity();
if (granularity < GranularityEnum::kNoTimeGranularity || granularity > GranularityEnum::kMicrosecondsGranularity)
if (granularity == GranularityEnum::kUnknownEnumValue)
{
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
}
if (timeSource.HasValue() && (timeSource.Value() < TimeSourceEnum::kNone || timeSource.Value() > TimeSourceEnum::kGnss))
if (timeSource.HasValue() && timeSource.Value() == TimeSourceEnum::kUnknownEnumValue)
{
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
}

if (granularity != GranularityEnum::kNoTimeGranularity &&
(currentGranularity == GranularityEnum::kNoTimeGranularity || granularity >= currentGranularity) &&
if (granularity > currentGranularity &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bzbarsky-apple any feedback?

CHIP_NO_ERROR ==
TimeSynchronizationServer::Instance().SetUTCTime(commandPath.mEndpointId, utcTime, granularity, TimeSourceEnum::kAdmin))
{
Expand Down
Loading