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 1 commit
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
Next Next commit
- simplify span usage 
- 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
  • Loading branch information
@fessehaeve
fessehaeve committed May 2, 2024
commit ed1bbac8060f293108f1ef7cf484e896e1d4c4c4
67 changes: 32 additions & 35 deletions src/app/clusters/time-synchronization-server/time-synchronization-server.cpp
View file
Original file line number Diff line number Diff line change
@@ -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");
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()
@@ -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");
return false;
}

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

@@ -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");
return false;
}

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

@@ -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");
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;
}
@@ -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;
}

@@ -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();
@@ -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];
@@ -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);
}
tzStore.timeZone.name.SetValue(CharSpan(tzStore.name, len));
tzStore.timeZone.name.SetValue(tempSpan);
}
else
{
@@ -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();
@@ -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];
@@ -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)
{
@@ -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)
@@ -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))
{