[Create Test] Creating IDM_3_2 python3 test module #41066
There are no files selected for viewing
bzbarsky-apple marked this conversation as resolved.
Show resolved
Hide resolved
|
bzbarsky-apple marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. I did not check all the FFI bits in here carefully.... I am assuming @cecille will or has. |
|
There was a problem hiding this comment. Glanced over this, and seems ok, but did not review carefully. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -267,6 +267,9 @@ PyChipError pychip_WriteClient_WriteAttributes(void * appContext, DeviceProxy * | |
| size_t interactionTimeoutMsSizeT, size_t busyWaitMsSizeT, | ||
| chip::python::PyWriteAttributeData * writeAttributesData, size_t attributeDataLength, | ||
| bool forceLegacyListEncoding); | ||
| PyChipError pychip_WriteClient_TestOnlyWriteAttributesTimedRequestNoTimedAction( | ||
| void * appContext, DeviceProxy * device, size_t interactionTimeoutMsSizeT, size_t busyWaitMsSizeT, | ||
| chip::python::PyWriteAttributeData * writeAttributesData, size_t attributeDataLength); | ||
| PyChipError pychip_WriteClient_WriteGroupAttributes(size_t groupIdSizeT, chip::Controller::DeviceCommissioner * devCtrl, | ||
| size_t busyWaitMsSizeT, | ||
| chip::python::PyWriteAttributeData * writeAttributesData, | ||
|
|
@@ -318,6 +321,40 @@ class WriteClientCallback : public WriteClient::Callback | |
|
|
||
| using namespace chip::python; | ||
|
|
||
| namespace { | ||
| // Helper function to process write attributes data - reduces code duplication | ||
| CHIP_ERROR ProcessWriteAttributesData(WriteClient * client, python::PyWriteAttributeData * writeAttributesData, | ||
|
There was a problem hiding this comment. what's the difference between the processing in this function vs. the legacy list encoding? There was a problem hiding this comment. Hi Cecille, To answer your question about the difference between the processing in this helper function vs. the legacy list encoding: Here's what was preserved:
The helper function intentionally does not include the listEncodingOverride parameter because:
So Amine's legacy list encoding functionality is 100% preserved - we just extracted the common attribute processing logic that was duplicated between the two WriteClient functions while keeping the legacy-specific code path intact. There was a problem hiding this comment. yeah, but the code is the same, right? So the code below is doing So why not just have the forcelegacy section also use this function and just pass in the required parameter? There was a problem hiding this comment. Understood, that is sorta what I had initially, then when I submitted the change and requested a re-review from the Gemini AI bot it suggested doing what I did above here in its comment on the change. There was a problem hiding this comment. In that comment, the bot suggests including forceLegacyListEncoding as a parameter. But you have something different than what the bot suggested regardless. There was a problem hiding this comment. Hi Cecille, you are correct. There was a problem hiding this comment. Yeah, if you can get it so the legacy list encoding just calls this function with a parameter, that would be cleaner. There was a problem hiding this comment. Got it! |
||
| size_t attributeDataLength) | ||
| { | ||
| CHIP_ERROR err = CHIP_NO_ERROR; | ||
|
|
||
| for (size_t i = 0; i < attributeDataLength; i++) | ||
| { | ||
| python::PyAttributePath path = writeAttributesData[i].attributePath; | ||
| void * tlv = writeAttributesData[i].tlvData; | ||
| size_t length = writeAttributesData[i].tlvLength; | ||
|
|
||
| uint8_t * tlvBuffer = reinterpret_cast<uint8_t *>(tlv); | ||
|
|
||
| TLV::TLVReader reader; | ||
| reader.Init(tlvBuffer, static_cast<uint32_t>(length)); | ||
| reader.Next(); | ||
| Optional<DataVersion> dataVersion; | ||
| if (path.hasDataVersion == 1) | ||
| { | ||
| dataVersion.SetValue(path.dataVersion); | ||
| } | ||
|
|
||
| SuccessOrExit( | ||
| err = client->PutPreencodedAttribute( | ||
| chip::app::ConcreteDataAttributePath(path.endpointId, path.clusterId, path.attributeId, dataVersion), reader)); | ||
| } | ||
|
|
||
| exit: | ||
| return err; | ||
| } | ||
| } // namespace | ||
|
|
||
| extern "C" { | ||
| void pychip_WriteClient_InitCallbacks(OnWriteResponseCallback onWriteResponseCallback, OnWriteErrorCallback onWriteErrorCallback, | ||
| OnWriteDoneCallback onWriteDoneCallback) | ||
|
|
@@ -364,31 +401,80 @@ PyChipError pychip_WriteClient_WriteAttributes(void * appContext, DeviceProxy * | |
|
|
||
| VerifyOrExit(device != nullptr && device->GetSecureSession().HasValue(), err = CHIP_ERROR_MISSING_SECURE_SESSION); | ||
|
|
||
| // Handle legacy list encoding override if needed | ||
| if (forceLegacyListEncoding) | ||
| { | ||
| for (size_t i = 0; i < attributeDataLength; i++) | ||
| { | ||
| python::PyAttributePath path = writeAttributesData[i].attributePath; | ||
| void * tlv = writeAttributesData[i].tlvData; | ||
| size_t length = writeAttributesData[i].tlvLength; | ||
|
|
||
| uint8_t * tlvBuffer = reinterpret_cast<uint8_t *>(tlv); | ||
|
|
||
| TLV::TLVReader reader; | ||
| reader.Init(tlvBuffer, static_cast<uint32_t>(length)); | ||
| reader.Next(); | ||
| Optional<DataVersion> dataVersion; | ||
| if (path.hasDataVersion == 1) | ||
| { | ||
| dataVersion.SetValue(path.dataVersion); | ||
| } | ||
|
|
||
| auto listEncodingOverride = WriteClient::TestListEncodingOverride::kForceLegacyEncoding; | ||
|
|
||
| SuccessOrExit(err = client->PutPreencodedAttribute( | ||
| chip::app::ConcreteDataAttributePath(path.endpointId, path.clusterId, path.attributeId, dataVersion), | ||
| reader, listEncodingOverride)); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| SuccessOrExit(err = ProcessWriteAttributesData(client.get(), writeAttributesData, attributeDataLength)); | ||
| } | ||
|
|
||
| SuccessOrExit(err = client->SendWriteRequest(device->GetSecureSession().Value(), | ||
| interactionTimeoutMs != 0 ? System::Clock::Milliseconds32(interactionTimeoutMs) | ||
| : System::Clock::kZero)); | ||
|
|
||
| client.release(); | ||
| callback.release(); | ||
|
|
||
| if (busyWaitMs) | ||
| { | ||
| usleep(busyWaitMs * 1000); | ||
| } | ||
|
|
||
| exit: | ||
| return ToPyChipError(err); | ||
| } | ||
|
|
||
| PyChipError pychip_WriteClient_TestOnlyWriteAttributesTimedRequestNoTimedAction(void * appContext, DeviceProxy * device, | ||
| size_t interactionTimeoutMsSizeT, | ||
| size_t busyWaitMsSizeT, | ||
| python::PyWriteAttributeData * writeAttributesData, | ||
| size_t attributeDataLength) | ||
| { | ||
| CHIP_ERROR err = CHIP_NO_ERROR; | ||
|
|
||
| uint16_t interactionTimeoutMs = static_cast<uint16_t>(interactionTimeoutMsSizeT); | ||
| uint16_t busyWaitMs = static_cast<uint16_t>(busyWaitMsSizeT); | ||
|
|
||
| std::unique_ptr<WriteClientCallback> callback = std::make_unique<WriteClientCallback>(appContext); | ||
|
|
||
| // CRITICAL: Use TestOnly constructor to set TimedRequest flag without timeout. | ||
| // This function intentionally sets TimedRequest=true but does NOT send a TimedRequest action first. | ||
| // This should result in TIMED_REQUEST_MISMATCH error. | ||
| std::unique_ptr<WriteClient> client = std::make_unique<WriteClient>( | ||
| app::InteractionModelEngine::GetInstance()->GetExchangeManager(), callback->GetChunkedCallback(), | ||
| true); // Set TimedRequest flag to true without timeout | ||
|
|
||
| VerifyOrExit(device != nullptr && device->GetSecureSession().HasValue(), err = CHIP_ERROR_MISSING_SECURE_SESSION); | ||
|
|
||
| SuccessOrExit(err = ProcessWriteAttributesData(client.get(), writeAttributesData, attributeDataLength)); | ||
|
|
||
| // Send WriteRequest with TimedRequest flag set but no preceding TimedRequest action | ||
| // This should trigger TIMED_REQUEST_MISMATCH error | ||
| SuccessOrExit(err = client->SendWriteRequest(device->GetSecureSession().Value(), | ||
| interactionTimeoutMs != 0 ? System::Clock::Milliseconds32(interactionTimeoutMs) | ||
| : System::Clock::kZero)); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.