Skip to content

Commit

Permalink
PR review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@mkardous-silabs
mkardous-silabs committed Mar 3, 2022
1 parent 1a7fc20 commit 724a233
Show file tree
Hide file tree
Showing 5 changed files with 47 additions and 90 deletions.
2 changes: 1 addition & 1 deletion examples/light-switch-app/efr32/include/binding-handler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ void SwitchWorkerFunction(intptr_t context);

struct BindingCommandData
{
chip::EndpointId endpointId = 1;
chip::EndpointId localEndpointId = 1;
chip::CommandId commandId;
chip::ClusterId clusterId;
bool isGroup = false;
Expand Down
62 changes: 16 additions & 46 deletions examples/light-switch-app/efr32/src/binding-handler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,51 +76,30 @@ void ProcessOnOffUnicastBindingCommand(CommandId commandId, const EmberBindingTa
Controller::InvokeCommandRequest(peer_device->GetExchangeManager(), peer_device->GetSecureSession().Value(), binding.remote,
offCommand, onSuccess, onFailure);
break;

default:
ChipLogError(NotSpecified, "Invalid binding command data - commandId is not supported");
break;
}
}

void ProcessOnOffGroupBindingCommand(CommandId commandId, const EmberBindingTableEntry & binding)
{
auto onSuccess = [](const ConcreteCommandPath & commandPath, const StatusIB & status, const auto & dataResponse) {
ChipLogProgress(NotSpecified, "OnOff command succeeds");
};

auto onFailure = [](CHIP_ERROR error) {
ChipLogError(NotSpecified, "OnOff command failed: %" CHIP_ERROR_FORMAT, error.Format());
};

auto onDone = [](CommandSender * commandSender) { ChipLogError(NotSpecified, "OnOff command done"); };

NodeId sourceNodeId = Server::GetInstance().GetFabricTable().FindFabricWithIndex(binding.fabricIndex)->GetNodeId();
Messaging::ExchangeManager & exchangeMgr = Server::GetInstance().GetExchangeManager();

switch (commandId)
{
case Clusters::OnOff::Commands::Toggle::Id:
Clusters::OnOff::Commands::Toggle::Type toggleCommand;
Controller::InvokeGroupCommandRequest(&exchangeMgr, binding.fabricIndex, binding.groupId, sourceNodeId, toggleCommand,
onSuccess, onFailure, onDone);
Controller::InvokeGroupCommandRequest(&exchangeMgr, binding.fabricIndex, binding.groupId, sourceNodeId, toggleCommand);
break;

case Clusters::OnOff::Commands::On::Id:
Clusters::OnOff::Commands::On::Type onCommand;
Controller::InvokeGroupCommandRequest(&exchangeMgr, binding.fabricIndex, binding.groupId, sourceNodeId, onCommand,
onSuccess, onFailure, onDone);
Controller::InvokeGroupCommandRequest(&exchangeMgr, binding.fabricIndex, binding.groupId, sourceNodeId, onCommand);

break;

case Clusters::OnOff::Commands::Off::Id:
Clusters::OnOff::Commands::Off::Type offCommand;
Controller::InvokeGroupCommandRequest(&exchangeMgr, binding.fabricIndex, binding.groupId, sourceNodeId, offCommand,
onSuccess, onFailure, onDone);
break;

default:
ChipLogError(NotSpecified, "Invalid binding command data - commandId is not supported");
Controller::InvokeGroupCommandRequest(&exchangeMgr, binding.fabricIndex, binding.groupId, sourceNodeId, offCommand);
break;
}
}
Expand All @@ -130,31 +109,22 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DevicePro
VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "OnDeviceConnectedFn: context is null"));
BindingCommandData * data = static_cast<BindingCommandData *>(context);

if (binding.local == 1 && (!binding.clusterId.HasValue() || binding.clusterId.Value() == data->clusterId))
if (binding.type == EMBER_MULTICAST_BINDING && data->isGroup)
{
if (binding.type == EMBER_MULTICAST_BINDING && data->isGroup)
switch (data->clusterId)
{
switch (data->clusterId)
{
case Clusters::OnOff::Id:
ProcessOnOffGroupBindingCommand(data->commandId, binding);
break;
default:
ChipLogError(NotSpecified, "Invalid binding command data - clusterId is not supported");
break;
}
case Clusters::OnOff::Id:
ProcessOnOffGroupBindingCommand(data->commandId, binding);
break;
}
else if (binding.type == EMBER_UNICAST_BINDING && !data->isGroup)
}
else if (binding.type == EMBER_UNICAST_BINDING && !data->isGroup)
{
switch (data->clusterId)
{
switch (data->clusterId)
{
case Clusters::OnOff::Id:
ProcessOnOffUnicastBindingCommand(data->commandId, binding, peer_device);
break;
default:
ChipLogError(NotSpecified, "Invalid binding command data - clusterId is not supported");
break;
}
case Clusters::OnOff::Id:
ProcessOnOffUnicastBindingCommand(data->commandId, binding, peer_device);
break;
}
}
}
Expand Down Expand Up @@ -357,7 +327,7 @@ void SwitchWorkerFunction(intptr_t context)
VerifyOrReturn(context != 0, ChipLogError(NotSpecified, "SwitchWorkerFunction - Invalid work data"));

BindingCommandData * data = reinterpret_cast<BindingCommandData *>(context);
BindingManager::GetInstance().NotifyBoundClusterChanged(1, data->clusterId, static_cast<void *>(data));
BindingManager::GetInstance().NotifyBoundClusterChanged(data->localEndpointId, data->clusterId, static_cast<void *>(data));

Platform::Delete(data);
}
Expand Down
14 changes: 14 additions & 0 deletions src/app/CommandPathParams.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,20 @@ struct CommandPathParams
mEndpointId(aEndpointId),
mGroupId(aGroupId), mClusterId(aClusterId), mCommandId(aCommandId), mFlags(aFlags)
{}

CommandPathParams(uint16_t aId, ClusterId aClusterId, CommandId aCommandId, const BitFlags<CommandPathFlags> & aFlags) :
mClusterId(aClusterId), mCommandId(aCommandId), mFlags(aFlags)
{
if (aFlags == CommandPathFlags::kEndpointIdValid)
{
mEndpointId = aId;
}
else if (aFlags == CommandPathFlags::kGroupIdValid)
{
mGroupId = aId;
}
}

bool IsSamePath(const CommandPathParams & other) const
{
if (other.mClusterId != mClusterId || other.mCommandId != mCommandId)
Expand Down
2 changes: 2 additions & 0 deletions src/app/CommandSender.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@ class CommandSender final : public Messaging::ExchangeDelegate
* Constructor.
*
* The callback passed in has to outlive this CommandSender object.
* If used in a groups setting, callbacks do not need to be passed.
* If callbacks are passed the only one that will be called in a group sesttings is the onDone
*/
CommandSender(Callback * apCallback, Messaging::ExchangeManager * apExchangeMgr, bool aIsTimedRequest = false);
CHIP_ERROR PrepareCommand(const CommandPathParams & aCommandPathParams, bool aStartDataStruct = true);
Expand Down
57 changes: 14 additions & 43 deletions src/controller/InvokeInteraction.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,55 +102,26 @@ InvokeCommandRequest(Messaging::ExchangeManager * aExchangeMgr, const SessionHan
* function
*/
template <typename RequestObjectT>
CHIP_ERROR
InvokeGroupCommandRequest(Messaging::ExchangeManager * exchangeMgr, chip::FabricIndex fabric, chip::GroupId groupId,
chip::NodeId sourceNodeId, const RequestObjectT & requestCommandData,
typename TypedCommandCallback<typename RequestObjectT::ResponseType>::OnSuccessCallbackType onSuccessCb,
typename TypedCommandCallback<typename RequestObjectT::ResponseType>::OnErrorCallbackType onErrorCb,
typename TypedCommandCallback<typename RequestObjectT::ResponseType>::OnDoneCallbackType onDoneCb)
CHIP_ERROR InvokeGroupCommandRequest(Messaging::ExchangeManager * exchangeMgr, chip::FabricIndex fabric, chip::GroupId groupId,
chip::NodeId sourceNodeId, const RequestObjectT & requestCommandData)
{
app::CommandPathParams commandPath = { 0 /* endpoint */, groupId, RequestObjectT::GetClusterId(),
RequestObjectT::GetCommandId(), (app::CommandPathFlags::kGroupIdValid) };

// Let's create a handle version of the decoder to ensure we do correct clean-up of it if things go south at any point below
auto decoder = chip::Platform::MakeUnique<TypedCommandCallback<typename RequestObjectT::ResponseType>>(onSuccessCb, onErrorCb);
VerifyOrReturnError(decoder != nullptr, CHIP_ERROR_NO_MEMORY);
CHIP_ERROR error = CHIP_NO_ERROR;
app::CommandPathParams commandPath = { groupId, RequestObjectT::GetClusterId(), RequestObjectT::GetCommandId(),
app::CommandPathFlags::kGroupIdValid };
Transport::OutgoingGroupSession session(groupId, fabric, sourceNodeId);

// Upon successful completion of SendCommandRequest below, we're expected to free up the respective allocated objects
// in the OnDone callback.
auto onDone = [rawDecoderPtr = decoder.get(), onDoneCallback = onDoneCb](app::CommandSender * commandSender) {
if (onDoneCallback != nullptr)
{
onDoneCallback(commandSender);
}

chip::Platform::Delete(commandSender);
chip::Platform::Delete(rawDecoderPtr);
};

decoder->SetOnDoneCallback(onDone);

auto commandSender = chip::Platform::MakeUnique<app::CommandSender>(decoder.get(), exchangeMgr);
auto commandSender = chip::Platform::MakeUnique<app::CommandSender>(nullptr, exchangeMgr);
VerifyOrReturnError(commandSender != nullptr, CHIP_ERROR_NO_MEMORY);

ReturnErrorOnFailure(commandSender->AddRequestData(commandPath, requestCommandData));

Optional<SessionHandle> session = exchangeMgr->GetSessionManager()->CreateGroupSession(groupId, fabric, sourceNodeId);
if (!session.HasValue())
{
return CHIP_ERROR_NO_MEMORY;
}
error = commandSender->AddRequestData(commandPath, requestCommandData);
SuccessOrExit(error);

// decoder and commandSender will be deleted by the onDone call before the return of SendGroupCommandRequest
// decoder and commandSender should not be used after the SendGroupCommandRequest call
ReturnErrorOnFailure(commandSender->SendGroupCommandRequest(session.Value()));
error = commandSender->SendGroupCommandRequest(SessionHandle(session));
SuccessOrExit(error);

// decoder and commandSender are already deleted and are not to be used
commandSender.release();
decoder.release();

exchangeMgr->GetSessionManager()->RemoveGroupSession(session.Value()->AsGroupSession());
return CHIP_NO_ERROR;
exit:
chip::Platform::Delete(commandSender.release());
return error;
}

template <typename RequestObjectT>
Expand Down

0 comments on commit 724a233

Please sign in to comment.