Skip to content

Commit a83562d

Browse files
Address followup issues for preset/atomic-write implementation.
* Puts some file-local functions in an anonymous namespace. * Fixes the "is this attribute supported?" check to correctly check for global attributes that are not in Ember metadata. * Moves the comment explaining why it's OK to skip the spec-required ACL check to the place where that check is being skipped. * Removes a non-spec-compliant "error if the timeout is 0" bit. Fixes project-chip#35168
1 parent c17fd97 commit a83562d

File tree

2 files changed

+33
-11
lines changed

2 files changed

+33
-11
lines changed

src/app/GlobalAttributes.h

+13
Original file line numberDiff line numberDiff line change
@@ -40,5 +40,18 @@ constexpr AttributeId GlobalAttributesNotInMetadata[] = {
4040

4141
static_assert(ArrayIsSorted(GlobalAttributesNotInMetadata), "Array of global attribute ids must be sorted");
4242

43+
inline bool IsSupportedGlobalAttributeNotInMetadata(AttributeId attributeId)
44+
{
45+
for (auto & attr : GlobalAttributesNotInMetadata)
46+
{
47+
if (attr == attributeId)
48+
{
49+
return true;
50+
}
51+
}
52+
53+
return false;
54+
}
55+
4356
} // namespace app
4457
} // namespace chip

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

+20-11
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
#include "thermostat-server.h"
1919

20+
#include <app/GlobalAttributes.h>
2021
#include <platform/internal/CHIPDeviceLayerInternal.h>
2122

2223
using namespace chip;
@@ -47,6 +48,8 @@ void TimerExpiredCallback(System::Layer * systemLayer, void * callbackContext)
4748
gThermostatAttrAccess.ResetAtomicWrite(endpoint);
4849
}
4950

51+
namespace {
52+
5053
/**
5154
* @brief Schedules a timer for the given timeout in milliseconds.
5255
*
@@ -185,15 +188,25 @@ Status BuildAttributeStatuses(const EndpointId endpoint, const DataModel::Decoda
185188
const EmberAfAttributeMetadata * metadata =
186189
emberAfLocateAttributeMetadata(endpoint, Thermostat::Id, attributeStatus.attributeID);
187190

188-
if (metadata == nullptr)
191+
if (metadata != nullptr)
192+
{
193+
// This is definitely an attribute we know about.
194+
continue;
195+
}
196+
197+
if (IsSupportedGlobalAttributeNotInMetadata(attributeStatus.attributeID))
189198
{
190-
// This is not a valid attribute on the Thermostat cluster on the supplied endpoint
191-
return Status::InvalidCommand;
199+
continue;
192200
}
201+
202+
// This is not a valid attribute on the Thermostat cluster on the supplied endpoint
203+
return Status::InvalidCommand;
193204
}
194205
return Status::Success;
195206
}
196207

208+
} // anonymous namespace
209+
197210
bool ThermostatAttrAccess::InAtomicWrite(EndpointId endpoint, Optional<AttributeId> attributeId)
198211
{
199212

@@ -422,6 +435,10 @@ void ThermostatAttrAccess::BeginAtomicWrite(CommandHandler * commandObj, const C
422435
status = Status::Success;
423436
for (size_t i = 0; i < attributeStatuses.AllocatedSize(); ++i)
424437
{
438+
// If we've gotten this far, then the client has manage permission to call AtomicRequest,
439+
// which is also the privilege necessary to write to the atomic attributes, so no need to do
440+
// the "If the client does not have sufficient privilege to write to the attribute" check
441+
// from the spec.
425442
auto & attributeStatus = attributeStatuses[i];
426443
auto statusCode = Status::Success;
427444
switch (attributeStatus.attributeID)
@@ -442,11 +459,6 @@ void ThermostatAttrAccess::BeginAtomicWrite(CommandHandler * commandObj, const C
442459
}
443460

444461
auto timeout = std::min(System::Clock::Milliseconds16(commandData.timeout.Value()), maximumTimeout);
445-
if (timeout.count() == 0)
446-
{
447-
commandObj->AddStatus(commandPath, Status::InvalidInState);
448-
return;
449-
}
450462

451463
if (status == Status::Success)
452464
{
@@ -605,9 +617,6 @@ bool emberAfThermostatClusterAtomicRequestCallback(CommandHandler * commandObj,
605617
{
606618
auto & requestType = commandData.requestType;
607619

608-
// If we've gotten this far, then the client has manage permission to call AtomicRequest, which is also the
609-
// privilege necessary to write to the atomic attributes, so no need to check
610-
611620
switch (requestType)
612621
{
613622
case Globals::AtomicRequestTypeEnum::kBeginWrite:

0 commit comments

Comments
 (0)