Skip to content

Commit

Permalink
Switch the color control move/step enums to enum class.
Browse files Browse the repository at this point in the history 
Gets us some type safety and removes some of the confusion where we were
switching on a thing called moveMode but comparing it to move direction values.
  • Loading branch information
@bzbarsky-apple
bzbarsky-apple committed Mar 10, 2023
1 parent bdee2ef commit 6bed4b3
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 186 deletions.
95 changes: 44 additions & 51 deletions src/app/clusters/color-control-server/color-control-server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -813,7 +813,7 @@ EmberEventControl * ColorControlServer::configureHSVEventControl(EndpointId endp
* @return false Failed
*/
bool ColorControlServer::moveHueCommand(app::CommandHandler * commandObj, const app::ConcreteCommandPath & commandPath,
uint8_t moveMode, uint16_t rate, uint8_t optionsMask, uint8_t optionsOverride,
HueMoveMode moveMode, uint16_t rate, uint8_t optionsMask, uint8_t optionsOverride,
bool isEnhanced)
{
EndpointId endpoint = commandPath.mEndpointId;
Expand All @@ -823,9 +823,7 @@ bool ColorControlServer::moveHueCommand(app::CommandHandler * commandObj, const
VerifyOrExit(colorHueTransitionState != nullptr, status = Status::UnsupportedEndpoint);

// check moveMode before any operation is done on the transition states
if ((rate == 0 && moveMode != EMBER_ZCL_HUE_MOVE_MODE_STOP) ||
(moveMode != EMBER_ZCL_HUE_MOVE_MODE_STOP && moveMode != EMBER_ZCL_HUE_MOVE_MODE_UP &&
moveMode != EMBER_ZCL_HUE_MOVE_MODE_DOWN))
if (moveMode == HueMoveMode::kUnknownEnumValue || (rate == 0 && moveMode != HueMoveMode::kStop))
{
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
Expand All @@ -842,7 +840,7 @@ bool ColorControlServer::moveHueCommand(app::CommandHandler * commandObj, const
// now, kick off the state machine.
initHueTransitionState(endpoint, colorHueTransitionState, isEnhanced);

if (moveMode == EMBER_ZCL_HUE_MOVE_MODE_STOP)
if (moveMode == HueMoveMode::kStop)
{
// Per spec any saturation transition must also be cancelled.
Color16uTransitionState * saturationState = getSaturationTransitionState(endpoint);
Expand All @@ -861,7 +859,7 @@ bool ColorControlServer::moveHueCommand(app::CommandHandler * commandObj, const
handleModeSwitch(endpoint, ColorControlServer::ColorMode::COLOR_MODE_HSV);
}

if (moveMode == EMBER_ZCL_HUE_MOVE_MODE_UP)
if (moveMode == HueMoveMode::kUp)
{
if (isEnhanced)
{
Expand All @@ -874,7 +872,7 @@ bool ColorControlServer::moveHueCommand(app::CommandHandler * commandObj, const

colorHueTransitionState->up = true;
}
else if (moveMode == EMBER_ZCL_HUE_MOVE_MODE_DOWN)
else if (moveMode == HueMoveMode::kDown)
{
if (isEnhanced)
{
Expand Down Expand Up @@ -919,14 +917,14 @@ bool ColorControlServer::moveHueCommand(app::CommandHandler * commandObj, const
* @return false Failed
*/
bool ColorControlServer::moveToHueCommand(app::CommandHandler * commandObj, const app::ConcreteCommandPath & commandPath,
uint16_t hue, uint8_t hueMoveMode, uint16_t transitionTime, uint8_t optionsMask,
uint16_t hue, HueDirection moveDirection, uint16_t transitionTime, uint8_t optionsMask,
uint8_t optionsOverride, bool isEnhanced)
{
EndpointId endpoint = commandPath.mEndpointId;

Status status = Status::Success;
uint16_t currentHue = 0;
uint8_t direction;
HueDirection direction;

ColorHueTransitionState * colorHueTransitionState = getColorHueTransitionState(endpoint);

Expand Down Expand Up @@ -957,42 +955,40 @@ bool ColorControlServer::moveToHueCommand(app::CommandHandler * commandObj, cons
transitionTime++;
}

// For move to hue, the move modes are different from the other move commands.
// Need to translate from the move to hue transitions to the internal
// representation.
switch (hueMoveMode)
// Convert the ShortestDistance/LongestDistance moveDirection values into Up/Down.
switch (moveDirection)
{
case EMBER_ZCL_HUE_DIRECTION_SHORTEST_DISTANCE:
case HueDirection::kShortestDistance:
if ((isEnhanced && (static_cast<uint16_t>(currentHue - hue) > HALF_MAX_UINT16T)) ||
(!isEnhanced && (static_cast<uint8_t>(currentHue - hue) > HALF_MAX_UINT8T)))
{
direction = MOVE_MODE_UP;
direction = HueDirection::kUp;
}
else
{
direction = MOVE_MODE_DOWN;
direction = HueDirection::kDown;
}
break;
case EMBER_ZCL_HUE_DIRECTION_LONGEST_DISTANCE:
case HueDirection::kLongestDistance:
if ((isEnhanced && (static_cast<uint16_t>(currentHue - hue) > HALF_MAX_UINT16T)) ||
(!isEnhanced && (static_cast<uint8_t>(currentHue - hue) > HALF_MAX_UINT8T)))
{
direction = MOVE_MODE_DOWN;
direction = HueDirection::kDown;
}
else
{
direction = MOVE_MODE_UP;
direction = HueDirection::kUp;
}
break;
case EMBER_ZCL_HUE_DIRECTION_UP:
direction = MOVE_MODE_UP;
case HueDirection::kUp:
case HueDirection::kDown:
direction = moveDirection;
break;
case EMBER_ZCL_HUE_DIRECTION_DOWN:
direction = MOVE_MODE_DOWN;
break;
default:
case HueDirection::kUnknownEnumValue:
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
/* No default case, so if a new direction value gets added we will just fail
to compile until we handle it correctly. */
}

if (!shouldExecuteIfOff(endpoint, optionsMask, optionsOverride))
Expand Down Expand Up @@ -1029,7 +1025,7 @@ bool ColorControlServer::moveToHueCommand(app::CommandHandler * commandObj, cons
colorHueTransitionState->stepsRemaining = transitionTime;
colorHueTransitionState->stepsTotal = transitionTime;
colorHueTransitionState->endpoint = endpoint;
colorHueTransitionState->up = (direction == MOVE_MODE_UP);
colorHueTransitionState->up = (direction == HueDirection::kUp);
colorHueTransitionState->repeat = false;

SetHSVRemainingTime(endpoint);
Expand Down Expand Up @@ -1169,7 +1165,7 @@ bool ColorControlServer::moveToHueAndSaturationCommand(app::CommandHandler * com
* @return false Failed
*/
bool ColorControlServer::stepHueCommand(app::CommandHandler * commandObj, const app::ConcreteCommandPath & commandPath,
uint8_t stepMode, uint16_t stepSize, uint16_t transitionTime, uint8_t optionsMask,
HueStepMode stepMode, uint16_t stepSize, uint16_t transitionTime, uint8_t optionsMask,
uint8_t optionsOverride, bool isEnhanced)
{
EndpointId endpoint = commandPath.mEndpointId;
Expand All @@ -1180,7 +1176,7 @@ bool ColorControlServer::stepHueCommand(app::CommandHandler * commandObj, const
VerifyOrExit(colorHueTransitionState != nullptr, status = Status::UnsupportedEndpoint);

// Confirm validity of the step mode received
if (stepMode != STEP_MODE_UP && stepMode != STEP_MODE_DOWN)
if (stepMode == HueStepMode::kUnknownEnumValue)
{
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
Expand Down Expand Up @@ -1216,25 +1212,25 @@ bool ColorControlServer::stepHueCommand(app::CommandHandler * commandObj, const
if (isEnhanced)
{

if (stepMode == STEP_MODE_UP)
if (stepMode == HueStepMode::kUp)
{
colorHueTransitionState->finalEnhancedHue = addEnhancedHue(colorHueTransitionState->currentEnhancedHue, stepSize);
colorHueTransitionState->up = true;
}
else if (stepMode == STEP_MODE_DOWN)
else if (stepMode == HueStepMode::kDown)
{
colorHueTransitionState->finalEnhancedHue = subtractEnhancedHue(colorHueTransitionState->currentEnhancedHue, stepSize);
colorHueTransitionState->up = false;
}
}
else
{
if (stepMode == MOVE_MODE_UP)
if (stepMode == HueStepMode::kUp)
{
colorHueTransitionState->finalHue = addHue(colorHueTransitionState->currentHue, static_cast<uint8_t>(stepSize));
colorHueTransitionState->up = true;
}
else if (stepMode == STEP_MODE_DOWN)
else if (stepMode == HueStepMode::kDown)
{
colorHueTransitionState->finalHue = subtractHue(colorHueTransitionState->currentHue, static_cast<uint8_t>(stepSize));
colorHueTransitionState->up = false;
Expand Down Expand Up @@ -1270,9 +1266,7 @@ bool ColorControlServer::moveSaturationCommand(app::CommandHandler * commandObj,
VerifyOrExit(colorSaturationTransitionState != nullptr, status = Status::UnsupportedEndpoint);

// check moveMode before any operation is done on the transition states
if ((rate == 0 && moveMode != EMBER_ZCL_SATURATION_MOVE_MODE_STOP) ||
(moveMode != EMBER_ZCL_SATURATION_MOVE_MODE_STOP && moveMode != EMBER_ZCL_SATURATION_MOVE_MODE_UP &&
moveMode != EMBER_ZCL_SATURATION_MOVE_MODE_DOWN))
if (moveMode == SaturationMoveMode::kUnknownEnumValue || (rate == 0 && moveMode != SaturationMoveMode::kStop))
{
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
Expand All @@ -1292,7 +1286,7 @@ bool ColorControlServer::moveSaturationCommand(app::CommandHandler * commandObj,
// now, kick off the state machine.
initSaturationTransitionState(endpoint, colorSaturationTransitionState);

if (moveMode == EMBER_ZCL_SATURATION_MOVE_MODE_STOP)
if (moveMode == SaturationMoveMode::kStop)
{
// Per spec any hue transition must also be cancelled.
ColorHueTransitionState * hueState = getColorHueTransitionState(endpoint);
Expand All @@ -1304,11 +1298,11 @@ bool ColorControlServer::moveSaturationCommand(app::CommandHandler * commandObj,
// Handle color mode transition, if necessary.
handleModeSwitch(endpoint, COLOR_MODE_HSV);

if (moveMode == EMBER_ZCL_SATURATION_MOVE_MODE_UP)
if (moveMode == SaturationMoveMode::kUp)
{
colorSaturationTransitionState->finalValue = MAX_SATURATION_VALUE;
}
else if (moveMode == EMBER_ZCL_SATURATION_MOVE_MODE_DOWN)
else if (moveMode == SaturationMoveMode::kDown)
{
colorSaturationTransitionState->finalValue = MIN_SATURATION_VALUE;
}
Expand Down Expand Up @@ -1401,7 +1395,7 @@ bool ColorControlServer::moveToSaturationCommand(app::CommandHandler * commandOb
bool ColorControlServer::stepSaturationCommand(app::CommandHandler * commandObj, const app::ConcreteCommandPath & commandPath,
const Commands::StepSaturation::DecodableType & commandData)
{
uint8_t stepMode = commandData.stepMode;
auto stepMode = commandData.stepMode;
uint8_t stepSize = commandData.stepSize;
uint8_t transitionTime = commandData.transitionTime;
uint8_t optionsMask = commandData.optionsMask;
Expand All @@ -1414,7 +1408,7 @@ bool ColorControlServer::stepSaturationCommand(app::CommandHandler * commandObj,
VerifyOrExit(colorSaturationTransitionState != nullptr, status = Status::UnsupportedEndpoint);

// Confirm validity of the step mode received
if (stepMode != STEP_MODE_UP && stepMode != STEP_MODE_DOWN)
if (stepMode == SaturationStepMode::kUnknownEnumValue)
{
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
Expand All @@ -1441,11 +1435,11 @@ bool ColorControlServer::stepSaturationCommand(app::CommandHandler * commandObj,
initSaturationTransitionState(endpoint, colorSaturationTransitionState);
currentSaturation = static_cast<uint8_t>(colorSaturationTransitionState->currentValue);

if (stepMode == MOVE_MODE_UP)
if (stepMode == SaturationStepMode::kUp)
{
colorSaturationTransitionState->finalValue = addSaturation(currentSaturation, stepSize);
}
else if (stepMode == MOVE_MODE_DOWN)
else if (stepMode == SaturationStepMode::kDown)
{
colorSaturationTransitionState->finalValue = subtractSaturation(currentSaturation, stepSize);
}
Expand Down Expand Up @@ -2205,7 +2199,7 @@ void ColorControlServer::updateTempCommand(EndpointId endpoint)
bool ColorControlServer::moveColorTempCommand(app::CommandHandler * commandObj, const app::ConcreteCommandPath & commandPath,
const Commands::MoveColorTemperature::DecodableType & commandData)
{
uint8_t moveMode = commandData.moveMode;
auto moveMode = commandData.moveMode;
uint16_t rate = commandData.rate;
uint16_t colorTemperatureMinimum = commandData.colorTemperatureMinimumMireds;
uint16_t colorTemperatureMaximum = commandData.colorTemperatureMaximumMireds;
Expand All @@ -2221,8 +2215,7 @@ bool ColorControlServer::moveColorTempCommand(app::CommandHandler * commandObj,
VerifyOrExit(colorTempTransitionState != nullptr, status = Status::UnsupportedEndpoint);

// check moveMode before any operation is done on the transition states
if ((rate == 0 && moveMode != MOVE_MODE_STOP) ||
(moveMode != MOVE_MODE_STOP && moveMode != MOVE_MODE_UP && moveMode != MOVE_MODE_DOWN))
if (moveMode == HueMoveMode::kUnknownEnumValue || (rate == 0 && moveMode != HueMoveMode::kStop))
{
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
Expand All @@ -2240,7 +2233,7 @@ bool ColorControlServer::moveColorTempCommand(app::CommandHandler * commandObj,
// New command. Need to stop any active transitions.
stopAllColorTransitions(endpoint);

if (moveMode == MOVE_MODE_STOP)
if (moveMode == HueMoveMode::kStop)
{
commandObj->AddStatus(commandPath, Status::Success);
return true;
Expand All @@ -2263,7 +2256,7 @@ bool ColorControlServer::moveColorTempCommand(app::CommandHandler * commandObj,
Attributes::ColorTemperatureMireds::Get(endpoint, &colorTempTransitionState->initialValue);
colorTempTransitionState->currentValue = colorTempTransitionState->initialValue;

if (moveMode == MOVE_MODE_UP)
if (moveMode == HueMoveMode::kUp)
{
if (tempPhysicalMax > colorTemperatureMaximum)
{
Expand Down Expand Up @@ -2326,7 +2319,7 @@ bool ColorControlServer::moveToColorTempCommand(app::CommandHandler * commandObj
bool ColorControlServer::stepColorTempCommand(app::CommandHandler * commandObj, const app::ConcreteCommandPath & commandPath,
const Commands::StepColorTemperature::DecodableType & commandData)
{
uint8_t stepMode = commandData.stepMode;
auto stepMode = commandData.stepMode;
uint16_t stepSize = commandData.stepSize;
uint16_t transitionTime = commandData.transitionTime;
uint16_t colorTemperatureMinimum = commandData.colorTemperatureMinimumMireds;
Expand All @@ -2342,7 +2335,7 @@ bool ColorControlServer::stepColorTempCommand(app::CommandHandler * commandObj,
VerifyOrExit(colorTempTransitionState != nullptr, status = Status::UnsupportedEndpoint);

// Confirm validity of the step mode received
if (stepMode != STEP_MODE_UP && stepMode != STEP_MODE_DOWN)
if (stepMode == HueStepMode::kUnknownEnumValue)
{
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
Expand Down Expand Up @@ -2382,7 +2375,7 @@ bool ColorControlServer::stepColorTempCommand(app::CommandHandler * commandObj,
Attributes::ColorTemperatureMireds::Get(endpoint, &colorTempTransitionState->initialValue);
colorTempTransitionState->currentValue = colorTempTransitionState->initialValue;

if (stepMode == STEP_MODE_UP)
if (stepMode == HueStepMode::kUp)
{
uint32_t finalValue32u = static_cast<uint32_t>(colorTempTransitionState->initialValue) + static_cast<uint32_t>(stepSize);
if (finalValue32u > UINT16_MAX)
Expand All @@ -2394,7 +2387,7 @@ bool ColorControlServer::stepColorTempCommand(app::CommandHandler * commandObj,
colorTempTransitionState->finalValue = static_cast<uint16_t>(finalValue32u);
}
}
else if (stepMode == STEP_MODE_DOWN)
else if (stepMode == HueStepMode::kDown)
{
uint32_t finalValue32u = static_cast<uint32_t>(colorTempTransitionState->initialValue) - static_cast<uint32_t>(stepSize);
if (finalValue32u > UINT16_MAX)
Expand Down
24 changes: 7 additions & 17 deletions src/app/clusters/color-control-server/color-control-server.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,19 +66,9 @@ class ColorControlServer
/**********************************************************
* Enums
*********************************************************/

enum MoveMode
{
MOVE_MODE_STOP = 0x00,
MOVE_MODE_UP = 0x01,
MOVE_MODE_DOWN = 0x03
};

enum StepMode
{
STEP_MODE_UP = 0x01,
STEP_MODE_DOWN = 0x03
};
using HueStepMode = chip::app::Clusters::ColorControl::HueStepMode;
using HueMoveMode = chip::app::Clusters::ColorControl::HueMoveMode;
using HueDirection = chip::app::Clusters::ColorControl::HueDirection;

enum ColorLoopDirection
{
Expand Down Expand Up @@ -151,16 +141,16 @@ class ColorControlServer

#ifdef EMBER_AF_PLUGIN_COLOR_CONTROL_SERVER_HSV
bool moveHueCommand(chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath,
uint8_t moveMode, uint16_t rate, uint8_t optionsMask, uint8_t optionsOverride, bool isEnhanced);
HueMoveMode moveMode, uint16_t rate, uint8_t optionsMask, uint8_t optionsOverride, bool isEnhanced);
bool moveToHueCommand(chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath, uint16_t hue,
uint8_t hueMoveMode, uint16_t transitionTime, uint8_t optionsMask, uint8_t optionsOverride,
HueDirection moveDirection, uint16_t transitionTime, uint8_t optionsMask, uint8_t optionsOverride,
bool isEnhanced);
bool moveToHueAndSaturationCommand(chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath,
uint16_t hue, uint8_t saturation, uint16_t transitionTime, uint8_t optionsMask,
uint8_t optionsOverride, bool isEnhanced);
bool stepHueCommand(chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath,
uint8_t stepMode, uint16_t stepSize, uint16_t transitionTime, uint8_t optionsMask, uint8_t optionsOverride,
bool isEnhanced);
HueStepMode stepMode, uint16_t stepSize, uint16_t transitionTime, uint8_t optionsMask,
uint8_t optionsOverride, bool isEnhanced);
bool moveSaturationCommand(chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath,
const chip::app::Clusters::ColorControl::Commands::MoveSaturation::DecodableType & commandData);
bool moveToSaturationCommand(chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath,
Expand Down
5 changes: 0 additions & 5 deletions src/app/common/templates/config-data.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,6 @@ WeakEnums:
- ColorMode
- EnhancedColorMode
- HardwareFaultEnum
- HueDirection
- HueMoveMode
- HueStepMode
- IdentifyEffectIdentifier
- IdentifyEffectVariant
- IdentifyIdentifyType
Expand All @@ -21,8 +18,6 @@ WeakEnums:
- PHYRateEnum
- RadioFaultEnum
- RoutingRole
- SaturationMoveMode
- SaturationStepMode
- StepMode

DefineBitmaps:
Expand Down
Loading

0 comments on commit 6bed4b3

Please sign in to comment.