project-chip / connectedhomeip

Matter (formerly Project CHIP) creates more connections between more objects, simplifying development for manufacturers and increasing compatibility for consumers, guided by the Connectivity Standards Alliance.
https://buildwithmatter.com
Apache License 2.0
7.53k stars 2.03k forks source link

TCP support is advertised by default but not implemented #23028

Open mspang opened 2 years ago

mspang commented 2 years ago
git grep GetTcpSupported
src/lib/dnssd/Advertiser.h:    Optional<bool> GetTcpSupported() const { return mTcpSupported; }
src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp:        if (params.GetTcpSupported().HasValue())
src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp:                snprintf(storage.tcpSupportedBuf, sizeof(storage.tcpSupportedBuf), "T=%d", params.GetTcpSupported().Value());
src/lib/dnssd/Discovery_ImplPlatform.cpp:        return CopyTextRecordValue(buffer, bufferLen, params.GetTcpSupported());

Depending on platform, this is set by default

config/ameba/args.gni:chip_inet_config_enable_tcp_endpoint = true
config/beken/args.gni:chip_inet_config_enable_tcp_endpoint = true
config/esp32/args.gni:chip_inet_config_enable_tcp_endpoint = true
config/genio/args.gni:chip_inet_config_enable_tcp_endpoint = true
config/mbed/chip-gn/args.gni:chip_inet_config_enable_tcp_endpoint = true
config/nrfconnect/chip-module/CMakeLists.txt:chip_gn_arg_bool  ("chip_inet_config_enable_tcp_endpoint"   CONFIG_CHIP_BUILD_TESTS)
config/qpg/chip-gn/args.gni:chip_inet_config_enable_tcp_endpoint = false
config/telink/chip-module/CMakeLists.txt:chip_gn_arg_bool  ("chip_inet_config_enable_tcp_endpoint"   CONFIG_CHIP_BUILD_TESTS)
config/zephyr/chip-module/CMakeLists.txt:chip_gn_arg_bool  ("chip_inet_config_enable_tcp_endpoint"   CONFIG_CHIP_BUILD_TESTS)
examples/lighting-app/beken/args.gni:chip_inet_config_enable_tcp_endpoint = true
src/inet/BUILD.gn:    "INET_CONFIG_ENABLE_TCP_ENDPOINT=${chip_inet_config_enable_tcp_endpoint}",
src/inet/BUILD.gn:  if (chip_inet_config_enable_tcp_endpoint) {
src/inet/inet.gni:  chip_inet_config_enable_tcp_endpoint = true
src/platform/Ameba/args.gni:chip_inet_config_enable_tcp_endpoint = true
src/platform/Beken/args.gni:chip_inet_config_enable_tcp_endpoint = true
src/platform/EFR32/args.gni:chip_inet_config_enable_tcp_endpoint = false
src/platform/EFR32/wifi_args.gni:chip_inet_config_enable_tcp_endpoint = true
src/platform/Infineon/CYW30739/args.gni:chip_inet_config_enable_tcp_endpoint = false
src/platform/bouffalolab/BL602/args.gni:chip_inet_config_enable_tcp_endpoint = true
src/platform/bouffalolab/BL702/args.gni:chip_inet_config_enable_tcp_endpoint = false
src/platform/nxp/k32w/k32w0/args.gni:chip_inet_config_enable_tcp_endpoint = false
src/platform/qpg/args.gni:chip_inet_config_enable_tcp_endpoint = false

but in particular, the overall default is 'true' despite not being implemented

src/inet/inet.gni:  chip_inet_config_enable_tcp_endpoint = true
tcarmelveilleux commented 2 years ago

This is a critical issue regarding future-proofing for TCP support.

The spec says:

4.3.4. Common TXT Key/Value Pairs ...

The optional key T indicates whether the Node supports TCP. This key MAY optionally be provided by a Node that does not support TCP. If the key is not included or invalid, the Node querying the service record SHALL assume the default value of T=0 indicating TCP is not supported. The T key, if included, SHALL have one of two valid values: '0' to indicate "TCP not supported", or '1' to indicate "TCP supported".

Example: T=1 to announce TCP is supported by the advertising Node.

The issue is the vagueness of "TCP support". It was meant to indicate "Matter operational TCP transport" rather than "MRP over UDP". This is more clear in:

4.12.1.2. Session Establishment over IP When establishing a session over IP, the initiator SHALL use TCP when both of the following are true:

The initiator supports TCP

The responder supports TCP as indicated by the T flag (*NOTE: the T here points to section 4.3.4 as a hyperlink).

If one or both nodes do not support TCP, the initiator SHALL use MRP to establish the session.

The choice of transport used during session establishment SHALL be used for the transport of messages of the established session.

Overall we never fully implemented/tested TCP support in Matter. TCP support is not actually used or supported by the transport layer in Matter 1.0 SDK!. Therefore, even if there is some TCP plumbing (and in fact, some platforms like Android require the TCPEndpointManager to be supported, there is no logic present to ever handle the lifecycle of the connections, or dispatch messages over TCP transport. None of it was tested either.

The code that exists for TCP support is vestigial from Weave and ready to be integrated to start implementing TCP transport post-1.0. For 1.0, all devices should never announce T=1. Otherwise, we will have trouble with post-1.0 devices where the initiator of an interaction supports TCP for real, in how it will be used if the responder claims TCP support due to invalid plumbing of the "TCP supported" condition. An alternative is to be clear in post-1.0 and in a 1.0 errata, that TCP support is provisional, and we would have to invent a T=2 to mean "Matter TCP support really supported.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale[bot] commented 1 year ago

This stale issue has been automatically closed. Thank you for your contributions.