Make max block size for BDX transfers from DiagnosticLogs cluster configurable in the controller and device (project-chip#41447)

* Implemented override of BDX block size on cluster and controller, and added max payload size override for UDP

* Revert payload limit override

* Fixed data type for block size and added "= None" to explicitly give None value if parameter is missing.

* Simplified the logic for setting gMaxBdxBlockSize.  Restyled fixes.

* Restyled nitpick

* Docstring updates, renamed setter function, and replaced hardcoded 1024 with macro

Author: feasel0

src/app/clusters/diagnostic-logs-server/BDXDiagnosticLogsProvider.cpp

@@ -36,7 +36,7 @@ constexpr System::Clock::Timeout kBdxPollIntervalMs = System::Clock::Millisecond
 // Timeout for the BDX transfer session
 constexpr System::Clock::Timeout kBdxTimeout = System::Clock::Seconds16(5 * 60);
 
-constexpr uint16_t kBdxMaxBlockSize = 1024;
+constexpr uint16_t kBdxMaxBlockSize = CHIP_CONFIG_BDX_LOG_TRANSFER_MAX_BLOCK_SIZE;
 
 CHIP_ERROR BDXDiagnosticLogsProvider::InitializeTransfer(CommandHandler * commandObj, const ConcreteCommandPath & path,
                                                          DiagnosticLogsProviderDelegate * delegate, IntentEnum intent,

src/controller/python/matter/ChipDeviceCtrl.py

@@ -1827,14 +1827,16 @@ def WriteGroupAttribute(
         # An empty list is the expected return for sending group write attribute.
         return []
 
-    def TestOnlyPrepareToReceiveBdxData(self) -> asyncio.Future:
+    def TestOnlyPrepareToReceiveBdxData(self, max_block_size: int | None = None) -> asyncio.Future:
         '''
         Sets up the system to expect a node to initiate a BDX transfer. The transfer will send data here.
 
         If no BDX transfer is initiated, the caller must cancel the returned future to avoid interfering with other BDX transfers.
         For example, the Diagnostic Logs clusters won't start a BDX transfer when the log is small so the future must be cancelled to allow later
         attempts to retrieve logs to succeed.
 
+        If max_block_size is provided (1..65535), it overrides the controller's default cap.
+
         Returns:
             a future that will yield a BdxTransfer with the init message from the transfer.
 
@@ -1846,7 +1848,7 @@ def TestOnlyPrepareToReceiveBdxData(self) -> asyncio.Future:
         eventLoop = asyncio.get_running_loop()
         future = eventLoop.create_future()
 
-        Bdx.PrepareToReceiveBdxData(future).raise_on_error()
+        Bdx.PrepareToReceiveBdxData(future, max_block_size).raise_on_error()
         return future
 
     def TestOnlyPrepareToSendBdxData(self, data: bytes) -> asyncio.Future:

src/controller/python/matter/bdx/Bdx.py

@@ -113,10 +113,12 @@ def _OnTransferCompletedCallback(transaction: AsyncTransferCompletedTransaction,
     transaction.handleResult(result)
 
 
-def _PrepareForBdxTransfer(future: Future, data: Optional[bytes]) -> PyChipError:
+def _PrepareForBdxTransfer(future: Future, data: Optional[bytes], max_block_size: Optional[int] = None) -> PyChipError:
     ''' Prepares the BDX system for a BDX transfer. The BDX transfer is set as the future's result. This must be called
     before the BDX transfer is initiated.
 
+    If max_block_size is provided (1..65535), it overrides the controller's default cap.
+
     Returns the CHIP_ERROR result from the C++ side.
     '''
     handle = GetLibraryHandle()
@@ -137,22 +139,32 @@ def _on_done(fut: Future):
 
     ctypes.pythonapi.Py_IncRef(ctypes.py_object(transaction))
     res = builtins.chipStack.Call(
-        lambda: handle.pychip_Bdx_ExpectBdxTransfer(ctypes.py_object(transaction))
+        lambda: handle.pychip_Bdx_ExpectBdxTransfer(
+            ctypes.py_object(transaction),
+            # 0 means “use default”.
+            ctypes.c_uint16(0 if max_block_size is None else max_block_size),
+        )
     )
     if not res.is_success:
         ctypes.pythonapi.Py_DecRef(ctypes.py_object(transaction))
     return res
 
 
-def PrepareToReceiveBdxData(future: Future) -> PyChipError:
+def PrepareToReceiveBdxData(future: Future, max_block_size: Optional[int] = None) -> PyChipError:
     ''' Prepares the BDX system for a BDX transfer where this device receives data. This must be called before the BDX
     transfer is initiated.
 
     When a BDX transfer is found it's set as the future's result. If an error occurs while waiting it is set as the future's exception.
 
+    If max_block_size is provided (1..65535), it overrides the controller's default cap.
+
     Returns an error if there was an issue preparing to wait a BDX transfer.
     '''
-    return _PrepareForBdxTransfer(future, None)
+    # Validate max_block_size to fit in uint16.
+    if max_block_size is not None and not (1 <= max_block_size <= 0xFFFF):
+        raise ValueError("max_block_size must be in [1, 65535]")
+
+    return _PrepareForBdxTransfer(future, None, max_block_size)
 
 
 def PrepareToSendBdxData(future: Future, data: bytes) -> PyChipError:
@@ -224,7 +236,7 @@ def Init():
         setter = NativeLibraryHandleMethodArguments(handle)
 
         setter.Set('pychip_Bdx_ExpectBdxTransfer',
-                   PyChipError, [py_object])
+                   PyChipError, [py_object, c_uint16])
         setter.Set('pychip_Bdx_StopExpectingBdxTransfer',
                    PyChipError, [py_object])
         setter.Set('pychip_Bdx_AcceptTransferAndReceiveData',

src/controller/python/matter/bdx/bdx-transfer.cpp

@@ -23,12 +23,21 @@ namespace chip {
 namespace bdx {
 namespace {
 
-constexpr uint32_t kMaxBdxBlockSize               = 1024;
+constexpr uint16_t kMaxBdxBlockSize               = CHIP_CONFIG_BDX_LOG_TRANSFER_MAX_BLOCK_SIZE;
 constexpr System::Clock::Timeout kBdxPollInterval = System::Clock::Milliseconds32(50);
 constexpr System::Clock::Timeout kBdxTimeout      = System::Clock::Seconds16(5 * 60);
 
+// Max block size for the next transfer; set from bdx.cpp when Python passes a value.
+// Resets to default kMaxBdxBlockSize after PrepareForTransfer is called.
+static uint16_t gMaxBdxBlockSize = kMaxBdxBlockSize;
+
 } // namespace
 
+void SetControllerBdxMaxBlockSizeForNextTransfer(uint16_t maxBlockSize)
+{
+    gMaxBdxBlockSize = maxBlockSize;
+}
+
 void BdxTransfer::SetDelegate(BdxTransfer::Delegate * delegate)
 {
     mDelegate = delegate;
@@ -204,7 +213,8 @@ CHIP_ERROR BdxTransfer::OnMessageReceived(chip::Messaging::ExchangeContext * exc
             role  = TransferRole::kSender;
         }
         ReturnLogErrorOnFailure(
-            Responder::PrepareForTransfer(mSystemLayer, role, flags, kMaxBdxBlockSize, kBdxTimeout, kBdxPollInterval));
+            Responder::PrepareForTransfer(mSystemLayer, role, flags, gMaxBdxBlockSize, kBdxTimeout, kBdxPollInterval));
+        gMaxBdxBlockSize = kMaxBdxBlockSize; // consume the one-shot override
     }
 
     return Responder::OnMessageReceived(exchangeContext, payloadHeader, std::move(payload));

src/controller/python/matter/bdx/bdx-transfer.h

@@ -86,5 +86,8 @@ class BdxTransfer : public Responder
     size_t mDataTransferredCount = 0;
 };
 
+// Overrides the cap for the next PrepareForTransfer() only. Reset after PrepareForTransfer.
+void SetControllerBdxMaxBlockSizeForNextTransfer(uint16_t maxBlockSize);
+
 } // namespace bdx
 } // namespace chip

src/controller/python/matter/bdx/bdx.cpp

@@ -216,11 +216,14 @@ void pychip_Bdx_InitCallbacks(OnTransferObtainedCallback onTransferObtainedCallb
 }
 
 // Prepares the BDX system to expect a new transfer.
-PyChipError pychip_Bdx_ExpectBdxTransfer(PyObject transferObtainedContext)
+// If max_block_size != 0, it overrides the default cap for the next transfer only.
+PyChipError pychip_Bdx_ExpectBdxTransfer(PyObject transferObtainedContext, uint16_t max_block_size)
 {
     TransferInfo * transferInfo = gTransfers.CreateUnassociatedTransferInfo();
     VerifyOrReturnValue(transferInfo != nullptr, ToPyChipError(CHIP_ERROR_NO_MEMORY));
     transferInfo->OnTransferObtainedContext = transferObtainedContext;
+    if (max_block_size != 0)
+        chip::bdx::SetControllerBdxMaxBlockSizeForNextTransfer(max_block_size);
     gBdxTransferServer.ExpectATransfer();
     return ToPyChipError(CHIP_NO_ERROR);
 }

src/lib/core/CHIPConfig.h

@@ -1922,6 +1922,21 @@ extern const char CHIP_NON_PRODUCTION_MARKER[];
 #define CHIP_CONFIG_MAX_BDX_LOG_TRANSFERS 5
 #endif // CHIP_CONFIG_MAX_BDX_LOG_TRANSFERS
 
+/**
+ *  @def CHIP_CONFIG_BDX_LOG_TRANSFER_MAX_BLOCK_SIZE
+ *
+ *  @brief
+ *    Maximum block size recommended by device for BDX transfers of diagnostic logs.
+ *    If increased, also increase CHIP_SYSTEM_CONFIG_PACKETBUFFER_CAPACITY_MAX.
+ *    Note that SecureSession::AllowsLargePayload limits the payload size for all transport
+ *    types besides TCP, so, if using UDP, BDX blocks larger than a certain size (e.g. 1174)
+ *    will be unable to send.
+ *
+ */
+#ifndef CHIP_CONFIG_BDX_LOG_TRANSFER_MAX_BLOCK_SIZE
+#define CHIP_CONFIG_BDX_LOG_TRANSFER_MAX_BLOCK_SIZE 1024
+#endif // CHIP_CONFIG_BDX_LOG_TRANSFER_MAX_BLOCK_SIZE
+
 /**
  *  @def CHIP_CONFIG_TEST_GOOGLETEST
  *

src/protocols/bdx/BdxTransferDiagnosticLog.cpp

@@ -25,7 +25,7 @@ namespace bdx {
 
 namespace {
 // Max block size for the BDX transfer.
-constexpr uint32_t kMaxBdxBlockSize = 1024;
+constexpr uint16_t kMaxBdxBlockSize = CHIP_CONFIG_BDX_LOG_TRANSFER_MAX_BLOCK_SIZE;
 
 // How often we poll our transfer session.  Sadly, we get allocated on
 // unsolicited message, which makes it hard for our clients to configure this.