Skip to content

Commit

Permalink
Stop calling the attribute change callback on the wrong thread in bri…
Browse files Browse the repository at this point in the history 
…dge-app. (#21230) (#21309)

A few changes here:

1. Stop trying to pass values to the attribute change callback, since those get
   ignored anyway.
2. Run the attribute change callback off a task on the Matter thread, instead of
   calling it directly from a different thread.
3. Change initial bridge setup to set state before setting change callbacks, so
   we won't try to notify changes at startup before the Matter stack is actually
   initialized.

Fixes #21212

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
  • Loading branch information
@woody-apple @bzbarsky-apple
woody-apple and bzbarsky-apple authored Jul 28, 2022
1 parent 4fad970 commit 052e242
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 54 deletions.
57 changes: 28 additions & 29 deletions examples/bridge-app/esp32/main/main.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,18 @@
#include <app-common/zap-generated/af-structs.h>
#include <app-common/zap-generated/attribute-id.h>
#include <app-common/zap-generated/cluster-id.h>
#include <app/ConcreteAttributePath.h>
#include <app/clusters/identify-server/identify-server.h>
#include <app/reporting/reporting.h>
#include <app/util/attribute-storage.h>
#include <common/Esp32AppServer.h>
#include <credentials/DeviceAttestationCredsProvider.h>
#include <credentials/examples/DeviceAttestationCredsExample.h>
#include <lib/core/CHIPError.h>
#include <lib/support/CHIPMem.h>
#include <lib/support/CHIPMemString.h>
#include <lib/support/ErrorStr.h>
#include <lib/support/ZclString.h>

#include <app/server/Server.h>

Expand Down Expand Up @@ -196,19 +199,6 @@ CHIP_ERROR RemoveDeviceEndpoint(Device * dev)
return CHIP_ERROR_INTERNAL;
}

// ZCL format -> (len, string)
uint8_t * ToZclCharString(uint8_t * zclString, const char * cString, uint8_t maxLength)
{
size_t len = strlen(cString);
if (len > maxLength)
{
len = maxLength;
}
zclString[0] = static_cast<uint8_t>(len);
memcpy(&zclString[1], cString, zclString[0]);
return zclString;
}

EmberAfStatus HandleReadBridgedDeviceBasicAttribute(Device * dev, chip::AttributeId attributeId, uint8_t * buffer,
uint16_t maxReadLength)
{
Expand All @@ -220,7 +210,8 @@ EmberAfStatus HandleReadBridgedDeviceBasicAttribute(Device * dev, chip::Attribut
}
else if ((attributeId == ZCL_NODE_LABEL_ATTRIBUTE_ID) && (maxReadLength == 32))
{
ToZclCharString(buffer, dev->GetName(), static_cast<uint8_t>(maxReadLength - 1));
MutableByteSpan zclNameSpan(buffer, maxReadLength);
MakeZclCharString(zclNameSpan, dev->GetName());
}
else if ((attributeId == ZCL_CLUSTER_REVISION_SERVER_ATTRIBUTE_ID) && (maxReadLength == 2))
{
Expand Down Expand Up @@ -304,28 +295,36 @@ EmberAfStatus emberAfExternalAttributeWriteCallback(EndpointId endpoint, Cluster
return EMBER_ZCL_STATUS_FAILURE;
}

namespace {
void CallReportingCallback(intptr_t closure)
{
auto path = reinterpret_cast<app::ConcreteAttributePath *>(closure);
MatterReportingAttributeChangeCallback(*path);
Platform::Delete(path);
}

void ScheduleReportingCallback(Device * dev, ClusterId cluster, AttributeId attribute)
{
auto * path = Platform::New<app::ConcreteAttributePath>(dev->GetEndpointId(), cluster, attribute);
DeviceLayer::PlatformMgr().ScheduleWork(CallReportingCallback, reinterpret_cast<intptr_t>(path));
}
} // anonymous namespace

void HandleDeviceStatusChanged(Device * dev, Device::Changed_t itemChangedMask)
{
if (itemChangedMask & Device::kChanged_Reachable)
{
uint8_t reachable = dev->IsReachable() ? 1 : 0;
MatterReportingAttributeChangeCallback(dev->GetEndpointId(), ZCL_BRIDGED_DEVICE_BASIC_CLUSTER_ID,
ZCL_REACHABLE_ATTRIBUTE_ID, ZCL_BOOLEAN_ATTRIBUTE_TYPE, &reachable);
ScheduleReportingCallback(dev, BridgedDeviceBasic::Id, BridgedDeviceBasic::Attributes::Reachable::Id);
}

if (itemChangedMask & Device::kChanged_State)
{
uint8_t isOn = dev->IsOn() ? 1 : 0;
MatterReportingAttributeChangeCallback(dev->GetEndpointId(), ZCL_ON_OFF_CLUSTER_ID, ZCL_ON_OFF_ATTRIBUTE_ID,
ZCL_BOOLEAN_ATTRIBUTE_TYPE, &isOn);
ScheduleReportingCallback(dev, OnOff::Id, OnOff::Attributes::OnOff::Id);
}

if (itemChangedMask & Device::kChanged_Name)
{
uint8_t zclName[kNodeLabelSize + 1];
ToZclCharString(zclName, dev->GetName(), kNodeLabelSize);
MatterReportingAttributeChangeCallback(dev->GetEndpointId(), ZCL_BRIDGED_DEVICE_BASIC_CLUSTER_ID,
ZCL_NODE_LABEL_ATTRIBUTE_ID, ZCL_CHAR_STRING_ATTRIBUTE_TYPE, zclName);
ScheduleReportingCallback(dev, BridgedDeviceBasic::Id, BridgedDeviceBasic::Attributes::NodeLabel::Id);
}
}

Expand Down Expand Up @@ -400,17 +399,17 @@ extern "C" void app_main()
// Clear database
memset(gDevices, 0, sizeof(gDevices));

gLight1.SetReachable(true);
gLight2.SetReachable(true);
gLight3.SetReachable(true);
gLight4.SetReachable(true);

// Whenever bridged device changes its state
gLight1.SetChangeCallback(&HandleDeviceStatusChanged);
gLight2.SetChangeCallback(&HandleDeviceStatusChanged);
gLight3.SetChangeCallback(&HandleDeviceStatusChanged);
gLight4.SetChangeCallback(&HandleDeviceStatusChanged);

gLight1.SetReachable(true);
gLight2.SetReachable(true);
gLight3.SetReachable(true);
gLight4.SetReachable(true);

CHIPDeviceManager & deviceMgr = CHIPDeviceManager::GetInstance();

chip_err = deviceMgr.Init(&AppCallback);
Expand Down
58 changes: 33 additions & 25 deletions examples/bridge-app/linux/main.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

#include <app-common/zap-generated/attribute-id.h>
#include <app-common/zap-generated/cluster-id.h>
#include <app/ConcreteAttributePath.h>
#include <app/EventLogging.h>
#include <app/chip-zcl-zpro-codec.h>
#include <app/reporting/reporting.h>
Expand Down Expand Up @@ -377,22 +378,31 @@ std::vector<Action *> GetActionListInfo(chip::EndpointId parentId)
return gActions;
}

namespace {
void CallReportingCallback(intptr_t closure)
{
auto path = reinterpret_cast<app::ConcreteAttributePath *>(closure);
MatterReportingAttributeChangeCallback(*path);
Platform::Delete(path);
}

void ScheduleReportingCallback(Device * dev, ClusterId cluster, AttributeId attribute)
{
auto * path = Platform::New<app::ConcreteAttributePath>(dev->GetEndpointId(), cluster, attribute);
PlatformMgr().ScheduleWork(CallReportingCallback, reinterpret_cast<intptr_t>(path));
}
} // anonymous namespace

void HandleDeviceStatusChanged(Device * dev, Device::Changed_t itemChangedMask)
{
if (itemChangedMask & Device::kChanged_Reachable)
{
uint8_t reachable = dev->IsReachable() ? 1 : 0;
MatterReportingAttributeChangeCallback(dev->GetEndpointId(), ZCL_BRIDGED_DEVICE_BASIC_CLUSTER_ID,
ZCL_REACHABLE_ATTRIBUTE_ID, ZCL_BOOLEAN_ATTRIBUTE_TYPE, &reachable);
ScheduleReportingCallback(dev, BridgedDeviceBasic::Id, BridgedDeviceBasic::Attributes::Reachable::Id);
}

if (itemChangedMask & Device::kChanged_Name)
{
uint8_t zclName[kNodeLabelSize];
MutableByteSpan zclNameSpan(zclName);
MakeZclCharString(zclNameSpan, dev->GetName());
MatterReportingAttributeChangeCallback(dev->GetEndpointId(), ZCL_BRIDGED_DEVICE_BASIC_CLUSTER_ID,
ZCL_NODE_LABEL_ATTRIBUTE_ID, ZCL_CHAR_STRING_ATTRIBUTE_TYPE, zclNameSpan.data());
ScheduleReportingCallback(dev, BridgedDeviceBasic::Id, BridgedDeviceBasic::Attributes::NodeLabel::Id);
}
}

Expand All @@ -405,9 +415,7 @@ void HandleDeviceOnOffStatusChanged(DeviceOnOff * dev, DeviceOnOff::Changed_t it

if (itemChangedMask & DeviceOnOff::kChanged_OnOff)
{
uint8_t isOn = dev->IsOn() ? 1 : 0;
MatterReportingAttributeChangeCallback(dev->GetEndpointId(), ZCL_ON_OFF_CLUSTER_ID, ZCL_ON_OFF_ATTRIBUTE_ID,
ZCL_BOOLEAN_ATTRIBUTE_TYPE, &isOn);
ScheduleReportingCallback(dev, OnOff::Id, OnOff::Attributes::OnOff::Id);
}
}

Expand Down Expand Up @@ -872,29 +880,29 @@ int main(int argc, char * argv[])
memset(gDevices, 0, sizeof(gDevices));

// Setup Mock Devices
Light1.SetChangeCallback(&HandleDeviceOnOffStatusChanged);
Light2.SetChangeCallback(&HandleDeviceOnOffStatusChanged);

Light1.SetReachable(true);
Light2.SetReachable(true);

Switch1.SetChangeCallback(&HandleDeviceSwitchStatusChanged);
Switch2.SetChangeCallback(&HandleDeviceSwitchStatusChanged);
Light1.SetChangeCallback(&HandleDeviceOnOffStatusChanged);
Light2.SetChangeCallback(&HandleDeviceOnOffStatusChanged);

Switch1.SetReachable(true);
Switch2.SetReachable(true);

// Setup devices for action cluster tests
ActionLight1.SetChangeCallback(&HandleDeviceOnOffStatusChanged);
ActionLight2.SetChangeCallback(&HandleDeviceOnOffStatusChanged);
ActionLight3.SetChangeCallback(&HandleDeviceOnOffStatusChanged);
ActionLight4.SetChangeCallback(&HandleDeviceOnOffStatusChanged);
Switch1.SetChangeCallback(&HandleDeviceSwitchStatusChanged);
Switch2.SetChangeCallback(&HandleDeviceSwitchStatusChanged);

// Setup devices for action cluster tests
ActionLight1.SetReachable(true);
ActionLight2.SetReachable(true);
ActionLight3.SetReachable(true);
ActionLight4.SetReachable(true);

ActionLight1.SetChangeCallback(&HandleDeviceOnOffStatusChanged);
ActionLight2.SetChangeCallback(&HandleDeviceOnOffStatusChanged);
ActionLight3.SetChangeCallback(&HandleDeviceOnOffStatusChanged);
ActionLight4.SetChangeCallback(&HandleDeviceOnOffStatusChanged);

// Define composed device with two switches
ComposedDevice ComposedDevice("Composed Switcher", "Bedroom");
DeviceSwitch ComposedSwitch1("Composed Switch 1", "Bedroom", EMBER_AF_SWITCH_FEATURE_LATCHING_SWITCH);
Expand All @@ -904,16 +912,16 @@ int main(int argc, char * argv[])
EMBER_AF_SWITCH_FEATURE_MOMENTARY_SWITCH_MULTI_PRESS);
DevicePowerSource ComposedPowerSource("Composed Power Source", "Bedroom", EMBER_AF_POWER_SOURCE_FEATURE_BATTERY);

ComposedSwitch1.SetChangeCallback(&HandleDeviceSwitchStatusChanged);
ComposedSwitch2.SetChangeCallback(&HandleDeviceSwitchStatusChanged);
ComposedPowerSource.SetChangeCallback(&HandleDevicePowerSourceStatusChanged);

ComposedDevice.SetReachable(true);
ComposedSwitch1.SetReachable(true);
ComposedSwitch2.SetReachable(true);
ComposedPowerSource.SetReachable(true);
ComposedPowerSource.SetBatChargeLevel(58);

ComposedSwitch1.SetChangeCallback(&HandleDeviceSwitchStatusChanged);
ComposedSwitch2.SetChangeCallback(&HandleDeviceSwitchStatusChanged);
ComposedPowerSource.SetChangeCallback(&HandleDevicePowerSourceStatusChanged);

if (ChipLinuxAppInit(argc, argv) != 0)
{
return -1;
Expand Down

0 comments on commit 052e242

Please sign in to comment.