From f027069a0da93f74bd697086db5af6a7fe7cd7ae Mon Sep 17 00:00:00 2001 From: Nivi Sarkar <55898241+nivi-apple@users.noreply.github.com> Date: Fri, 26 Jul 2024 16:07:04 -0700 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Boris Zbarsky --- .../linux/include/thermostat-delegate-impl.h | 4 ++-- .../linux/thermostat-delegate-impl.cpp | 18 +++++------------- .../thermostat/linux/thermostat-manager.cpp | 2 +- .../PresetStructWithOwnedMembers.cpp | 2 +- .../thermostat-server/thermostat-server.cpp | 18 +++++++++--------- 5 files changed, 18 insertions(+), 26 deletions(-) diff --git a/examples/thermostat/linux/include/thermostat-delegate-impl.h b/examples/thermostat/linux/include/thermostat-delegate-impl.h index 49d0184f6d56e5..dfb36e1957dc26 100644 --- a/examples/thermostat/linux/include/thermostat-delegate-impl.h +++ b/examples/thermostat/linux/include/thermostat-delegate-impl.h @@ -28,7 +28,7 @@ namespace Thermostat { /** * The ThermostatDelegate class serves as the instance delegate for storing Presets related information and providing it to the * Thermostat server code. It also manages the presets attribute and provides methods to write to presets, edit presets, maintain a - * pending presets list and either commit the presets when requested or discard the changes. It also provide API's to get and set + * pending presets list and either commit the presets when requested or discard the changes. It also provides APIs to get and set * the attribute values. * */ @@ -36,7 +36,7 @@ namespace Thermostat { static constexpr uint8_t kMaxNumberOfPresetTypes = 6; // We will support only one preset of each preset type. -static constexpr uint8_t kMaxNumberOfPresetTypesOfEachType = 1; +static constexpr uint8_t kMaxNumberOfPresetsOfEachType = 1; class ThermostatDelegate : public Delegate { diff --git a/examples/thermostat/linux/thermostat-delegate-impl.cpp b/examples/thermostat/linux/thermostat-delegate-impl.cpp index 949ea26947cdaf..04dc3b63f14e84 100644 --- a/examples/thermostat/linux/thermostat-delegate-impl.cpp +++ b/examples/thermostat/linux/thermostat-delegate-impl.cpp @@ -56,7 +56,7 @@ ThermostatDelegate::ThermostatDelegate() InitializePresetTypes(); InitializePresets(); - memset(mActivePresetHandleData, 0, kPresetHandleSize); + memset(mActivePresetHandleData, 0, sizeof(mActivePresetHandleData)); mActivePresetHandleDataSize = 0; } @@ -83,7 +83,7 @@ void ThermostatDelegate::InitializePresetTypes() void ThermostatDelegate::InitializePresets() { - // Initilaize the presets with 2 built in presets - occupied and unoccupied. + // Initialize the presets with 2 built in presets - occupied and unoccupied. PresetScenarioEnum presetScenarioEnumArray[2] = { PresetScenarioEnum::kOccupied, PresetScenarioEnum::kUnoccupied }; static_assert(ArraySize(presetScenarioEnumArray) <= ArraySize(mPresets)); @@ -136,15 +136,7 @@ CHIP_ERROR ThermostatDelegate::GetPresetAtIndex(size_t index, PresetStructWithOw CHIP_ERROR ThermostatDelegate::GetActivePresetHandle(MutableByteSpan & activePresetHandle) { - if (mActivePresetHandleDataSize > 0) - { - CopySpanToMutableSpan(ByteSpan(mActivePresetHandleData, mActivePresetHandleDataSize), activePresetHandle); - } - else - { - activePresetHandle.reduce_size(0); - } - return CHIP_NO_ERROR; + return CopySpanToMutableSpan(ByteSpan(mActivePresetHandleData, mActivePresetHandleDataSize), activePresetHandle); } CHIP_ERROR ThermostatDelegate::SetActivePresetHandle(const DataModel::Nullable & newActivePresetHandle) @@ -152,7 +144,7 @@ CHIP_ERROR ThermostatDelegate::SetActivePresetHandle(const DataModel::Nullable kPresetHandleSize) + if (newActivePresetHandleSize > sizeof(mActivePresetHandleData)) { ChipLogError(NotSpecified, "Failed to set ActivePresetHandle. newActivePresetHandle size %u is larger than preset handle size %u", @@ -166,7 +158,7 @@ CHIP_ERROR ThermostatDelegate::SetActivePresetHandle(const DataModel::Nullable currentTemperature; currentTemperature.SetNull(); LocalTemperature::Get(kThermostatEndpoint, currentTemperature); - return (!currentTemperature.IsNull()) ? currentTemperature.Value() : 0; + return currentTemperature.ValueOr(0); } int16_t ThermostatManager::GetCurrentHeatingSetPoint() diff --git a/src/app/clusters/thermostat-server/PresetStructWithOwnedMembers.cpp b/src/app/clusters/thermostat-server/PresetStructWithOwnedMembers.cpp index 19c40359a1c4c1..86f354192540e6 100644 --- a/src/app/clusters/thermostat-server/PresetStructWithOwnedMembers.cpp +++ b/src/app/clusters/thermostat-server/PresetStructWithOwnedMembers.cpp @@ -86,7 +86,7 @@ CHIP_ERROR PresetStructWithOwnedMembers::SetName(const Optional kPresetNameSize) { ChipLogError(Zcl, "Failed to set Preset name. New name size (%u) > allowed preset name size (%u)", - static_cast(newNameSize), static_cast(kPresetNameSize)); + static_cast(newNameSize), static_cast(kPresetNameSize)); return CHIP_ERROR_NO_MEMORY; } MutableCharSpan targetSpan(presetNameData); diff --git a/src/app/clusters/thermostat-server/thermostat-server.cpp b/src/app/clusters/thermostat-server/thermostat-server.cpp index 4554d8dc2be04c..81a1b9c31ca9ad 100644 --- a/src/app/clusters/thermostat-server/thermostat-server.cpp +++ b/src/app/clusters/thermostat-server/thermostat-server.cpp @@ -92,13 +92,13 @@ Delegate * GetDelegate(EndpointId endpoint) */ bool IsValidPresetEntry(const PresetStruct::Type & preset) { - // If the presetHandle is not null, the size of the handle does not exceed 16 bytes, return true. + // Check that the preset handle is not too long. if (!preset.presetHandle.IsNull() && preset.presetHandle.Value().size() > kPresetHandleSize) { return false; } - // Return true if the preset scenario is valid, false otherwise. + // Ensure we have a valid PresetScenario. return (preset.presetScenario != PresetScenarioEnum::kUnknownEnumValue); } @@ -419,7 +419,7 @@ uint8_t CountUpdatedPresetsAfterApplyingPendingPresets(Delegate * delegate) * * @return true if the presetScenario is found, false otherwise. */ -bool FindPresetScenarioInPresetTypes(Delegate * delegate, PresetScenarioEnum presetScenario) +bool PresetScenarioExistsInPresetTypes(Delegate * delegate, PresetScenarioEnum presetScenario) { VerifyOrReturnValue(delegate != nullptr, false); @@ -786,7 +786,7 @@ CHIP_ERROR ThermostatAttrAccess::Read(const ConcreteReadAttributePath & aPath, A Delegate * delegate = GetDelegate(aPath.mEndpointId); VerifyOrReturnError(delegate != nullptr, CHIP_ERROR_INCORRECT_STATE, ChipLogError(Zcl, "Delegate is null")); - ReturnErrorOnFailure(aEncoder.Encode(gThermostatAttrAccess.GetPresetsEditable(aPath.mEndpointId))); + ReturnErrorOnFailure(aEncoder.Encode(GetPresetsEditable(aPath.mEndpointId))); } break; case ActivePresetHandle::Id: { @@ -855,7 +855,7 @@ CHIP_ERROR ThermostatAttrAccess::Write(const ConcreteDataAttributePath & aPath, VerifyOrReturnError(delegate != nullptr, CHIP_ERROR_INCORRECT_STATE, ChipLogError(Zcl, "Delegate is null")); // Presets are not editable, return INVALID_IN_STATE. - VerifyOrReturnError(gThermostatAttrAccess.GetPresetsEditable(endpoint), CHIP_IM_GLOBAL_STATUS(InvalidInState), + VerifyOrReturnError(GetPresetsEditable(endpoint), CHIP_IM_GLOBAL_STATUS(InvalidInState), ChipLogError(Zcl, "Presets are not editable")); // Check if the OriginatorScopedNodeId at the endpoint is the same as the node editing the presets, @@ -863,7 +863,7 @@ CHIP_ERROR ThermostatAttrAccess::Write(const ConcreteDataAttributePath & aPath, const Access::SubjectDescriptor subjectDescriptor = aDecoder.GetSubjectDescriptor(); ScopedNodeId scopedNodeId = ScopedNodeId(subjectDescriptor.subject, subjectDescriptor.fabricIndex); - if (gThermostatAttrAccess.GetOriginatorScopedNodeId(endpoint) != scopedNodeId) + if (GetOriginatorScopedNodeId(endpoint) != scopedNodeId) { ChipLogError(Zcl, "Another node is editing presets. Server is busy. Try again later"); return CHIP_IM_GLOBAL_STATUS(Busy); @@ -1273,7 +1273,7 @@ bool emberAfThermostatClusterSetActivePresetRequestCallback( if (err != CHIP_NO_ERROR) { ChipLogError(Zcl, "Failed to set ActivePresetHandle with error %" CHIP_ERROR_FORMAT, err.Format()); - commandObj->AddStatus(commandPath, imcode::Failure); + commandObj->AddStatus(commandPath, StatusIB(err).mStatus); return true; } @@ -1509,7 +1509,7 @@ bool emberAfThermostatClusterCommitPresetsSchedulesRequestCallback( return SendResponseAndCleanUp(delegate, endpoint, commandObj, commandPath, imcode::ConstraintError); } - // If the preset type for the preset scenario does not supports names and a name is specified, return CONSTRAINT_ERROR. + // If the preset type for the preset scenario does not supports name and a name is specified, return CONSTRAINT_ERROR. if (!PresetTypeSupportsNames(delegate, presetScenario) && pendingPreset.GetName().HasValue()) { return SendResponseAndCleanUp(delegate, endpoint, commandObj, commandPath, imcode::ConstraintError); @@ -1522,7 +1522,7 @@ bool emberAfThermostatClusterCommitPresetsSchedulesRequestCallback( pendingPreset.SetCoolingSetpoint(MakeOptional(EnforceCoolingSetpointLimits(coolingSetpointValue.Value(), endpoint))); } - Optional heatingSetpointValue = preset.GetHeatingSetpoint(); + Optional heatingSetpointValue = pendingPreset.GetHeatingSetpoint(); if (heatingSetpointValue.HasValue()) { pendingPreset.SetHeatingSetpoint(MakeOptional(EnforceHeatingSetpointLimits(heatingSetpointValue.Value(), endpoint)));