Skip to content

Commit 610043b

Browse files
committed
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.
1 parent b896bb9 commit 610043b

File tree

5 files changed

+69
-183
lines changed

5 files changed

+69
-183
lines changed

src/app/WriteClient.h

Lines changed: 14 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -144,9 +144,13 @@ class WriteClient : public Messaging::ExchangeDelegate
144144
assertChipStackLockedByCurrentThread();
145145
}
146146

147+
// Tag type to distinguish the test constructor from the normal constructor
148+
struct TestOnlyOverrideTimedRequestFieldTag
149+
{
150+
};
151+
147152
/**
148-
* TestOnly constructor that allows setting the TimedRequest field in the WriteRequest without performing
149-
* a Timed Request action (i.e., without actually sending a TimedRequest message first).
153+
* TestOnly constructor that decouples the Timed Request action from the TimedRequest field value.
150154
*
151155
* IMPORTANT: Understanding the distinction between two concepts:
152156
* 1. TIMED REQUEST ACTION: A preceding TimedRequest protocol message sent before the actual Write Request.
@@ -161,41 +165,16 @@ class WriteClient : public Messaging::ExchangeDelegate
161165
* - A Timed Request action is sent (controlled by mTimedWriteTimeoutMs)
162166
* - The TimedRequest field in WriteRequest is set to true (mTimedRequestFieldValue = true)
163167
*
164-
* This test constructor allows you to decouple these for testing edge cases:
165-
* - aTimedRequestFieldValue = false: No Timed Request action AND TimedRequest field is false (normal non-timed write)
166-
* - aTimedRequestFieldValue = true: No Timed Request action BUT TimedRequest field is true
167-
* (This is invalid behavior that should be rejected by the server, used for negative testing)
168-
*
169-
* @param[in] aTimedRequestFieldValue If true, sets the TimedRequest field in WriteRequest to true without actually
170-
* performing a Timed Request action. This creates a mismatch that should be rejected
171-
* by a compliant server.
172-
*/
173-
WriteClient(Messaging::ExchangeManager * apExchangeMgr, Callback * apCallback, bool aTimedRequestFieldValue) :
174-
mpExchangeMgr(apExchangeMgr), mExchangeCtx(*this), mpCallback(apCallback), mTimedRequestFieldValue(aTimedRequestFieldValue)
175-
{
176-
assertChipStackLockedByCurrentThread();
177-
}
178-
179-
// Tag type to distinguish the test constructor from the normal constructor
180-
struct TestOnlyOverrideTimedRequestFieldTag
181-
{
182-
};
183-
184-
/**
185-
* TestOnly constructor that allows performing a Timed Request action while setting the TimedRequest field
186-
* in the WriteRequest to false (i.e., sending a TimedRequest message first, but then lying about it in the WriteRequest).
187-
*
188-
* This tests the third edge case: Timed Request action IS performed, but the TimedRequest field is set to false.
168+
* This test constructor allows you to decouple these for testing all edge cases:
189169
*
190-
* Test scenarios enabled by WriteClient constructors:
191-
* 1. Normal write (both false): Action = No, Field = False [Standard constructor with no timeout]
192-
* 2. Normal timed write (both true): Action = Yes, Field = True [Standard constructor with timeout]
193-
* 3. Field true, no action (invalid): Action = No, Field = True [Test constructor taking bool
194-
* aTimedRequestFieldValue]
195-
* 4. Action present, field false (invalid): Action = Yes, Field = False [THIS constructor]
170+
* Test scenarios enabled by this constructor:
171+
* 1. Normal write (both false): Action = No, Field = False [aTimedWriteTimeoutMs = Missing, aTimedRequestFieldValue = false]
172+
* 2. Normal timed write (both true): Action = Yes, Field = True [aTimedWriteTimeoutMs = value, aTimedRequestFieldValue = true]
173+
* 3. Field true, no action (invalid): Action = No, Field = True [aTimedWriteTimeoutMs = Missing, aTimedRequestFieldValue = true]
174+
* 4. Action present, field false (invalid): Action = Yes, Field = False [aTimedWriteTimeoutMs = value, aTimedRequestFieldValue = false]
196175
*
197-
* @param[in] aTimedWriteTimeoutMs The timeout for the Timed Request action (WILL be sent)
198-
* @param[in] aTimedRequestFieldValue If false, the TimedRequest field in WriteRequest will be false despite the action
176+
* @param[in] aTimedWriteTimeoutMs The timeout for the Timed Request action (if provided, action WILL be sent)
177+
* @param[in] aTimedRequestFieldValue The value of the TimedRequest field in WriteRequest (can mismatch the action for testing)
199178
*/
200179
WriteClient(Messaging::ExchangeManager * apExchangeMgr, Callback * apCallback, const Optional<uint16_t> & aTimedWriteTimeoutMs,
201180
bool aTimedRequestFieldValue, TestOnlyOverrideTimedRequestFieldTag) :

src/controller/python/matter/ChipDeviceCtrl.py

Lines changed: 16 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1616,46 +1616,23 @@ def _prepare_write_attribute_requests(self, attributes: typing.List[typing.Tuple
16161616
v[0], v[1], v[2], 1, v[1].value))
16171617
return attrs
16181618

1619-
async def TestOnlyWriteAttributeTimedRequestFlagWithNoTimedAction(self, nodeid: int,
1620-
attributes: typing.List[typing.Tuple[int, ClusterObjects.ClusterAttributeDescriptor]],
1621-
interactionTimeoutMs: typing.Optional[int] = None, busyWaitMs: typing.Optional[int] = None,
1622-
payloadCapability: int = TransportPayloadCapability.MRP_PAYLOAD):
1619+
async def TestOnlyWriteAttributeWithMismatchedTimedRequestField(self, nodeid: int,
1620+
attributes: typing.List[typing.Tuple[int, ClusterObjects.ClusterAttributeDescriptor]],
1621+
timedRequestTimeoutMs: int,
1622+
timedRequestFieldValue: bool,
1623+
interactionTimeoutMs: typing.Optional[int] = None, busyWaitMs: typing.Optional[int] = None,
1624+
payloadCapability: int = TransportPayloadCapability.MRP_PAYLOAD):
16231625
'''
1624-
ONLY TO BE USED FOR TEST: Write attributes with TimedRequest flag but no TimedAction transaction.
1625-
This should result in TIMED_REQUEST_MISMATCH error.
1626+
ONLY TO BE USED FOR TEST: Write attributes with decoupled Timed Request action and TimedRequest field.
1627+
This allows testing TIMED_REQUEST_MISMATCH scenarios:
1628+
- timedRequestTimeoutMs=0, timedRequestFieldValue=True: No action, but field=true (MISMATCH)
1629+
- timedRequestTimeoutMs>0, timedRequestFieldValue=False: Action sent, but field=false (MISMATCH)
16261630
1627-
Please see WriteAttribute for description of parameters.
1631+
Please see WriteAttribute for description of common parameters.
16281632
1629-
Returns:
1630-
[AttributeStatus] (list - one for each path).
1631-
1632-
Raises:
1633-
InteractionModelError on error (expected: TIMED_REQUEST_MISMATCH)
1634-
'''
1635-
self.CheckIsActive()
1636-
1637-
eventLoop = asyncio.get_running_loop()
1638-
future = eventLoop.create_future()
1639-
1640-
device = await self.GetConnectedDevice(nodeid, timeoutMs=interactionTimeoutMs, payloadCapability=payloadCapability)
1641-
1642-
attrs = self._prepare_write_attribute_requests(attributes)
1643-
1644-
ClusterAttribute.TestOnlyWriteAttributeTimedRequestFlagWithNoTimedAction(
1645-
future, eventLoop, device.deviceProxy, attrs,
1646-
interactionTimeoutMs=interactionTimeoutMs, busyWaitMs=busyWaitMs).raise_on_error()
1647-
return await future
1648-
1649-
async def TestOnlyWriteAttributeTimedActionNoTimedRequestFlag(self, nodeid: int,
1650-
attributes: typing.List[typing.Tuple[int, ClusterObjects.ClusterAttributeDescriptor]],
1651-
timedRequestTimeoutMs: int,
1652-
interactionTimeoutMs: typing.Optional[int] = None, busyWaitMs: typing.Optional[int] = None,
1653-
payloadCapability: int = TransportPayloadCapability.MRP_PAYLOAD):
1654-
'''
1655-
ONLY TO BE USED FOR TEST: Write attributes with Timed Action performed but TimedRequest flag set to false.
1656-
This should result in TIMED_REQUEST_MISMATCH error.
1657-
1658-
Please see WriteAttribute for description of parameters.
1633+
Additional parameters:
1634+
timedRequestTimeoutMs: Timeout for the Timed Request action (0 means no action)
1635+
timedRequestFieldValue: Value of the TimedRequest field in WriteRequest
16591636
16601637
Returns:
16611638
[AttributeStatus] (list - one for each path).
@@ -1672,9 +1649,10 @@ async def TestOnlyWriteAttributeTimedActionNoTimedRequestFlag(self, nodeid: int,
16721649

16731650
attrs = self._prepare_write_attribute_requests(attributes)
16741651

1675-
ClusterAttribute.TestOnlyWriteAttributeTimedActionNoTimedRequestFlag(
1652+
ClusterAttribute.TestOnlyWriteAttributeWithMismatchedTimedRequestField(
16761653
future, eventLoop, device.deviceProxy, attrs,
16771654
timedRequestTimeoutMs=timedRequestTimeoutMs,
1655+
timedRequestFieldValue=timedRequestFieldValue,
16781656
interactionTimeoutMs=interactionTimeoutMs, busyWaitMs=busyWaitMs).raise_on_error()
16791657
return await future
16801658

src/controller/python/matter/clusters/Attribute.py

Lines changed: 16 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -1061,13 +1061,17 @@ def WriteAttributes(future: Future, eventLoop, device,
10611061
return res
10621062

10631063

1064-
def TestOnlyWriteAttributeTimedRequestFlagWithNoTimedAction(future: Future, eventLoop, device,
1065-
attributes: List[AttributeWriteRequest],
1066-
interactionTimeoutMs: Union[None, int] = None,
1067-
busyWaitMs: Union[None, int] = None) -> PyChipError:
1064+
def TestOnlyWriteAttributeWithMismatchedTimedRequestField(future: Future, eventLoop, device,
1065+
attributes: List[AttributeWriteRequest],
1066+
timedRequestTimeoutMs: int,
1067+
timedRequestFieldValue: bool,
1068+
interactionTimeoutMs: Union[None, int] = None,
1069+
busyWaitMs: Union[None, int] = None) -> PyChipError:
10681070
'''
1069-
ONLY TO BE USED FOR TEST: Writes attributes with TimedRequest flag but no TimedAction transaction
1070-
This should result in TIMED_REQUEST_MISMATCH error.
1071+
ONLY TO BE USED FOR TEST: Writes attributes with decoupled Timed Request action and TimedRequest field.
1072+
This allows testing TIMED_REQUEST_MISMATCH scenarios:
1073+
- timedRequestTimeoutMs=0, timedRequestFieldValue=True: No action, but field=true (MISMATCH)
1074+
- timedRequestTimeoutMs>0, timedRequestFieldValue=False: Action sent, but field=false (MISMATCH)
10711075
'''
10721076
handle = GetLibraryHandle()
10731077

@@ -1079,43 +1083,12 @@ def TestOnlyWriteAttributeTimedRequestFlagWithNoTimedAction(future: Future, even
10791083
transaction = AsyncWriteTransaction(future, eventLoop)
10801084
ctypes.pythonapi.Py_IncRef(ctypes.py_object(transaction))
10811085

1082-
# Call the TestOnly C++ function that sets TimedRequest=True but no timed transaction
1086+
# Call the TestOnly C++ function that decouples action and field
10831087
res = builtins.chipStack.Call(
1084-
lambda: handle.pychip_WriteClient_TestOnlyWriteAttributesTimedRequestNoTimedAction(
1085-
ctypes.py_object(transaction), device,
1086-
ctypes.c_size_t(0 if interactionTimeoutMs is None else interactionTimeoutMs),
1087-
ctypes.c_size_t(0 if busyWaitMs is None else busyWaitMs),
1088-
pyWriteAttributes, ctypes.c_size_t(numberOfAttributes))
1089-
)
1090-
if not res.is_success:
1091-
ctypes.pythonapi.Py_DecRef(ctypes.py_object(transaction))
1092-
return res
1093-
1094-
1095-
def TestOnlyWriteAttributeTimedActionNoTimedRequestFlag(future: Future, eventLoop, device,
1096-
attributes: List[AttributeWriteRequest],
1097-
timedRequestTimeoutMs: int,
1098-
interactionTimeoutMs: Union[None, int] = None,
1099-
busyWaitMs: Union[None, int] = None) -> PyChipError:
1100-
'''
1101-
ONLY TO BE USED FOR TEST: Writes attributes with Timed Action performed but TimedRequest flag set to false.
1102-
This should result in TIMED_REQUEST_MISMATCH error.
1103-
'''
1104-
handle = GetLibraryHandle()
1105-
1106-
# Note: We skip the timed write check here to allow testing the TIMED_REQUEST_MISMATCH scenario
1107-
# In normal WriteAttributes, this would check for must_use_timed_write
1108-
pyWriteAttributes, numberOfAttributes = _prepare_write_attributes_data(
1109-
attributes, must_use_timed_write_check=False)
1110-
1111-
transaction = AsyncWriteTransaction(future, eventLoop)
1112-
ctypes.pythonapi.Py_IncRef(ctypes.py_object(transaction))
1113-
1114-
# Call the TestOnly C++ function that performs TimedRequest action but sets flag=False
1115-
res = builtins.chipStack.Call(
1116-
lambda: handle.pychip_WriteClient_TestOnlyWriteAttributesTimedActionNoTimedRequestFlag(
1088+
lambda: handle.pychip_WriteClient_TestOnlyWriteAttributesWithMismatchedTimedRequestField(
11171089
ctypes.py_object(transaction), device,
11181090
ctypes.c_size_t(timedRequestTimeoutMs),
1091+
ctypes.c_bool(timedRequestFieldValue),
11191092
ctypes.c_size_t(0 if interactionTimeoutMs is None else interactionTimeoutMs),
11201093
ctypes.c_size_t(0 if busyWaitMs is None else busyWaitMs),
11211094
pyWriteAttributes, ctypes.c_size_t(numberOfAttributes))
@@ -1295,12 +1268,9 @@ def Init():
12951268

12961269
handle.pychip_WriteClient_WriteAttributes.restype = PyChipError
12971270
handle.pychip_WriteClient_WriteGroupAttributes.restype = PyChipError
1298-
handle.pychip_WriteClient_TestOnlyWriteAttributesTimedRequestNoTimedAction.restype = PyChipError
1299-
handle.pychip_WriteClient_TestOnlyWriteAttributesTimedRequestNoTimedAction.argtypes = [py_object, c_void_p,
1300-
c_size_t, c_size_t, POINTER(PyWriteAttributeData), c_size_t]
1301-
handle.pychip_WriteClient_TestOnlyWriteAttributesTimedActionNoTimedRequestFlag.restype = PyChipError
1302-
handle.pychip_WriteClient_TestOnlyWriteAttributesTimedActionNoTimedRequestFlag.argtypes = [py_object, c_void_p,
1303-
c_size_t, c_size_t, c_size_t, POINTER(PyWriteAttributeData), c_size_t]
1271+
handle.pychip_WriteClient_TestOnlyWriteAttributesWithMismatchedTimedRequestField.restype = PyChipError
1272+
handle.pychip_WriteClient_TestOnlyWriteAttributesWithMismatchedTimedRequestField.argtypes = [py_object, c_void_p,
1273+
c_size_t, c_bool, c_size_t, c_size_t, POINTER(PyWriteAttributeData), c_size_t]
13041274

13051275
# Both WriteAttributes and WriteGroupAttributes are variadic functions. As per ctype documentation
13061276
# https://docs.python.org/3/library/ctypes.html#calling-varadic-functions, it is critical that we

0 commit comments

Comments
 (0)