Plumb Peer DNS-SD advertisement for TCP support into SessionSetup context #30392 #33481
+194
−54
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Update local master
Update local master
update local master
Update local-master
update local-master
Update local master
Update local-master
update local master
update local-master
update local master
|
PR #33481: Size comparison from 219e198 to 274c135 Increases (66 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nxp, psoc6, qpg, stm32, telink)
Decreases (3 builds for bl702l, linux)
Full report (66 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nxp, psoc6, qpg, stm32, telink)
|
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo and
chshu
pullapprove
bot
requested review from
selissia,
sharadb-amazon,
tcarmelveilleux,
tecimovic,
tima-q,
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang and
yufengwangca
|
PR #33481: Size comparison from 219e198 to 3617481 Increases (3 builds for cc32xx, stm32)
Full report (3 builds for cc32xx, stm32)
|
|
PR #33481: Size comparison from 219e198 to c61be5c Increases (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
Decreases (3 builds for bl702l, linux)
Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
bzbarsky-apple
requested changes
May 16, 2024
update local master
Local master
|
PR #33481: Size comparison from 4c6e8bc to 1373fb4 Increases (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
Decreases (3 builds for bl702l, linux)
Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
|
PR #33481: Size comparison from 4c6e8bc to a76de01 Increases (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
Decreases (3 builds for bl702l, linux)
Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
|
PR #33481: Size comparison from 4c6e8bc to bc9c12d Increases above 0.2%:
Increases (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
Decreases (4 builds for bl702l, linux)
Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
pidarped
approved these changes
May 18, 2024
| @@ -195,8 +195,9 @@ void OperationalSessionSetup::Connect(Callback::Callback<OnDeviceConnected> * on | |||
| Connect(onConnection, nullptr, onSetupFailure, transportPayloadCapability); | |||
| } | |||
|
|
|||
| void OperationalSessionSetup::UpdateDeviceData(const Transport::PeerAddress & addr, const ReliableMessageProtocolConfig & config) | |||
There was a problem hiding this comment.
Since the peerAddress is part of the ResolveResult, the latter can be the only argument to UpdateDeviceData() now.
| { | ||
| auto config = result.mrpRemoteConfig; |
There was a problem hiding this comment.
Suggested change
| auto config = result.mrpRemoteConfig; | |
| auto & config = result.mrpRemoteConfig; |
unless we are trying to make a copy, which we are not.
Comment on lines
298
to
299
| // TODO: Combine LargePayload flag with DNS-SD advertisements from peer. | ||
| // Issue #32348. |
There was a problem hiding this comment.
Should this TODO comment still be here?
| { | ||
| auto config = result.mrpRemoteConfig; |
There was a problem hiding this comment.
Suggested change
| auto config = result.mrpRemoteConfig; | |
| auto & config = result.mrpRemoteConfig; |
| #if INET_CONFIG_ENABLE_TCP_ENDPOINT | ||
| // TODO: Combine LargePayload flag with DNS-SD advertisements from peer. | ||
| // Issue #32348. | ||
| if (mTransportPayloadCapability == TransportPayloadCapability::kLargePayload) | ||
| if (mTransportPayloadCapability == TransportPayloadCapability::kLargePayload && result.supportsTcpServer && | ||
| result.supportsTcpClient) |
There was a problem hiding this comment.
Why do we care whether it supports TCP client? We're going to be the client; as long as it supports server we are fine, no?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, the TCP support advertisement from the peer is not correctly consumed and stored. This needs to be parsed properly during operational session setup and stored in the session context(given that the TCP support advertisement is a bit field in the 1.3 spec). This would be used to set the PeerAddress type to TCP or otherwise, when attempting to establish CASE with the peer.
Fixes #29539