From 052e242ecdd95e6b5066b49360579a87e09cebca Mon Sep 17 00:00:00 2001 From: Justin Wood Date: Wed, 27 Jul 2022 19:14:00 -0700 Subject: [PATCH] Stop calling the attribute change callback on the wrong thread in bridge-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 https://github.com/project-chip/connectedhomeip/issues/21212 Co-authored-by: Boris Zbarsky --- examples/bridge-app/esp32/main/main.cpp | 57 ++++++++++++------------ examples/bridge-app/linux/main.cpp | 58 ++++++++++++++----------- 2 files changed, 61 insertions(+), 54 deletions(-) diff --git a/examples/bridge-app/esp32/main/main.cpp b/examples/bridge-app/esp32/main/main.cpp index 0d022828740cfb..17f0e15568e768 100644 --- a/examples/bridge-app/esp32/main/main.cpp +++ b/examples/bridge-app/esp32/main/main.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -29,8 +30,10 @@ #include #include #include +#include #include #include +#include #include @@ -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(len); - memcpy(&zclString[1], cString, zclString[0]); - return zclString; -} - EmberAfStatus HandleReadBridgedDeviceBasicAttribute(Device * dev, chip::AttributeId attributeId, uint8_t * buffer, uint16_t maxReadLength) { @@ -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(maxReadLength - 1)); + MutableByteSpan zclNameSpan(buffer, maxReadLength); + MakeZclCharString(zclNameSpan, dev->GetName()); } else if ((attributeId == ZCL_CLUSTER_REVISION_SERVER_ATTRIBUTE_ID) && (maxReadLength == 2)) { @@ -304,28 +295,36 @@ EmberAfStatus emberAfExternalAttributeWriteCallback(EndpointId endpoint, Cluster return EMBER_ZCL_STATUS_FAILURE; } +namespace { +void CallReportingCallback(intptr_t closure) +{ + auto path = reinterpret_cast(closure); + MatterReportingAttributeChangeCallback(*path); + Platform::Delete(path); +} + +void ScheduleReportingCallback(Device * dev, ClusterId cluster, AttributeId attribute) +{ + auto * path = Platform::New(dev->GetEndpointId(), cluster, attribute); + DeviceLayer::PlatformMgr().ScheduleWork(CallReportingCallback, reinterpret_cast(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); } } @@ -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); diff --git a/examples/bridge-app/linux/main.cpp b/examples/bridge-app/linux/main.cpp index 71486c8fc392bd..58e804782ddf1d 100644 --- a/examples/bridge-app/linux/main.cpp +++ b/examples/bridge-app/linux/main.cpp @@ -24,6 +24,7 @@ #include #include +#include #include #include #include @@ -377,22 +378,31 @@ std::vector GetActionListInfo(chip::EndpointId parentId) return gActions; } +namespace { +void CallReportingCallback(intptr_t closure) +{ + auto path = reinterpret_cast(closure); + MatterReportingAttributeChangeCallback(*path); + Platform::Delete(path); +} + +void ScheduleReportingCallback(Device * dev, ClusterId cluster, AttributeId attribute) +{ + auto * path = Platform::New(dev->GetEndpointId(), cluster, attribute); + PlatformMgr().ScheduleWork(CallReportingCallback, reinterpret_cast(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); } } @@ -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); } } @@ -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); @@ -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;