Skip to content

Commit 0ac3ca8

Browse files
j-ororkegemini-code-assist[bot]restyled-commitscecillebzbarsky-apple
authored
[Create Test] Creating IDM_3_2 python3 test module (project-chip#41066)
* [Create Test] Creating IDM_3_2 python3 test module - Creating test module for Task #[322](project-chip/matter-test-scripts#322) - Created updated test plan to remove test step 1 here: [5539](CHIP-Specifications/chip-test-plans#5539) * Update src/python_testing/TC_IDM_3_2.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update src/python_testing/TC_IDM_3_2.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Restyled by autopep8 * Removing unneeded imports * Resolving linting errors * Restyled by autopep8 * Resolving linting errors part 2 * Updating docstrings to be more logical and understandable * Updating docstrings further for clarity * Adding plumbing for TIMED_REQUEST_MISMATCH validation, changed to node label if check for test steps 5 and 6, and added commissioning test step 0: - Added plumbing for TIMED_REQUEST_MISMATCH validation in test step 7: Following similar logic as noticed in TestOnlySendCommandTimedRequestFlagWithNoTimedInvoke function - Added commissioning test step 0 - Added if check for exist of node label attribute to see if we should run test step 5 and 6, else we currently skip these test steps (Until follow-up is implemented) * Restyled by whitespace * Restyled by clang-format * Restyled by autopep8 * Update TC_IDM_3_2.py Adding todo follow-up task: project-chip/matter-test-scripts#693 for test steps 5 and 6 logic * Resolving linting errors and added some additional comments to test step 7 * Resolving code dups in ChipDeviceCtrl.py, Attribute.py, and attribute.cpp * Restyled by whitespace * Restyled by clang-format * Restyled by autopep8 * Refactored timed request logic and improve attribute discovery - Updated WriteClient.cpp/h to use mForceTimedRequestFlag instead of mTimedWriteTimeoutMs.HasValue() for timed request control - Created new find_timed_write_attribute function which uses ClusterAttributeDescriptor.must_use_timed_write to find timed write attributes dynamically for use in test step 7 * Restyled by autopep8 * Implement SuppressResponse functionality and fix WriteHandler bug per Cecille's request - Add suppressResponse parameter to Python WriteAttribute APIs in ChipDeviceCtrl.py - Update Python bindings in Attribute.py to pass suppressResponse flag to C++ - Modify WriteClient to properly set SuppressResponse in WriteRequest messages - Fix critical bug in WriteHandler.cpp: HandleWriteRequestMessage() was not checking SuppressResponse flag before sending WriteResponse messages - Update TC_IDM_3_2.py step 4 to test SuppressResponse with proper exception handling: * Use assert_raises(ChipStackError) pattern instead of broad exception catching * Expect CHIP Error 0x00000032 (Timeout) when suppressResponse=True works correctly * Verify write operation succeeds despite timeout by reading attribute back - Consolidate NodeLabel attribute guard for steps 4-6 to reduce duplication - Add ChipStackError import from matter.exceptions * Restyled by whitespace * Restyled by clang-format * Restyled by autopep8 * Restyled by isort * Update WriteHandler.cpp Removing check for SuppressResponse flag in HandleWriteRequestMessage() * Update TC_IDM_3_2.py Adding skip test step 4 until SuppressResponse implementation can be implemented on the SDK server side. Once implemented the code in test step 4 should work and the commented out block of code can then be uncommented. * Update TC_IDM_3_2.py Moved the self.skip_test(4) to above the commented out code block for test step 4 for easier reading and understanding of why it is currently being skipped. * Update TC_IDM_3_2.py Removed unnecessary comment for why test step 4 is being skipped * Applying autopep8 * Recovering lost variable * Restyled by autopep8 * resolving lint error * Restyled by autopep8 * Update src/controller/python/matter/ChipDeviceCtrl.py Co-authored-by: C Freeman <[email protected]> * Address PR feedback from Cecille: improve type safety and simplify attribute detection - Change suppressResponse from Optional[bool] to bool in WriteAttribute methods - Add type hints to find_timed_write_attribute method - Access must_use_timed_write as class property directly - Use wildcard read data instead of attribute_guard for better performance * Restyled by autopep8 * Resolving linting error and removing some unneeded comments * Apply suggestions from code review by Cecille Applying suggested code changes from Cecille Co-authored-by: C Freeman <[email protected]> * Improve TC_IDM_3_2 and add type hint to Attribute.py's _prepare_write_attributes_data function: TC_IDM_3_2.py changes: - Exclude test cluster (kTest) from timed write attribute search - Log warning instead of failing when no unsupported attributes found - Enable step 4 suppressResponse test (accepts response or no response) - Add note referencing Issue project-chip#41227 for future spec enforcement - Remove unneeded return if not unsupported_cluster_ids Attribute.py changes: - Add return type hint to _prepare_write_attributes_data() * Restyled by autopep8 * Improve timed write documentation and add missing edge case test scenario: - Adds detailed docs explaining Timed Request ACTION vs TimedRequest FLAG relationship - Documents why timed writes and chunking are incompatible in StartNewMessage() - Adds test-only constructor for "action present, flag false" scenario (3rd edge case) - Adds Python bindings and controller method for new test scenario - Adds test case in TC_IDM_3_2.py step 7 for TIMED_REQUEST_MISMATCH validation - Fixes find_timed_write_attribute() to properly handle kStandard and kTest cluster types - Registers new binding function in Attribute.py Init() * Restyled by whitespace * Restyled by clang-format * Restyled by autopep8 * Apply suggestions for timed request field value name change from Boris Co-authored-by: Boris Zbarsky <[email protected]> * Rename mForceTimedRequestFlag to mTimedRequestFieldValue and improve clarity: - Rename mForceTimedRequestFlag to mTimedRequestFieldValue for clarity - Update all documentation to use 'field' instead of 'flag' consistently - Rename TestOnlyOverrideTimedRequestFlagTag to TestOnlyOverrideTimedRequestFieldTag - Simplify member variable documentation to avoid redundant details - Update parameter names in test constructors for consistency * Removed SuppressResponse enablement from current python framework changes and fixed minor timed request issue caused during renaming * Correcting test step 4 comment to provide issue link and more context * Restyled by whitespace * Restyled by clang-format * Restyled by autopep8 * Remove leftover SuppressResponse reference and revert unintended file mode changes back to 644 * Consolidate redundant test-only Timed Request code per Boris's feedback: - Remove redundant WriteClient constructor taking only bool parameter - Merge pychip_WriteClient_TestOnlyWriteAttributesTimedRequestNoTimedAction and pychip_WriteClient_TestOnlyWriteAttributesTimedActionNoTimedRequestFlag into single pychip_WriteClient_TestOnlyWriteAttributesWithMismatchedTimedRequestField - Update Python bindings and tests to use unified API The single test function now takes both timedRequestTimeoutMs and timedRequestFieldValue parameters, allowing callers to specify exactly what combination they need for each test scenario. * Restyled by clang-format * Restyled by autopep8 * Refactor attribute write helpers per code review from Cecille: - Add parameter names to AttributeWriteRequest calls in ChipDeviceCtrl.py for clarity - Update type hints to show union of 2-tuple and 3-tuple formats - Refactor ProcessWriteAttributesData in attribute.cpp to accept forceLegacyListEncoding parameter, eliminating code duplication between legacy and normal encoding paths * Resolving minor noticed change aftter merging master * Restyled by clang-format * Restyled by autopep8 * Resolving linting errors * Restyled by autopep8 * Resolving mypy issues * Restyled by autopep8 * Resolving further mypy issues in ChipDeviceCtrl python3 module * attribute.cpp merge fix --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: C Freeman <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
1 parent 046531a commit 0ac3ca8

File tree

8 files changed

+707
-452
lines changed

8 files changed

+707
-452
lines changed

src/app/WriteClient.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ CHIP_ERROR WriteClient::StartNewMessage()
171171
ReturnErrorOnFailure(FinalizeMessage(true));
172172
}
173173

174-
// Do not allow timed request with chunks.
174+
// Per Matter specification: a Write Request that is part of a Timed Write Interaction SHALL NOT be chunked.
175175
VerifyOrReturnError(!(mTimedWriteTimeoutMs.HasValue() && !mChunks.IsNull()), CHIP_ERROR_NO_MEMORY);
176176

177177
System::PacketBufferHandle packet = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes);
@@ -201,7 +201,7 @@ CHIP_ERROR WriteClient::StartNewMessage()
201201

202202
ReturnErrorOnFailure(mWriteRequestBuilder.Init(&mMessageWriter));
203203
mWriteRequestBuilder.SuppressResponse(mSuppressResponse);
204-
mWriteRequestBuilder.TimedRequest(mTimedWriteTimeoutMs.HasValue());
204+
mWriteRequestBuilder.TimedRequest(mTimedRequestFieldValue);
205205
ReturnErrorOnFailure(mWriteRequestBuilder.GetError());
206206
mWriteRequestBuilder.CreateWriteRequests();
207207
ReturnErrorOnFailure(mWriteRequestBuilder.GetError());

src/app/WriteClient.h

Lines changed: 57 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ class WriteClient : public Messaging::ExchangeDelegate
129129
bool aSuppressResponse = false) :
130130
mpExchangeMgr(apExchangeMgr),
131131
mExchangeCtx(*this), mpCallback(apCallback), mTimedWriteTimeoutMs(aTimedWriteTimeoutMs),
132-
mSuppressResponse(aSuppressResponse)
132+
mSuppressResponse(aSuppressResponse), mTimedRequestFieldValue(aTimedWriteTimeoutMs.HasValue())
133133
{
134134
assertChipStackLockedByCurrentThread();
135135
}
@@ -138,7 +138,53 @@ class WriteClient : public Messaging::ExchangeDelegate
138138
WriteClient(Messaging::ExchangeManager * apExchangeMgr, Callback * apCallback, const Optional<uint16_t> & aTimedWriteTimeoutMs,
139139
uint16_t aReservedSize) :
140140
mpExchangeMgr(apExchangeMgr),
141-
mExchangeCtx(*this), mpCallback(apCallback), mTimedWriteTimeoutMs(aTimedWriteTimeoutMs), mReservedSize(aReservedSize)
141+
mExchangeCtx(*this), mpCallback(apCallback), mTimedWriteTimeoutMs(aTimedWriteTimeoutMs), mReservedSize(aReservedSize),
142+
mTimedRequestFieldValue(aTimedWriteTimeoutMs.HasValue())
143+
{
144+
assertChipStackLockedByCurrentThread();
145+
}
146+
147+
// Tag type to distinguish the test constructor from the normal constructor
148+
struct TestOnlyOverrideTimedRequestFieldTag
149+
{
150+
};
151+
152+
/**
153+
* TestOnly constructor that decouples the Timed Request action from the TimedRequest field value.
154+
*
155+
* IMPORTANT: Understanding the distinction between two concepts:
156+
* 1. TIMED REQUEST ACTION: A preceding TimedRequest protocol message sent before the actual Write Request.
157+
* This establishes a time window during which the server will accept the write.
158+
* This is controlled by the mTimedWriteTimeoutMs field.
159+
*
160+
* 2. TIMEDREQUEST FIELD: A boolean field in the WriteRequest message itself that indicates whether
161+
* the write was preceded by a Timed Request action.
162+
* This is controlled by the mTimedRequestFieldValue field.
163+
*
164+
* Normal behavior: When you provide a timeout value to the standard constructor, both happen together:
165+
* - A Timed Request action is sent (controlled by mTimedWriteTimeoutMs)
166+
* - The TimedRequest field in WriteRequest is set to true (mTimedRequestFieldValue = true)
167+
*
168+
* This test constructor allows you to decouple these for testing all edge cases:
169+
*
170+
* Test scenarios enabled by this constructor:
171+
* 1. Normal write (both false): Action = No, Field = False [aTimedWriteTimeoutMs = Missing,
172+
* aTimedRequestFieldValue = false]
173+
* 2. Normal timed write (both true): Action = Yes, Field = True [aTimedWriteTimeoutMs = value,
174+
* aTimedRequestFieldValue = true]
175+
* 3. Field true, no action (invalid): Action = No, Field = True [aTimedWriteTimeoutMs = Missing,
176+
* aTimedRequestFieldValue = true]
177+
* 4. Action present, field false (invalid): Action = Yes, Field = False [aTimedWriteTimeoutMs = value,
178+
* aTimedRequestFieldValue = false]
179+
*
180+
* @param[in] aTimedWriteTimeoutMs The timeout for the Timed Request action (if provided, action WILL be sent)
181+
* @param[in] aTimedRequestFieldValue The value of the TimedRequest field in WriteRequest (can mismatch the action for testing)
182+
*/
183+
WriteClient(Messaging::ExchangeManager * apExchangeMgr, Callback * apCallback, const Optional<uint16_t> & aTimedWriteTimeoutMs,
184+
bool aTimedRequestFieldValue, TestOnlyOverrideTimedRequestFieldTag) :
185+
mpExchangeMgr(apExchangeMgr),
186+
mExchangeCtx(*this), mpCallback(apCallback), mTimedWriteTimeoutMs(aTimedWriteTimeoutMs),
187+
mTimedRequestFieldValue(aTimedRequestFieldValue)
142188
{
143189
assertChipStackLockedByCurrentThread();
144190
}
@@ -525,6 +571,15 @@ class WriteClient : public Messaging::ExchangeDelegate
525571
uint16_t mReservedSize = 0;
526572
// #endif
527573

574+
/**
575+
* The value of the TimedRequest field in the WriteRequest message.
576+
*
577+
* This tells the server whether this write was preceded by a Timed Request action.
578+
* Normally this matches whether mTimedWriteTimeoutMs has a value, but test constructors
579+
* can decouple these to test protocol mismatch scenarios.
580+
*/
581+
bool mTimedRequestFieldValue = false;
582+
528583
/**
529584
* Below we define several const variables for encoding overheads.
530585
* WriteRequestMessage =

0 commit comments

Comments
 (0)