From 6f6748aa5623695fcdb849fcc3a3a55912dc6f95 Mon Sep 17 00:00:00 2001 From: yunhanw-google Date: Thu, 30 Nov 2023 10:34:00 -0800 Subject: [PATCH] [Android] Fix onReport (#30724) * return when OnError happens in OnReportEnd * onReport and onError needs to be mutual exclusive * fix typo * Restyled by clang-format --------- Co-authored-by: Restyled.io --- src/app/BufferedReadCallback.cpp | 1 + src/app/ConcreteAttributePath.h | 6 ++ src/app/EventHeader.h | 6 ++ src/controller/java/AndroidCallbacks.cpp | 86 ++++++++++++------------ 4 files changed, 55 insertions(+), 44 deletions(-) diff --git a/src/app/BufferedReadCallback.cpp b/src/app/BufferedReadCallback.cpp index 0279eff5eeab73..da38d0b38418bb 100644 --- a/src/app/BufferedReadCallback.cpp +++ b/src/app/BufferedReadCallback.cpp @@ -40,6 +40,7 @@ void BufferedReadCallback::OnReportEnd() if (err != CHIP_NO_ERROR) { mCallback.OnError(err); + return; } mCallback.OnReportEnd(); diff --git a/src/app/ConcreteAttributePath.h b/src/app/ConcreteAttributePath.h index b06a4b8a3b3acf..076575dcafa2a1 100644 --- a/src/app/ConcreteAttributePath.h +++ b/src/app/ConcreteAttributePath.h @@ -132,6 +132,12 @@ struct ConcreteDataAttributePath : public ConcreteAttributePath bool IsListOperation() const { return mListOp != ListOperation::NotList; } bool IsListItemOperation() const { return ((mListOp != ListOperation::NotList) && (mListOp != ListOperation::ReplaceAll)); } + void LogPath() const + { + ChipLogProgress(DataManagement, "Concrete Attribute Path: (%d, " ChipLogFormatMEI ", " ChipLogFormatMEI ") ", mEndpointId, + ChipLogValueMEI(mClusterId), ChipLogValueMEI(mAttributeId)); + } + // // This index is only valid if `mListOp` is set to a list item operation, i.e // ReplaceItem, DeleteItem or AppendItem. Otherwise, it is to be ignored. diff --git a/src/app/EventHeader.h b/src/app/EventHeader.h index c3ba210bfbfa5d..72887962a9348e 100644 --- a/src/app/EventHeader.h +++ b/src/app/EventHeader.h @@ -30,6 +30,12 @@ struct EventHeader EventNumber mEventNumber = 0; PriorityLevel mPriorityLevel = PriorityLevel::Invalid; Timestamp mTimestamp; + + void LogPath() const + { + ChipLogProgress(DataManagement, "Concrete Event Path: (%d, " ChipLogFormatMEI ", " ChipLogFormatMEI ") ", mPath.mEndpointId, + ChipLogValueMEI(mPath.mClusterId), ChipLogValueMEI(mPath.mEventId)); + } }; } // namespace app } // namespace chip diff --git a/src/controller/java/AndroidCallbacks.cpp b/src/controller/java/AndroidCallbacks.cpp index fd6b043beacb34..85c948404343ab 100644 --- a/src/controller/java/AndroidCallbacks.cpp +++ b/src/controller/java/AndroidCallbacks.cpp @@ -280,23 +280,10 @@ void ReportCallback::OnAttributeData(const app::ConcreteDataAttributePath & aPat err = CreateChipAttributePath(env, aPath, attributePathObj); VerifyOrReturn(err == CHIP_NO_ERROR, ChipLogError(Controller, "Unable to create Java ChipAttributePath: %s", ErrorStr(err))); - if (aPath.IsListItemOperation()) - { - ReportError(attributePathObj, nullptr, CHIP_ERROR_INCORRECT_STATE); - return; - } - - if (aStatus.mStatus != Protocols::InteractionModel::Status::Success) - { - ReportError(attributePathObj, nullptr, aStatus.mStatus); - return; - } - - if (apData == nullptr) - { - ReportError(attributePathObj, nullptr, CHIP_ERROR_INVALID_ARGUMENT); - return; - } + VerifyOrReturn(!aPath.IsListItemOperation(), ChipLogError(Controller, "Expect non-list item operation"); aPath.LogPath()); + VerifyOrReturn(aStatus.IsSuccess(), ChipLogError(Controller, "Receive bad status %s", ErrorStr(aStatus.ToChipError())); + aPath.LogPath()); + VerifyOrReturn(apData != nullptr, ChipLogError(Controller, "Receive empty apData"); aPath.LogPath()); TLV::TLVReader readerForJavaTLV; readerForJavaTLV.Init(*apData); @@ -313,9 +300,9 @@ void ReportCallback::OnAttributeData(const app::ConcreteDataAttributePath & aPat err = CHIP_NO_ERROR; } - VerifyOrReturn(err == CHIP_NO_ERROR, ReportError(attributePathObj, nullptr, err)); - VerifyOrReturn(!env->ExceptionCheck(), env->ExceptionDescribe(), - ReportError(attributePathObj, nullptr, CHIP_JNI_ERROR_EXCEPTION_THROWN)); + VerifyOrReturn(err == CHIP_NO_ERROR, ChipLogError(Controller, "Fail to decode attribute with error %s", ErrorStr(err)); + aPath.LogPath()); + VerifyOrReturn(!env->ExceptionCheck(), env->ExceptionDescribe()); #endif // Create TLV byte array to pass to Java layer size_t bufferLen = readerForJavaTLV.GetRemainingLength() + readerForJavaTLV.GetLengthRead(); @@ -328,33 +315,41 @@ void ReportCallback::OnAttributeData(const app::ConcreteDataAttributePath & aPat TLV::TLVWriter writer; writer.Init(buffer.get(), bufferLen); err = writer.CopyElement(TLV::AnonymousTag(), readerForJavaTLV); - VerifyOrReturn(err == CHIP_NO_ERROR, ReportError(attributePathObj, nullptr, err)); + VerifyOrReturn(err == CHIP_NO_ERROR, ChipLogError(Controller, "Fail to copy tlv element with error %s", ErrorStr(err)); + aPath.LogPath()); size = writer.GetLengthWritten(); chip::ByteArray jniByteArray(env, reinterpret_cast(buffer.get()), size); // Convert TLV to JSON std::string json; err = ConvertReportTlvToJson(static_cast(aPath.mAttributeId), *apData, json); - VerifyOrReturn(err == CHIP_NO_ERROR, ReportError(attributePathObj, nullptr, err)); + VerifyOrReturn(err == CHIP_NO_ERROR, + ChipLogError(Controller, "Fail to convert report tlv to json with error %s", ErrorStr(err)); + aPath.LogPath()); UtfString jsonString(env, json.c_str()); // Create AttributeState object jclass attributeStateCls; err = JniReferences::GetInstance().GetLocalClassRef(env, "chip/devicecontroller/model/AttributeState", attributeStateCls); - VerifyOrReturn(err == CHIP_NO_ERROR, ChipLogError(Controller, "Could not find AttributeState class")); - VerifyOrReturn(attributeStateCls != nullptr, ChipLogError(Controller, "Could not find AttributeState class")); + VerifyOrReturn(err == CHIP_NO_ERROR, + ChipLogError(Controller, "Could not find AttributeState class with error %s", ErrorStr(err)); + aPath.LogPath()); + VerifyOrReturn(attributeStateCls != nullptr, ChipLogError(Controller, "Could not find AttributeState class"); aPath.LogPath()); jmethodID attributeStateCtor = env->GetMethodID(attributeStateCls, "", "(Ljava/lang/Object;[BLjava/lang/String;)V"); - VerifyOrReturn(attributeStateCtor != nullptr, ChipLogError(Controller, "Could not find AttributeState constructor")); + VerifyOrReturn(attributeStateCtor != nullptr, ChipLogError(Controller, "Could not find AttributeState constructor"); + aPath.LogPath()); jobject attributeStateObj = env->NewObject(attributeStateCls, attributeStateCtor, value, jniByteArray.jniValue(), jsonString.jniValue()); - VerifyOrReturn(attributeStateObj != nullptr, ChipLogError(Controller, "Could not create AttributeState object")); + VerifyOrReturn(attributeStateObj != nullptr, ChipLogError(Controller, "Could not create AttributeState object"); + aPath.LogPath()); jobject nodeState = mNodeStateObj.ObjectRef(); // Add AttributeState to NodeState jmethodID addAttributeMethod; err = JniReferences::GetInstance().FindMethod(env, nodeState, "addAttribute", "(IJJLchip/devicecontroller/model/AttributeState;)V", &addAttributeMethod); - VerifyOrReturn(err == CHIP_NO_ERROR, ChipLogError(Controller, "Could not find addAttribute method")); + VerifyOrReturn(err == CHIP_NO_ERROR, + ChipLogError(Controller, "Could not find addAttribute method with error %s", ErrorStr(err))); env->CallVoidMethod(nodeState, addAttributeMethod, static_cast(aPath.mEndpointId), static_cast(aPath.mClusterId), static_cast(aPath.mAttributeId), attributeStateObj); VerifyOrReturn(!env->ExceptionCheck(), env->ExceptionDescribe()); @@ -408,11 +403,7 @@ void ReportCallback::OnEventData(const app::EventHeader & aEventHeader, TLV::TLV err = CreateChipEventPath(env, aEventHeader.mPath, eventPathObj); VerifyOrReturn(err == CHIP_NO_ERROR, ChipLogError(Controller, "Unable to create Java ChipEventPath: %s", ErrorStr(err))); - if (apData == nullptr) - { - ReportError(nullptr, eventPathObj, CHIP_ERROR_INVALID_ARGUMENT); - return; - } + VerifyOrReturn(apData != nullptr, ChipLogError(Controller, "Receive empty apData"); aEventHeader.LogPath()); TLV::TLVReader readerForJavaTLV; readerForJavaTLV.Init(*apData); @@ -433,7 +424,7 @@ void ReportCallback::OnEventData(const app::EventHeader & aEventHeader, TLV::TLV else { ChipLogError(Controller, "Unsupported event timestamp type"); - ReportError(nullptr, eventPathObj, CHIP_ERROR_INVALID_ARGUMENT); + aEventHeader.LogPath(); return; } @@ -447,9 +438,9 @@ void ReportCallback::OnEventData(const app::EventHeader & aEventHeader, TLV::TLV { err = CHIP_NO_ERROR; } - VerifyOrReturn(err == CHIP_NO_ERROR, ReportError(nullptr, eventPathObj, err)); - VerifyOrReturn(!env->ExceptionCheck(), env->ExceptionDescribe(), - ReportError(nullptr, eventPathObj, CHIP_JNI_ERROR_EXCEPTION_THROWN)); + VerifyOrReturn(err == CHIP_NO_ERROR, ChipLogError(Controller, "Fail to decode event with error %s", ErrorStr(err)); + aEventHeader.LogPath()); + VerifyOrReturn(!env->ExceptionCheck(), env->ExceptionDescribe()); #endif // Create TLV byte array to pass to Java layer @@ -462,37 +453,44 @@ void ReportCallback::OnEventData(const app::EventHeader & aEventHeader, TLV::TLV TLV::TLVWriter writer; writer.Init(buffer.get(), bufferLen); err = writer.CopyElement(TLV::AnonymousTag(), readerForJavaTLV); - VerifyOrReturn(err == CHIP_NO_ERROR, ReportError(nullptr, eventPathObj, err)); + + VerifyOrReturn(err == CHIP_NO_ERROR, ChipLogError(Controller, "Fail to copy element with error %s", ErrorStr(err)); + aEventHeader.LogPath()); size = writer.GetLengthWritten(); chip::ByteArray jniByteArray(env, reinterpret_cast(buffer.get()), size); // Convert TLV to JSON std::string json; err = ConvertReportTlvToJson(static_cast(aEventHeader.mPath.mEventId), *apData, json); - VerifyOrReturn(err == CHIP_NO_ERROR, ReportError(eventPathObj, nullptr, err)); + VerifyOrReturn(err == CHIP_NO_ERROR, + ChipLogError(Controller, "Fail to convert report tlv to Json with error %s", ErrorStr(err)); + aEventHeader.LogPath()); UtfString jsonString(env, json.c_str()); // Create EventState object jclass eventStateCls; err = JniReferences::GetInstance().GetLocalClassRef(env, "chip/devicecontroller/model/EventState", eventStateCls); - VerifyOrReturn(err == CHIP_NO_ERROR, ChipLogError(Controller, "Failed to find EventState class")); - VerifyOrReturn(eventStateCls != nullptr, ChipLogError(Controller, "Could not find EventState class")); + VerifyOrReturn(err == CHIP_NO_ERROR, ChipLogError(Controller, "Failed to find EventState class"); aEventHeader.LogPath()); + VerifyOrReturn(eventStateCls != nullptr, ChipLogError(Controller, "Could not find EventState class"); aEventHeader.LogPath()); jmethodID eventStateCtor = env->GetMethodID(eventStateCls, "", "(JIIJLjava/lang/Object;[BLjava/lang/String;)V"); - VerifyOrReturn(eventStateCtor != nullptr, ChipLogError(Controller, "Could not find EventState constructor")); + VerifyOrReturn(eventStateCtor != nullptr, ChipLogError(Controller, "Could not find EventState constructor"); + aEventHeader.LogPath()); jobject eventStateObj = env->NewObject(eventStateCls, eventStateCtor, eventNumber, priorityLevel, timestampType, timestampValue, value, jniByteArray.jniValue(), jsonString.jniValue()); - VerifyOrReturn(eventStateObj != nullptr, ChipLogError(Controller, "Could not create EventState object")); + VerifyOrReturn(eventStateObj != nullptr, ChipLogError(Controller, "Could not create EventState object"); + aEventHeader.LogPath()); // Add EventState to NodeState jmethodID addEventMethod; jobject nodeState = mNodeStateObj.ObjectRef(); err = JniReferences::GetInstance().FindMethod(env, nodeState, "addEvent", "(IJJLchip/devicecontroller/model/EventState;)V", &addEventMethod); - VerifyOrReturn(err == CHIP_NO_ERROR, ChipLogError(Controller, "Could not find addEvent method")); + VerifyOrReturn(err == CHIP_NO_ERROR, ChipLogError(Controller, "Could not find addEvent method with error %s", ErrorStr(err)); + aEventHeader.LogPath()); env->CallVoidMethod(nodeState, addEventMethod, static_cast(aEventHeader.mPath.mEndpointId), static_cast(aEventHeader.mPath.mClusterId), static_cast(aEventHeader.mPath.mEventId), eventStateObj); - VerifyOrReturn(!env->ExceptionCheck(), env->ExceptionDescribe()); + VerifyOrReturn(!env->ExceptionCheck(), env->ExceptionDescribe(); aEventHeader.LogPath()); } CHIP_ERROR InvokeCallback::CreateInvokeElement(JNIEnv * env, const app::ConcreteCommandPath & aPath, TLV::TLVReader * apData,