Skip to content

Commit

Permalink
Add check for command parameters or move them before any other logic …
Browse files Browse the repository at this point in the history 
…is done. Update some return status to correct error codes
  • Loading branch information
@jmartinez-silabs
jmartinez-silabs committed Mar 7, 2023
1 parent 7056239 commit 82f5581
Show file tree
Hide file tree
Showing 2 changed files with 106 additions and 63 deletions.
156 changes: 93 additions & 63 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 @@ -822,15 +822,18 @@ bool ColorControlServer::moveHueCommand(app::CommandHandler * commandObj, const

VerifyOrExit(colorHueTransitionState != nullptr, status = Status::UnsupportedEndpoint);

if (!shouldExecuteIfOff(endpoint, optionsMask, optionsOverride))
// 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))
{
commandObj->AddStatus(commandPath, Status::Success);
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
}

if (rate == 0 && moveMode != EMBER_ZCL_HUE_MOVE_MODE_STOP)
if (!shouldExecuteIfOff(endpoint, optionsMask, optionsOverride))
{
commandObj->AddStatus(commandPath, Status::InvalidCommand);
commandObj->AddStatus(commandPath, Status::Success);
return true;
}

Expand Down Expand Up @@ -884,11 +887,7 @@ bool ColorControlServer::moveHueCommand(app::CommandHandler * commandObj, const

colorHueTransitionState->up = false;
}
else
{
commandObj->AddStatus(commandPath, Status::InvalidAction);
return true;
}

colorHueTransitionState->stepsRemaining = TRANSITION_TIME_1S;
colorHueTransitionState->stepsTotal = TRANSITION_TIME_1S;
colorHueTransitionState->endpoint = endpoint;
Expand Down Expand Up @@ -933,9 +932,11 @@ bool ColorControlServer::moveToHueCommand(app::CommandHandler * commandObj, cons

VerifyOrExit(colorHueTransitionState != nullptr, status = Status::UnsupportedEndpoint);

if (!shouldExecuteIfOff(endpoint, optionsMask, optionsOverride))
// Standard Hue limit checking: hue is 0..254. Spec dictates we ignore
// this and report a constraint error.
if (!isEnhanced && (hue > MAX_HUE_VALUE))
{
commandObj->AddStatus(commandPath, Status::Success);
commandObj->AddStatus(commandPath, Status::ConstraintError);
return true;
}

Expand All @@ -956,14 +957,6 @@ bool ColorControlServer::moveToHueCommand(app::CommandHandler * commandObj, cons
transitionTime++;
}

// Standard Hue limit checking: hue is 0..254. Spec dictates we ignore
// this and report a malformed packet.
if (!isEnhanced && (hue > MAX_HUE_VALUE))
{
commandObj->AddStatus(commandPath, Status::InvalidAction);
return true;
}

// 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.
Expand Down Expand Up @@ -998,7 +991,13 @@ bool ColorControlServer::moveToHueCommand(app::CommandHandler * commandObj, cons
direction = MOVE_MODE_DOWN;
break;
default:
commandObj->AddStatus(commandPath, Status::InvalidAction);
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
}

if (!shouldExecuteIfOff(endpoint, optionsMask, optionsOverride))
{
commandObj->AddStatus(commandPath, Status::Success);
return true;
}

Expand Down Expand Up @@ -1075,16 +1074,11 @@ bool ColorControlServer::moveToHueAndSaturationCommand(app::CommandHandler * com
VerifyOrExit(colorSaturationTransitionState != nullptr, status = Status::UnsupportedEndpoint);
VerifyOrExit(colorHueTransitionState != nullptr, status = Status::UnsupportedEndpoint);

if (transitionTime == 0)
{
transitionTime++;
}

// limit checking: hue and saturation are 0..254. Spec dictates we ignore
// this and report a malformed packet.
// this and report a constraint error.
if ((!isEnhanced && hue > MAX_HUE_VALUE) || saturation > MAX_SATURATION_VALUE)
{
commandObj->AddStatus(commandPath, Status::InvalidAction);
commandObj->AddStatus(commandPath, Status::ConstraintError);
return true;
}

Expand All @@ -1094,6 +1088,11 @@ bool ColorControlServer::moveToHueAndSaturationCommand(app::CommandHandler * com
return true;
}

if (transitionTime == 0)
{
transitionTime++;
}

// New command. Need to stop any active transitions.
stopAllColorTransitions(endpoint);

Expand Down Expand Up @@ -1180,6 +1179,13 @@ bool ColorControlServer::stepHueCommand(app::CommandHandler * commandObj, const
ColorHueTransitionState * colorHueTransitionState = getColorHueTransitionState(endpoint);
VerifyOrExit(colorHueTransitionState != nullptr, status = Status::UnsupportedEndpoint);

// Confirm validity of the step mode received
if (stepMode != STEP_MODE_STOP && stepMode != STEP_MODE_UP && stepMode != STEP_MODE_DOWN)
{
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
}

if (!shouldExecuteIfOff(endpoint, optionsMask, optionsOverride))
{
commandObj->AddStatus(commandPath, Status::Success);
Expand All @@ -1194,7 +1200,7 @@ bool ColorControlServer::stepHueCommand(app::CommandHandler * commandObj, const
// New command. Need to stop any active transitions.
stopAllColorTransitions(endpoint);

if (stepMode == MOVE_MODE_STOP)
if (stepMode == STEP_MODE_STOP)
{
commandObj->AddStatus(commandPath, Status::Success);
return true;
Expand All @@ -1216,12 +1222,12 @@ bool ColorControlServer::stepHueCommand(app::CommandHandler * commandObj, const
if (isEnhanced)
{

if (stepMode == MOVE_MODE_UP)
if (stepMode == STEP_MODE_UP)
{
colorHueTransitionState->finalEnhancedHue = addEnhancedHue(colorHueTransitionState->currentEnhancedHue, stepSize);
colorHueTransitionState->up = true;
}
else
else if (stepMode == STEP_MODE_DOWN)
{
colorHueTransitionState->finalEnhancedHue = subtractEnhancedHue(colorHueTransitionState->currentEnhancedHue, stepSize);
colorHueTransitionState->up = false;
Expand All @@ -1234,7 +1240,7 @@ bool ColorControlServer::stepHueCommand(app::CommandHandler * commandObj, const
colorHueTransitionState->finalHue = addHue(colorHueTransitionState->currentHue, static_cast<uint8_t>(stepSize));
colorHueTransitionState->up = true;
}
else
else if (stepMode == STEP_MODE_DOWN)
{
colorHueTransitionState->finalHue = subtractHue(colorHueTransitionState->currentHue, static_cast<uint8_t>(stepSize));
colorHueTransitionState->up = false;
Expand Down Expand Up @@ -1269,7 +1275,10 @@ bool ColorControlServer::moveSaturationCommand(app::CommandHandler * commandObj,
Color16uTransitionState * colorSaturationTransitionState = getSaturationTransitionState(endpoint);
VerifyOrExit(colorSaturationTransitionState != nullptr, status = Status::UnsupportedEndpoint);

if (rate == 0 && moveMode != EMBER_ZCL_SATURATION_MOVE_MODE_STOP)
// 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))
{
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
Expand Down Expand Up @@ -1305,7 +1314,7 @@ bool ColorControlServer::moveSaturationCommand(app::CommandHandler * commandObj,
{
colorSaturationTransitionState->finalValue = MAX_SATURATION_VALUE;
}
else
else if (moveMode == EMBER_ZCL_SATURATION_MOVE_MODE_DOWN)
{
colorSaturationTransitionState->finalValue = MIN_SATURATION_VALUE;
}
Expand Down Expand Up @@ -1351,6 +1360,14 @@ bool ColorControlServer::moveToSaturationCommand(app::CommandHandler * commandOb
Color16uTransitionState * colorSaturationTransitionState = getSaturationTransitionState(endpoint);
VerifyOrExit(colorSaturationTransitionState != nullptr, status = Status::UnsupportedEndpoint);

// limit checking: hue and saturation are 0..254. Spec dictates we ignore
// this and report a malformed packet.
if (saturation > MAX_SATURATION_VALUE)
{
commandObj->AddStatus(commandPath, Status::ConstraintError);
return true;
}

if (!shouldExecuteIfOff(endpoint, optionsMask, optionsOverride))
{
commandObj->AddStatus(commandPath, Status::Success);
Expand All @@ -1362,14 +1379,6 @@ bool ColorControlServer::moveToSaturationCommand(app::CommandHandler * commandOb
transitionTime++;
}

// limit checking: hue and saturation are 0..254. Spec dictates we ignore
// this and report a malformed packet.
if (saturation > MAX_SATURATION_VALUE)
{
commandObj->AddStatus(commandPath, Status::InvalidAction);
return true;
}

// New command. Need to stop any active transitions.
stopAllColorTransitions(endpoint);

Expand Down Expand Up @@ -1410,6 +1419,13 @@ bool ColorControlServer::stepSaturationCommand(app::CommandHandler * commandObj,
Color16uTransitionState * colorSaturationTransitionState = getSaturationTransitionState(endpoint);
VerifyOrExit(colorSaturationTransitionState != nullptr, status = Status::UnsupportedEndpoint);

// Confirm validity of the step mode received
if (stepMode != STEP_MODE_STOP && stepMode != STEP_MODE_UP && stepMode != STEP_MODE_DOWN)
{
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
}

if (!shouldExecuteIfOff(endpoint, optionsMask, optionsOverride))
{
commandObj->AddStatus(commandPath, Status::Success);
Expand Down Expand Up @@ -1441,7 +1457,7 @@ bool ColorControlServer::stepSaturationCommand(app::CommandHandler * commandObj,
{
colorSaturationTransitionState->finalValue = addSaturation(currentSaturation, stepSize);
}
else
else if (stepMode == MOVE_MODE_DOWN)
{
colorSaturationTransitionState->finalValue = subtractSaturation(currentSaturation, stepSize);
}
Expand Down Expand Up @@ -1479,6 +1495,16 @@ bool ColorControlServer::colorLoopCommand(app::CommandHandler * commandObj, cons
ColorHueTransitionState * colorHueTransitionState = getColorHueTransitionState(endpoint);
VerifyOrExit(colorHueTransitionState != nullptr, status = Status::UnsupportedEndpoint);

// Validate the action and direction parameters of the command
if ((action != EMBER_ZCL_COLOR_LOOP_ACTION_DEACTIVATE &&
action != EMBER_ZCL_COLOR_LOOP_ACTION_ACTIVATE_FROM_COLOR_LOOP_START_ENHANCED_HUE &&
action != EMBER_ZCL_COLOR_LOOP_ACTION_ACTIVATE_FROM_ENHANCED_CURRENT_HUE) ||
(direction != DECREMENT_HUE && direction != INCREMENT_HUE))
{
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
}

if (!shouldExecuteIfOff(endpoint, optionsMask, optionsOverride))
{
commandObj->AddStatus(commandPath, Status::Success);
Expand Down Expand Up @@ -1566,11 +1592,6 @@ bool ColorControlServer::colorLoopCommand(app::CommandHandler * commandObj, cons
{
startColorLoop(endpoint, false);
}
else
{
commandObj->AddStatus(commandPath, Status::InvalidAction);
return true;
}
}

exit:
Expand Down Expand Up @@ -1846,7 +1867,7 @@ bool ColorControlServer::moveColorCommand(app::CommandHandler * commandObj, cons
else
{
colorYTransitionState->finalValue = MIN_CIE_XY_VALUE;
unsignedRate = (uint16_t)(rateY * -1);
unsignedRate = (uint16_t) (rateY * -1);
}
transitionTimeY = computeTransitionTimeFromStateAndRate(colorYTransitionState, unsignedRate);
colorYTransitionState->stepsRemaining = transitionTimeY;
Expand Down Expand Up @@ -2211,6 +2232,14 @@ bool ColorControlServer::moveColorTempCommand(app::CommandHandler * commandObj,
Color16uTransitionState * colorTempTransitionState = getTempTransitionState(endpoint);
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))
{
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
}

if (!shouldExecuteIfOff(endpoint, optionsMask, optionsOverride))
{
commandObj->AddStatus(commandPath, Status::Success);
Expand All @@ -2220,12 +2249,6 @@ bool ColorControlServer::moveColorTempCommand(app::CommandHandler * commandObj,
Attributes::ColorTempPhysicalMinMireds::Get(endpoint, &tempPhysicalMin);
Attributes::ColorTempPhysicalMaxMireds::Get(endpoint, &tempPhysicalMax);

if (rate == 0 && moveMode != MOVE_MODE_STOP)
{
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
}

// New command. Need to stop any active transitions.
stopAllColorTransitions(endpoint);

Expand Down Expand Up @@ -2330,18 +2353,17 @@ bool ColorControlServer::stepColorTempCommand(app::CommandHandler * commandObj,
Color16uTransitionState * colorTempTransitionState = getTempTransitionState(endpoint);
VerifyOrExit(colorTempTransitionState != nullptr, status = Status::UnsupportedEndpoint);

if (!shouldExecuteIfOff(endpoint, optionsMask, optionsOverride))
// Confirm validity of the step mode received
if (stepMode != STEP_MODE_STOP && stepMode != STEP_MODE_UP && stepMode != STEP_MODE_DOWN)
{
commandObj->AddStatus(commandPath, Status::Success);
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
}

Attributes::ColorTempPhysicalMinMireds::Get(endpoint, &tempPhysicalMin);
Attributes::ColorTempPhysicalMaxMireds::Get(endpoint, &tempPhysicalMax);

if (transitionTime == 0)
if (!shouldExecuteIfOff(endpoint, optionsMask, optionsOverride))
{
transitionTime++;
commandObj->AddStatus(commandPath, Status::Success);
return true;
}

// New command. Need to stop any active transitions.
Expand All @@ -2353,6 +2375,14 @@ bool ColorControlServer::stepColorTempCommand(app::CommandHandler * commandObj,
return true;
}

Attributes::ColorTempPhysicalMinMireds::Get(endpoint, &tempPhysicalMin);
Attributes::ColorTempPhysicalMaxMireds::Get(endpoint, &tempPhysicalMax);

if (transitionTime == 0)
{
transitionTime++;
}

if (colorTemperatureMinimum < tempPhysicalMin)
{
colorTemperatureMinimum = tempPhysicalMin;
Expand All @@ -2370,7 +2400,7 @@ bool ColorControlServer::stepColorTempCommand(app::CommandHandler * commandObj,
Attributes::ColorTemperatureMireds::Get(endpoint, &colorTempTransitionState->initialValue);
colorTempTransitionState->currentValue = colorTempTransitionState->initialValue;

if (stepMode == MOVE_MODE_UP)
if (stepMode == STEP_MODE_UP)
{
uint32_t finalValue32u = static_cast<uint32_t>(colorTempTransitionState->initialValue) + static_cast<uint32_t>(stepSize);
if (finalValue32u > UINT16_MAX)
Expand All @@ -2382,7 +2412,7 @@ bool ColorControlServer::stepColorTempCommand(app::CommandHandler * commandObj,
colorTempTransitionState->finalValue = static_cast<uint16_t>(finalValue32u);
}
}
else
else if (stepMode == STEP_MODE_DOWN)
{
uint32_t finalValue32u = static_cast<uint32_t>(colorTempTransitionState->initialValue) - static_cast<uint32_t>(stepSize);
if (finalValue32u > UINT16_MAX)
Expand Down
13 changes: 13 additions & 0 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 @@ -74,6 +74,19 @@ class ColorControlServer
MOVE_MODE_DOWN = 0x03
};

enum StepMode
{
STEP_MODE_STOP = 0x00,
STEP_MODE_UP = 0x01,
STEP_MODE_DOWN = 0x03
};

enum ColorLoopDirection
{
DECREMENT_HUE = 0x00,
INCREMENT_HUE = 0x01,
};

enum ColorMode
{
COLOR_MODE_HSV = 0x00,
Expand Down

0 comments on commit 82f5581

Please sign in to comment.