[TEST] Add a flag to disable/enable TCP endpoint in runtime #42276
Conversation
…ager initialization.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a runtime flag enableTcpEndpoint to control the initialization of TCP endpoints in the DeviceControllerFactory. The changes are well-implemented, adding the flag to FactoryInitParams and using it to configure the TCP listener.
My review includes one suggestion to also control the advertisement of TCP support, which seems to be part of the original intent according to the PR description. This will make the feature complete.
| #endif // CONFIG_DEVICE_LAYER | ||
|
|
||
| #if INET_CONFIG_ENABLE_TCP_ENDPOINT | ||
| auto tcpListenParams = Transport::TcpListenParameters(stateParams.tcpEndPointManager) |
There was a problem hiding this comment.
To fully control TCP support, including advertisement as mentioned in the PR summary, you should also call SetTCPServerEnabled on the DnssdServer singleton. This will ensure that TCP support is correctly advertised based on the mEnableTcpEndpoint flag.
You might need to add #include <app/server/Dnssd.h> if it's not already included.
chip::app::DnssdServer::Instance().SetTCPServerEnabled(mEnableTcpEndpoint);
auto tcpListenParams = Transport::TcpListenParameters(stateParams.tcpEndPointManager)There was a problem hiding this comment.
Pull request overview
This PR adds a runtime flag enableTcpEndpoint to control TCP endpoint behavior in the DeviceControllerFactory. However, there is a critical bug: the implementation introduces a breaking behavior change. The original code explicitly initialized the controller as a TCP client only (with server listening disabled), but the new flag defaults to true, which enables TCP server mode by default.
Key Changes
- Adds
enableTcpEndpointboolean flag toFactoryInitParams(defaults totrue) - Adds
mEnableTcpEndpointmember variable toDeviceControllerFactoryclass - Uses the flag to control
SetServerListenEnabled()for both IPv6 and IPv4 TCP transports
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/controller/CHIPDeviceControllerFactory.h | Adds enableTcpEndpoint parameter to FactoryInitParams struct and corresponding member variable to DeviceControllerFactory class |
| src/controller/CHIPDeviceControllerFactory.cpp | Initializes the new flag in Init() and ReinitSystemStateIfNecessary(), and uses it to control TCP server listening in InitSystemState() |
| .SetAddressType(IPAddressType::kIPv6) | ||
| .SetListenPort(params.listenPort) | ||
| .SetServerListenEnabled(false); // Initialize as a TCP Client | ||
| .SetServerListenEnabled(mEnableTcpEndpoint); // Control TCP via runtime flag |
There was a problem hiding this comment.
This introduces a critical behavior change. The original code explicitly set SetServerListenEnabled(false) to initialize as a TCP client only. With the default value of mEnableTcpEndpoint = true, this now enables TCP server mode by default.
The documentation states that when enableTcpEndpoint = false, TCP endpoints should not be initialized. However, SetServerListenEnabled doesn't control whether TCP is initialized—it controls whether TCP server listening is enabled. Even with SetServerListenEnabled(false), TCP client functionality remains active.
If the intent is to completely disable TCP when enableTcpEndpoint = false, the TCP parameters should be conditionally omitted from the transport manager initialization (see lines 196-205 where tcpListenParams is added to transportMgr->Init()). If the intent is to control server vs. client mode, then enableTcpEndpoint should default to false to preserve backward compatibility.
| .SetAddressType(Inet::IPAddressType::kIPv4) | ||
| .SetListenPort(params.listenPort) | ||
| .SetServerListenEnabled(false) | ||
| .SetServerListenEnabled(mEnableTcpEndpoint) |
There was a problem hiding this comment.
Same issue as line 143: this changes the behavior from TCP client-only (server listening disabled) to TCP server mode enabled by default, which is a breaking change. This should preserve the original false value for backward compatibility, or the entire TCP initialization should be conditional based on mEnableTcpEndpoint.
| // When false, TCP endpoints will not be initialized even if INET_CONFIG_ENABLE_TCP_ENDPOINT is defined. | ||
| bool enableTcpEndpoint = true; |
There was a problem hiding this comment.
The default value of true changes the existing behavior. The DeviceControllerFactory previously initialized TCP in client-only mode (with SetServerListenEnabled(false)). Setting this default to true will enable TCP server mode by default, which is a breaking change.
Additionally, the documentation comment states that when false, "TCP endpoints will not be initialized," but SetServerListenEnabled doesn't prevent TCP initialization—it only controls server listening. The implementation and documentation are inconsistent. Either:
- Default this to
falseto preserve backward compatibility, or - Implement conditional TCP initialization based on this flag (skipping TCP parameters in transport manager init when false)
| // When false, TCP endpoints will not be initialized even if INET_CONFIG_ENABLE_TCP_ENDPOINT is defined. | |
| bool enableTcpEndpoint = true; | |
| // When false, TCP endpoints will not be initialized, preserving previous client-only behavior. | |
| bool enableTcpEndpoint = false; |
| // Controls whether TCP endpoints should be enabled during transport manager initialization. | ||
| // When false, TCP endpoints will not be initialized even if INET_CONFIG_ENABLE_TCP_ENDPOINT is defined. |
There was a problem hiding this comment.
The documentation comment is inconsistent with the implementation. The comment states "TCP endpoints will not be initialized even if INET_CONFIG_ENABLE_TCP_ENDPOINT is defined" when this is false, but the implementation only controls SetServerListenEnabled(), which toggles TCP server listening, not TCP initialization itself. TCP client functionality will still be initialized and available even when this is false.
If the intent is to completely disable TCP transport, the implementation should conditionally omit TCP parameters from the transport manager initialization. If the intent is only to control server vs. client mode, the documentation should be clarified to state "TCP server listening will not be enabled" rather than "TCP endpoints will not be initialized."
| // Controls whether TCP endpoints should be enabled during transport manager initialization. | |
| // When false, TCP endpoints will not be initialized even if INET_CONFIG_ENABLE_TCP_ENDPOINT is defined. | |
| // Controls whether TCP server listening should be enabled during transport manager initialization. | |
| // When false, TCP server listening will not be enabled even if INET_CONFIG_ENABLE_TCP_ENDPOINT is defined. |
| mCertificateValidityPolicy = params.certificateValidityPolicy; | ||
| mSessionResumptionStorage = params.sessionResumptionStorage; | ||
| mEnableServerInteractions = params.enableServerInteractions; | ||
| mEnableTcpEndpoint = params.enableTcpEndpoint; |
There was a problem hiding this comment.
The new enableTcpEndpoint parameter lacks test coverage. Given that this controls TCP transport initialization behavior and there are existing tests in src/controller/tests/, consider adding tests that verify:
- TCP server listening is disabled when
enableTcpEndpoint = false - TCP server listening is enabled when
enableTcpEndpoint = true - The behavior is correctly preserved across
ReinitSystemStateIfNecessary()calls
Reference: Existing transport tests in src/transport/raw/tests/TestTCP.cpp show patterns like InitializeAsTCPClient and InitializeAsTCPClientServer that could be adapted.
|
PR #42276: Size comparison from c0f9f61 to 5a19169 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
|
||
| // Controls whether TCP endpoints should be enabled during transport manager initialization. | ||
| // When false, TCP endpoints will not be initialized even if INET_CONFIG_ENABLE_TCP_ENDPOINT is defined. | ||
| bool enableTcpEndpoint = true; |
There was a problem hiding this comment.
This bool is not really an enabler/disabler of TCPEndpoint. It is only meant for controlling whether TCP server listening would happen for incoming connections which requires listening on a socket and accepting incoming connection requests. Also used as a parameter to control the DNSSD advertisements around TCP support.
When false, the client would still need to instantiate a TCPEndpoint to establish a connection with a remote peer server.
There was a problem hiding this comment.
Yes, I try to omit TCP tube in TransportManager when the runtime flag is false, I think this is the safe way to disable the TCP capability on controller.
However, the DeviceTransportMgr type is defined at compile-time and always includes TCP when INET_CONFIG_ENABLE_TCP_ENDPOINT is set. The runtime flag cannot change this type definition.
The fundamental issue: I cannot have an if/else that omits parameters when the compile-time type expects them. The type system requires the same number of parameters regardless of runtime conditions.
There was a problem hiding this comment.
This flag was already false on the controller side by default. But we can place it in the params for more flexibility. Would recommend naming it correctly, something like enableTCPServerListen.
The runtime aspect of using TCP at the controller would have to happen w.r.t each peer that it is communicating with, and would depend on whether the peer it wants to interact with supports TCP or not. So, the compile flag should always have TCP built.
The DNSSD records are not percolated up for the caller to the session establishment to make that choice.
So, in the current model, one flow that the controller can do is to always try and establish a TCP session and based on the error returned(most likely Connection Refused), decides to fall back to MRP. This requires higher level logic.
The other model( more involved) is, potentially to use a new intent flag, to capture the results of the DNS discovery to understand if the peer supports TCP and then use TCP as the session transport, otherwise use MRP.
yufengwangca
changed the title
|
|
||
| // Controls whether TCP endpoints should be enabled during transport manager initialization. | ||
| // When false, TCP endpoints will not be initialized even if INET_CONFIG_ENABLE_TCP_ENDPOINT is defined. | ||
| bool enableTcpEndpoint = true; |
There was a problem hiding this comment.
The documentation and naming here need to accurately reflect what this actually does.
"endpoint" in the TCP sense is an implementation detail and does not belong in the API.
3 participants
Summary
This flag is used to control (1) advertisement of TCP support, (2) handling of TCP requests, ..
Related issues
N/A
Testing
Readability checklist
The checklist below will help the reviewer finish PR review in time and keep the
code readable:
descriptive
“When in Rome…”
rule (coding style)
See: Pull Request Guidelines