Apply suggestions from code review

Co-authored-by: Boris Zbarsky <[email protected]>

examples/thermostat/linux/include/thermostat-delegate-impl.h

@@ -28,15 +28,15 @@ 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.
  *
  */
 
 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
 {

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,23 +136,15 @@ 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<ByteSpan> & newActivePresetHandle)
 {
     if (!newActivePresetHandle.IsNull())
     {
         size_t newActivePresetHandleSize = newActivePresetHandle.Value().size();
-        if (newActivePresetHandleSize > 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<B
     }
     else
     {
-        memset(mActivePresetHandleData, 0, kPresetHandleSize);
+        memset(mActivePresetHandleData, 0, sizeof(mActivePresetHandleData));
         mActivePresetHandleDataSize = 0;
         ChipLogDetail(NotSpecified, "Clear ActivePresetHandle");
     }

examples/thermostat/linux/thermostat-manager.cpp

@@ -289,7 +289,7 @@ int16_t ThermostatManager::GetCurrentTemperature()
     DataModel::Nullable<int16_t> currentTemperature;
     currentTemperature.SetNull();
     LocalTemperature::Get(kThermostatEndpoint, currentTemperature);
-    return (!currentTemperature.IsNull()) ? currentTemperature.Value() : 0;
+    return currentTemperature.ValueOr(0);
 }
 
 int16_t ThermostatManager::GetCurrentHeatingSetPoint()

src/app/clusters/thermostat-server/PresetStructWithOwnedMembers.cpp

@@ -86,7 +86,7 @@ CHIP_ERROR PresetStructWithOwnedMembers::SetName(const Optional<DataModel::Nulla
         if (newNameSize > kPresetNameSize)
         {
             ChipLogError(Zcl, "Failed to set Preset name. New name size (%u) > allowed preset name size (%u)",
-                         static_cast<uint8_t>(newNameSize), static_cast<uint8_t>(kPresetNameSize));
+                         static_cast<unsigned>(newNameSize), static_cast<unsigned>(kPresetNameSize));
             return CHIP_ERROR_NO_MEMORY;
         }
         MutableCharSpan targetSpan(presetNameData);

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,15 +855,15 @@ 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,
         // otherwise return BUSY.
         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<int16_t> heatingSetpointValue = preset.GetHeatingSetpoint();
+        Optional<int16_t> heatingSetpointValue = pendingPreset.GetHeatingSetpoint();
         if (heatingSetpointValue.HasValue())
         {
             pendingPreset.SetHeatingSetpoint(MakeOptional(EnforceHeatingSetpointLimits(heatingSetpointValue.Value(), endpoint)));