Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use BitMask instead of BitFlags for data model encoding of bitmaps #19008

Merged
merged 22 commits into from
Jun 3, 2022
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions examples/chip-tool/commands/clusters/ComplexArgument.h
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,16 @@ class ComplexArgumentParser
return CHIP_NO_ERROR;
}

template <typename T>
static CHIP_ERROR Setup(const char * label, chip::BitMask<T> & request, Json::Value & value)
{
T requestValue;
ReturnErrorOnFailure(ComplexArgumentParser::Setup(label, requestValue, value));

request = chip::BitMask<T>(requestValue);
return CHIP_NO_ERROR;
}

template <typename T>
static CHIP_ERROR Setup(const char * label, chip::Optional<T> & request, Json::Value & value)
{
Expand Down
9 changes: 9 additions & 0 deletions examples/chip-tool/commands/common/Command.h
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,15 @@ class Command
return AddArgument(name, min, max, reinterpret_cast<T *>(out), desc, flags);
}

template <typename T>
size_t AddArgument(const char * name, int64_t min, uint64_t max, chip::BitMask<T> * out, const char * desc = "",
uint8_t flags = 0)
{
// This is a terrible hack that relies on BitMask only having the one
// mValue member.
return AddArgument(name, min, max, reinterpret_cast<T *>(out), desc, flags);
}

template <typename T>
size_t AddArgument(const char * name, chip::Optional<T> * value, const char * desc = "")
{
Expand Down
2 changes: 1 addition & 1 deletion examples/pump-app/cc13x2x7_26x2x7/main/AppTask.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ void AppTask::UpdateClusterState(void)
void AppTask::UpdateCluster(intptr_t context)
{
EmberStatus status;
BitFlags<PumpConfigurationAndControl::PumpStatus> pumpStatus;
BitMask<PumpConfigurationAndControl::PumpStatus> pumpStatus;

ChipLogProgress(NotSpecified, "Update Cluster State");

Expand Down
8 changes: 4 additions & 4 deletions examples/window-app/common/src/WindowApp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,8 @@ void WindowApp::DispatchEvent(const WindowApp::Event & event)
void WindowApp::DispatchEventAttributeChange(chip::EndpointId endpoint, chip::AttributeId attribute)
{
Cover * cover = GetCover(endpoint);
chip::BitFlags<Mode> mode;
chip::BitFlags<ConfigStatus> configStatus;
chip::BitMask<Mode> mode;
chip::BitMask<ConfigStatus> configStatus;

if (nullptr == cover)
{
Expand Down Expand Up @@ -426,7 +426,7 @@ void WindowApp::Cover::Init(chip::EndpointId endpoint)
TypeSet(endpoint, Type::kTiltBlindLiftAndTilt);

// Attribute: Id 7 ConfigStatus
chip::BitFlags<ConfigStatus> configStatus = ConfigStatusGet(endpoint);
chip::BitMask<ConfigStatus> configStatus = ConfigStatusGet(endpoint);
configStatus.Set(ConfigStatus::kLiftEncoderControlled);
configStatus.Set(ConfigStatus::kTiltEncoderControlled);
configStatus.Set(ConfigStatus::kOnlineReserved);
Expand All @@ -438,7 +438,7 @@ void WindowApp::Cover::Init(chip::EndpointId endpoint)
EndProductTypeSet(endpoint, EndProductType::kInteriorBlind);

// Attribute: Id 24 Mode
chip::BitFlags<Mode> mode;
chip::BitMask<Mode> mode;
mode.Clear(Mode::kMotorDirectionReversed);
mode.Clear(Mode::kMaintenanceMode);
mode.Clear(Mode::kCalibrationMode);
Expand Down
2 changes: 1 addition & 1 deletion src/app/clusters/door-lock-server/door-lock-server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2248,7 +2248,7 @@ void DoorLockServer::sendGetWeekDayScheduleResponse(chip::app::CommandHandler *
response.status = status;
if (DlStatus::kSuccess == status)
{
response.daysMask = Optional<chip::BitFlags<DlDaysMaskMap>>(daysMask);
response.daysMask = Optional<chip::BitMask<DlDaysMaskMap>>(daysMask);
response.startHour = Optional<uint8_t>(startHour);
response.startMinute = Optional<uint8_t>(startMinute);
response.endHour = Optional<uint8_t>(endHour);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ static void updateAttributeLinks(EndpointId endpoint)
{
PumpControlMode controlMode;
PumpOperationMode operationMode;
BitFlags<PumpStatus> pumpStatus;
BitMask<PumpStatus> pumpStatus;

// Get the current control- and operation modes
Attributes::ControlMode::Get(endpoint, &controlMode);
Expand Down
32 changes: 16 additions & 16 deletions src/app/clusters/window-covering-server/window-covering-server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ Type TypeGet(chip::EndpointId endpoint)
return value;
}

void ConfigStatusPrint(const chip::BitFlags<ConfigStatus> & configStatus)
void ConfigStatusPrint(const chip::BitMask<ConfigStatus> & configStatus)
{
emberAfWindowCoveringClusterPrint("ConfigStatus 0x%02X Operational=%u OnlineReserved=%u", configStatus.Raw(),
configStatus.Has(ConfigStatus::kOperational),
Expand All @@ -177,22 +177,22 @@ void ConfigStatusPrint(const chip::BitFlags<ConfigStatus> & configStatus)
configStatus.Has(ConfigStatus::kTiltPositionAware), configStatus.Has(ConfigStatus::kTiltEncoderControlled));
}

void ConfigStatusSet(chip::EndpointId endpoint, const chip::BitFlags<ConfigStatus> & configStatus)
void ConfigStatusSet(chip::EndpointId endpoint, const chip::BitMask<ConfigStatus> & configStatus)
{
Attributes::ConfigStatus::Set(endpoint, configStatus);
}

chip::BitFlags<ConfigStatus> ConfigStatusGet(chip::EndpointId endpoint)
chip::BitMask<ConfigStatus> ConfigStatusGet(chip::EndpointId endpoint)
{
chip::BitFlags<ConfigStatus> configStatus;
chip::BitMask<ConfigStatus> configStatus;
Attributes::ConfigStatus::Get(endpoint, &configStatus);

return configStatus;
}

void ConfigStatusUpdateFeatures(chip::EndpointId endpoint)
{
chip::BitFlags<ConfigStatus> configStatus = ConfigStatusGet(endpoint);
chip::BitMask<ConfigStatus> configStatus = ConfigStatusGet(endpoint);

configStatus.Set(ConfigStatus::kLiftPositionAware, HasFeaturePaLift(endpoint));
configStatus.Set(ConfigStatus::kTiltPositionAware, HasFeaturePaTilt(endpoint));
Expand Down Expand Up @@ -255,19 +255,19 @@ EndProductType EndProductTypeGet(chip::EndpointId endpoint)
return value;
}

void ModePrint(const chip::BitFlags<Mode> & mode)
void ModePrint(const chip::BitMask<Mode> & mode)
{
emberAfWindowCoveringClusterPrint("Mode 0x%02X MotorDirReversed=%u LedFeedback=%u Maintenance=%u Calibration=%u", mode.Raw(),
mode.Has(Mode::kMotorDirectionReversed), mode.Has(Mode::kLedFeedback),
mode.Has(Mode::kMaintenanceMode), mode.Has(Mode::kCalibrationMode));
}

void ModeSet(chip::EndpointId endpoint, chip::BitFlags<Mode> & newMode)
void ModeSet(chip::EndpointId endpoint, chip::BitMask<Mode> & newMode)
{
chip::BitFlags<ConfigStatus> newStatus;
chip::BitMask<ConfigStatus> newStatus;

chip::BitFlags<ConfigStatus> oldStatus = ConfigStatusGet(endpoint);
chip::BitFlags<Mode> oldMode = ModeGet(endpoint);
chip::BitMask<ConfigStatus> oldStatus = ConfigStatusGet(endpoint);
chip::BitMask<Mode> oldMode = ModeGet(endpoint);

newStatus = oldStatus;

Expand All @@ -288,9 +288,9 @@ void ModeSet(chip::EndpointId endpoint, chip::BitFlags<Mode> & newMode)
ConfigStatusSet(endpoint, newStatus);
}

chip::BitFlags<Mode> ModeGet(chip::EndpointId endpoint)
chip::BitMask<Mode> ModeGet(chip::EndpointId endpoint)
{
chip::BitFlags<Mode> mode;
chip::BitMask<Mode> mode;

Attributes::Mode::Get(endpoint, &mode);
return mode;
Expand Down Expand Up @@ -591,8 +591,8 @@ void PostAttributeChange(chip::EndpointId endpoint, chip::AttributeId attributeI
{
// all-cluster-app: simulation for the CI testing
// otherwise it is defined for manufacturer specific implementation */
BitFlags<Mode> mode;
BitFlags<ConfigStatus> configStatus;
BitMask<Mode> mode;
BitMask<ConfigStatus> configStatus;
NPercent100ths current, target;
OperationalStatus prevOpStatus = OperationalStatusGet(endpoint);
OperationalStatus opStatus = prevOpStatus;
Expand Down Expand Up @@ -659,8 +659,8 @@ void PostAttributeChange(chip::EndpointId endpoint, chip::AttributeId attributeI

EmberAfStatus GetMotionLockStatus(chip::EndpointId endpoint)
{
BitFlags<Mode> mode = ModeGet(endpoint);
BitFlags<ConfigStatus> configStatus = ConfigStatusGet(endpoint);
BitMask<Mode> mode = ModeGet(endpoint);
BitMask<ConfigStatus> configStatus = ConfigStatusGet(endpoint);

// Is the device locked?
if (!configStatus.Has(ConfigStatus::kOperational))
Expand Down
12 changes: 6 additions & 6 deletions src/app/clusters/window-covering-server/window-covering-server.h
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,9 @@ bool HasFeaturePaTilt(chip::EndpointId endpoint);
void TypeSet(chip::EndpointId endpoint, Type type);
Type TypeGet(chip::EndpointId endpoint);

void ConfigStatusPrint(const chip::BitFlags<ConfigStatus> & configStatus);
void ConfigStatusSet(chip::EndpointId endpoint, const chip::BitFlags<ConfigStatus> & status);
chip::BitFlags<ConfigStatus> ConfigStatusGet(chip::EndpointId endpoint);
void ConfigStatusPrint(const chip::BitMask<ConfigStatus> & configStatus);
void ConfigStatusSet(chip::EndpointId endpoint, const chip::BitMask<ConfigStatus> & status);
chip::BitMask<ConfigStatus> ConfigStatusGet(chip::EndpointId endpoint);
void ConfigStatusUpdateFeatures(chip::EndpointId endpoint);

void OperationalStatusSet(chip::EndpointId endpoint, const OperationalStatus & status);
Expand All @@ -115,9 +115,9 @@ Percent100ths ComputePercent100thsStep(OperationalState direction, Percent100ths
void EndProductTypeSet(chip::EndpointId endpoint, EndProductType type);
EndProductType EndProductTypeGet(chip::EndpointId endpoint);

void ModePrint(const chip::BitFlags<Mode> & mode);
void ModeSet(chip::EndpointId endpoint, chip::BitFlags<Mode> & mode);
chip::BitFlags<Mode> ModeGet(chip::EndpointId endpoint);
void ModePrint(const chip::BitMask<Mode> & mode);
void ModeSet(chip::EndpointId endpoint, chip::BitMask<Mode> & mode);
chip::BitMask<Mode> ModeGet(chip::EndpointId endpoint);

void SafetyStatusSet(chip::EndpointId endpoint, SafetyStatus & status);
const SafetyStatus SafetyStatusGet(chip::EndpointId endpoint);
Expand Down
7 changes: 7 additions & 0 deletions src/app/data-model/BasicTypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#pragma once

#include <lib/support/BitFlags.h>
#include <lib/support/BitMask.h>
#include <lib/support/Span.h>

#include <type_traits>
Expand All @@ -43,6 +44,12 @@ struct IsBasicType<BitFlags<X>>
static constexpr bool value = true;
};

template <typename X>
struct IsBasicType<BitMask<X>>
{
static constexpr bool value = true;
};

template <>
struct IsBasicType<ByteSpan>
{
Expand Down
49 changes: 49 additions & 0 deletions src/app/tests/suites/include/ConstraintsChecker.h
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,18 @@ class ConstraintsChecker
return true;
}

template <typename T, typename U>
bool CheckConstraintMinValue(const char * itemName, chip::BitMask<T> current, U expected)
andy31415 marked this conversation as resolved.
Show resolved Hide resolved
{
if (current.Raw() < expected)
{
Exit(std::string(itemName) + " value < minValue: " + std::to_string(current.Raw()) + " < " + std::to_string(expected));
return false;
}

return true;
}

template <typename T, typename U>
bool CheckConstraintMinValue(const char * itemName, const chip::app::DataModel::Nullable<T> & current, U expected)
{
Expand Down Expand Up @@ -270,6 +282,18 @@ class ConstraintsChecker
return true;
}

template <typename T, typename U>
bool CheckConstraintMaxValue(const char * itemName, chip::BitMask<T> current, U expected)
{
if (current.Raw() > expected)
{
Exit(std::string(itemName) + " value > maxValue: " + std::to_string(current.Raw()) + " > " + std::to_string(expected));
return false;
}

return true;
}

template <typename T, typename U>
bool CheckConstraintMaxValue(const char * itemName, const chip::app::DataModel::Nullable<T> & current, U expected)
{
Expand Down Expand Up @@ -345,6 +369,18 @@ class ConstraintsChecker
return true;
}

template <typename T>
bool CheckConstraintNotValue(const char * itemName, chip::BitMask<T> current, chip::BitMask<T> expected)
{
if (current == expected)
{
Exit(std::string(itemName) + " got unexpected value: " + std::to_string(current.Raw()));
return false;
}

return true;
}

template <typename T, typename U>
bool CheckConstraintNotValue(const char * itemName, chip::BitFlags<T> current, U expected)
{
Expand All @@ -358,6 +394,19 @@ class ConstraintsChecker
return true;
}

template <typename T, typename U>
bool CheckConstraintNotValue(const char * itemName, chip::BitMask<T> current, U expected)
{
if (current.Raw() == expected)
{

Exit(std::string(itemName) + " got unexpected value: " + std::to_string(current.Raw()));
return false;
}

return true;
}

template <typename T, typename U>
bool CheckConstraintNotValue(const char * itemName, const chip::app::DataModel::Nullable<T> & current, U expected)
{
Expand Down
12 changes: 12 additions & 0 deletions src/app/tests/suites/include/ValueChecker.h
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,12 @@ class ValueChecker
return CheckValue(itemName, current.Raw(), expected);
}

template <typename T, typename U>
bool CheckValue(const char * itemName, chip::BitMask<T> current, U expected)
{
return CheckValue(itemName, current.Raw(), expected);
}

// Allow an expected value that is a nullable wrapped around the actual
// value (e.g. a SaveAs from reading a different attribute that has a value
// space that includes null). In that case we check that:
Expand All @@ -114,6 +120,12 @@ class ValueChecker
return CheckValue(itemName, current.Raw(), expected.Raw());
}

template <typename T>
bool CheckValue(const char * itemName, chip::BitMask<T> current, chip::BitMask<T> expected)
{
return CheckValue(itemName, current.Raw(), expected.Raw());
}

template <typename T>
bool CheckValuePresent(const char * itemName, const chip::Optional<T> & value)
{
Expand Down
47 changes: 47 additions & 0 deletions src/app/util/attribute-storage-null-handling.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
#pragma once

#include <lib/core/CHIPTLV.h>
#include <lib/support/BitFlags.h>
#include <lib/support/BitMask.h>
#include <lib/support/TypeTraits.h>

#include <limits>
Expand Down Expand Up @@ -171,6 +173,51 @@ struct NumericAttributeTraits<BitFlags<T>>
static uint8_t * ToAttributeStoreRepresentation(StorageType & value) { return reinterpret_cast<uint8_t *>(&value); }
};

template <typename T>
struct NumericAttributeTraits<BitMask<T>>
andy31415 marked this conversation as resolved.
Show resolved Hide resolved
{
using StorageType = T;
using WorkingType = BitMask<T>;

static constexpr void WorkingToStorage(WorkingType workingValue, StorageType & storageValue)
{
storageValue = static_cast<StorageType>(workingValue.Raw());
}

static constexpr WorkingType StorageToWorking(StorageType storageValue) { return WorkingType(storageValue); }

static constexpr void SetNull(StorageType & value)
{
//
// When setting to null, store a value that has all bits set. This aliases to the same behavior
// we do for other integral types, ensuring consistency across all underlying integral types in the data store as well as
// re-using logic for serialization/de-serialization of that data in the IM.
//
value = static_cast<StorageType>(std::numeric_limits<std::underlying_type_t<T>>::max());
}

static constexpr bool IsNullValue(StorageType value)
{
//
// While we store a nullable bitmap value by setting all its bits, we can be a bit more conservative when actually
// testing for null since the spec only mandates that the MSB be reserved for nullable bitmaps.
//
constexpr auto msbSetValue = std::underlying_type_t<T>(1) << (std::numeric_limits<std::underlying_type_t<T>>::digits - 1);
return !!(std::underlying_type_t<T>(value) & msbSetValue);
}

static constexpr bool CanRepresentValue(bool isNullable, StorageType value)
{
//
// We permit the full-range of the underlying StorageType if !isNullable,
// and the restricted range otherwise.
//
return !isNullable || !IsNullValue(value);
}

static uint8_t * ToAttributeStoreRepresentation(StorageType & value) { return reinterpret_cast<uint8_t *>(&value); }
};

template <>
struct NumericAttributeTraits<bool>
{
Expand Down
Loading