From ed1bbac8060f293108f1ef7cf484e896e1d4c4c4 Mon Sep 17 00:00:00 2001 From: Fesseha Date: Thu, 4 Apr 2024 13:45:14 +0200 Subject: [PATCH 1/3] - 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 --- .../time-synchronization-server.cpp | 67 +++++++++---------- 1 file changed, 32 insertions(+), 35 deletions(-) diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp index fda3bb50d7fda0..fe7195b5d8e17c 100644 --- a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp +++ b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp @@ -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(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()) + { + 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()) + { + 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 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 && CHIP_NO_ERROR == TimeSynchronizationServer::Instance().SetUTCTime(commandPath.mEndpointId, utcTime, granularity, TimeSourceEnum::kAdmin)) { From de527171a44634cb46decfedfb294c9bdb4a9780 Mon Sep 17 00:00:00 2001 From: Fesseha Date: Thu, 4 Apr 2024 23:51:27 +0200 Subject: [PATCH 2/3] added error code on logs when generating events fails --- .../time-synchronization-server.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp index fe7195b5d8e17c..5cf258a7abc5de 100644 --- a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp +++ b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp @@ -145,7 +145,7 @@ static bool emitDSTTableEmptyEvent(EndpointId ep) if (CHIP_NO_ERROR != error) { - ChipLogError(Zcl, "DSTTableEmptyEvent failed"); + ChipLogError(Zcl, "DSTTableEmptyEvent failed: %" CHIP_ERROR_FORMAT, error.Format()); return false; } ChipLogProgress(Zcl, "DSTTableEmptyEvent"); @@ -165,7 +165,7 @@ static bool emitDSTStatusEvent(EndpointId ep, bool dstOffsetActive) if (CHIP_NO_ERROR != error) { - ChipLogError(Zcl, "DSTStatusEvent failed"); + ChipLogError(Zcl, "DSTStatusEvent failed: %" CHIP_ERROR_FORMAT, error.Format()); return false; } @@ -191,7 +191,7 @@ static bool emitTimeZoneStatusEvent(EndpointId ep) if (CHIP_NO_ERROR != error) { - ChipLogError(Zcl, "TimeZoneStatusEvent failed"); + ChipLogError(Zcl, "TimeZoneStatusEvent failed: %" CHIP_ERROR_FORMAT, error.Format()); return false; } @@ -208,7 +208,7 @@ static bool emitTimeFailureEvent(EndpointId ep) if (CHIP_NO_ERROR != error) { - ChipLogError(Zcl, "TimeFailureEvent failed"); + ChipLogError(Zcl, "TimeFailureEvent failed: %" CHIP_ERROR_FORMAT, error.Format()); return false; } From 283b174f48912d2fca29c458c6bc6a6b1fd1b9e2 Mon Sep 17 00:00:00 2001 From: Fesseha Date: Fri, 5 Apr 2024 10:01:23 +0200 Subject: [PATCH 3/3] fix error code - other errors in this file will be fixed in another PR --- .../time-synchronization-server/time-synchronization-server.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp index 5cf258a7abc5de..f8fcfac64a874d 100644 --- a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp +++ b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp @@ -600,7 +600,7 @@ CHIP_ERROR TimeSynchronizationServer::SetTimeZone(const DataModel::DecodableList if (CHIP_NO_ERROR != CopyCharSpanToMutableCharSpan(newTz.name.Value(), tempSpan)) { ReturnErrorOnFailure(LoadTimeZone()); - return CHIP_IM_GLOBAL_STATUS(InvalidCommand); + return CHIP_IM_GLOBAL_STATUS(ConstraintError); } tzStore.timeZone.name.SetValue(tempSpan); }