Skip to content

Commit

Permalink
Introduce a Status + Cluster Code abstraction (#31121)
Browse files Browse the repository at this point in the history
* Introduce a Status + Cluster Code abstraction

- Existing code always has to split IM status and
  cluster-specific status codes, which causes clumsy
  implementation of clusters (see issue #31120).
- This PR introduces a value that encapsulates both
  the IM status and cluster-specific status, in an
  easy-to-pass-around abstraction, which can be directly
  used in place of Status.
- Subsequent PR will implement usage in IM with overloads
  for common cases, such as for AddStatus.

Issue #31120

Testing done:
- Added unit tests
- Other tests still pass

* Restyled by clang-format

* Restyled by gn

* Address review comments

* Rename ClusterStatus to ClusterStatusCode

* Fix build deps

* Restyled by gn

---------

Co-authored-by: Restyled.io <[email protected]>
  • Loading branch information
2 people authored and pull[bot] committed Jan 18, 2024
1 parent f6a233b commit 1251399
Show file tree
Hide file tree
Showing 11 changed files with 320 additions and 18 deletions.
2 changes: 1 addition & 1 deletion examples/darwin-framework-tool/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ executable("darwin-framework-tool") {

# pics is needed by tests
"${chip_root}/src/app/tests/suites/pics",
"${chip_root}/src/protocols:im_status",
"${chip_root}/src/protocols/interaction_model",
]

defines = []
Expand Down
1 change: 1 addition & 0 deletions src/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ if (chip_build_tests) {
"${chip_root}/src/lib/core/tests",
"${chip_root}/src/messaging/tests",
"${chip_root}/src/protocols/bdx/tests",
"${chip_root}/src/protocols/interaction_model/tests",
"${chip_root}/src/protocols/user_directed_commissioning/tests",
"${chip_root}/src/transport/retransmit/tests",
]
Expand Down
1 change: 1 addition & 0 deletions src/app/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@ static_library("app") {
"${chip_root}/src/lib/address_resolve",
"${chip_root}/src/lib/support",
"${chip_root}/src/messaging",
"${chip_root}/src/protocols/interaction_model",
"${chip_root}/src/protocols/secure_channel",
"${chip_root}/src/system",
"${nlio_root}:nlio",
Expand Down
1 change: 1 addition & 0 deletions src/app/common/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ static_library("cluster-objects") {
public_deps = [
"${chip_root}/src/lib/core",
"${chip_root}/src/lib/support",
"${chip_root}/src/protocols/interaction_model",
]

defines = []
Expand Down
1 change: 1 addition & 0 deletions src/app/tests/TestStatusIB.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <lib/core/ErrorStr.h>
#include <lib/support/CHIPMem.h>
#include <lib/support/UnitTestRegistration.h>
#include <protocols/interaction_model/StatusCode.h>

#include <nlunit-test.h>

Expand Down
2 changes: 1 addition & 1 deletion src/app/tests/suites/commands/interaction_model/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import("//build_overrides/build.gni")
import("//build_overrides/chip.gni")

static_library("interaction_model") {
output_name = "libInteractionModel"
output_name = "libInteractionModelCommands"

sources = [
"InteractionModel.cpp",
Expand Down
17 changes: 1 addition & 16 deletions src/protocols/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -38,27 +38,12 @@ static_library("protocols") {
cflags = [ "-Wconversion" ]

public_deps = [
":im_status",
":type_definitions",
"${chip_root}/src/lib/core",
"${chip_root}/src/lib/support",
"${chip_root}/src/messaging",
"${chip_root}/src/protocols/bdx",
"${chip_root}/src/protocols/interaction_model",
"${chip_root}/src/protocols/secure_channel",
]
}

static_library("im_status") {
sources = [
"interaction_model/StatusCode.cpp",
"interaction_model/StatusCode.h",
]

cflags = [ "-Wconversion" ]

public_deps = [
":type_definitions",
"${chip_root}/src/lib/core",
"${chip_root}/src/lib/support",
]
}
34 changes: 34 additions & 0 deletions src/protocols/interaction_model/BUILD.gn
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# Copyright (c) 2020 Project CHIP Authors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

import("//build_overrides/chip.gni")

static_library("interaction_model") {
output_name = "libInteractionModel"

sources = [
"Constants.h",
"StatusCode.cpp",
"StatusCode.h",
"StatusCodeList.h",
]

cflags = [ "-Wconversion" ]

public_deps = [
"${chip_root}/src/lib/core",
"${chip_root}/src/lib/support",
"${chip_root}/src/protocols:type_definitions",
]
}
109 changes: 109 additions & 0 deletions src/protocols/interaction_model/StatusCode.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,12 @@

#pragma once

#include <limits>
#include <stdint.h>

#include <lib/core/CHIPConfig.h>
#include <lib/core/DataModelTypes.h>
#include <lib/core/Optional.h>
#include <lib/support/TypeTraits.h>

#if CHIP_CONFIG_IM_STATUS_CODE_VERBOSE_FORMAT
Expand Down Expand Up @@ -48,6 +51,112 @@ enum class Status : uint8_t
const char * StatusName(Status status);
#endif // CHIP_CONFIG_IM_STATUS_CODE_VERBOSE_FORMAT

/**
* @brief Class to encapsulate a Status code, including possibly a
* cluster-specific code for generic SUCCESS/FAILURE.
*
* This abstractions joins together Status and ClusterStatus, which
* are the components of a StatusIB, used in many IM actions, in a
* way which allows both of them to carry together.
*
* This can be used everywhere a `Status` is used, but it is lossy
* to the cluster-specific code if used in place of `Status` when
* the cluster-specific code is set.
*
* This class can only be directly constructed from a `Status`. To
* attach a cluster-specific-code, please use the `ClusterSpecificFailure()`
* and `ClusterSpecificSuccess()` factory methods.
*/
class ClusterStatusCode
{
public:
explicit ClusterStatusCode(Status status) : mStatus(status) {}

// We only have simple copyable members, so we should be trivially copyable.
ClusterStatusCode(const ClusterStatusCode & other) = default;
ClusterStatusCode & operator=(const ClusterStatusCode & other) = default;

bool operator==(const ClusterStatusCode & other)
{
return (this->mStatus == other.mStatus) && (this->HasClusterSpecificCode() == other.HasClusterSpecificCode()) &&
(this->GetClusterSpecificCode() == other.GetClusterSpecificCode());
}

bool operator!=(const ClusterStatusCode & other) { return !(*this == other); }

ClusterStatusCode & operator=(const Status & status)
{
this->mStatus = status;
this->mClusterSpecificCode = chip::NullOptional;
return *this;
}

/**
* @brief Builder for a cluster-specific failure status code.
*
* @tparam T - enum type for the cluster-specific status code
* (e.g. chip::app::Clusters::AdministratorCommissioning::CommissioningWindowStatusEnum)
* @param cluster_specific_code - cluster-specific code to record with the failure
* (e.g. chip::app::Clusters::AdministratorCommissioning::CommissioningWindowStatusEnum::kWindowNotOpen)
* @return a ClusterStatusCode instance properly configured.
*/
template <typename T>
static ClusterStatusCode ClusterSpecificFailure(T cluster_specific_code)
{
static_assert(std::numeric_limits<T>::max() <= std::numeric_limits<ClusterStatus>::max(), "Type used must fit in uint8_t");
return ClusterStatusCode(Status::Failure, chip::to_underlying(cluster_specific_code));
}

/**
* @brief Builder for a cluster-specific success status code.
*
* @tparam T - enum type for the cluster-specific status code
* (e.g. chip::app::Clusters::AdministratorCommissioning::CommissioningWindowStatusEnum)
* @param cluster_specific_code - cluster-specific code to record with the success
* (e.g. chip::app::Clusters::AdministratorCommissioning::CommissioningWindowStatusEnum::kBasicWindowOpen)
* @return a ClusterStatusCode instance properly configured.
*/
template <typename T>
static ClusterStatusCode ClusterSpecificSuccess(T cluster_specific_code)
{
static_assert(std::numeric_limits<T>::max() <= std::numeric_limits<ClusterStatus>::max(), "Type used must fit in uint8_t");
return ClusterStatusCode(Status::Success, chip::to_underlying(cluster_specific_code));
}

/// @return true if the core Status associated with this ClusterStatusCode is the one for success.
bool IsSuccess() const { return mStatus == Status::Success; }

/// @return the core Status code associated withi this ClusterStatusCode.
Status GetStatus() const { return mStatus; }

/// @return true if a cluster-specific code is associated with the ClusterStatusCode.
bool HasClusterSpecificCode() const { return mClusterSpecificCode.HasValue(); }

/// @return the cluster-specific code associated with this ClusterStatusCode or chip::NullOptional if none is associated.
chip::Optional<ClusterStatus> GetClusterSpecificCode() const
{
if ((mStatus != Status::Failure) && (mStatus != Status::Success))
{
return chip::NullOptional;
}
return mClusterSpecificCode;
}

// Automatic conversions to common types, using the status code alone.
operator Status() const { return mStatus; }

private:
ClusterStatusCode() = delete;
ClusterStatusCode(Status status, ClusterStatus cluster_specific_code) :
mStatus(status), mClusterSpecificCode(chip::MakeOptional(cluster_specific_code))
{}

Status mStatus;
chip::Optional<ClusterStatus> mClusterSpecificCode;
};

static_assert(sizeof(ClusterStatusCode) <= sizeof(uint32_t), "ClusterStatusCode must not grow to be larger than a uint32_t");

} // namespace InteractionModel
} // namespace Protocols
} // namespace chip
35 changes: 35 additions & 0 deletions src/protocols/interaction_model/tests/BUILD.gn
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# Copyright (c) 2023 Project CHIP Authors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

import("//build_overrides/build.gni")
import("//build_overrides/chip.gni")
import("//build_overrides/nlunit_test.gni")

import("${chip_root}/build/chip/chip_test_suite.gni")

chip_test_suite_using_nltest("tests") {
output_name = "libInteractionModelTests"

test_sources = [ "TestStatusCode.cpp" ]

public_deps = [
"${chip_root}/src/lib/core",
"${chip_root}/src/lib/support",
"${chip_root}/src/lib/support:testing",
"${chip_root}/src/protocols/interaction_model",
"${nlunit_test_root}:nlunit-test",
]

cflags = [ "-Wconversion" ]
}
Loading

0 comments on commit 1251399

Please sign in to comment.