Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable CRMP if we send/receive messages not over UDP transport #6407

Merged
merged 2 commits into from
May 4, 2021
Merged

Disable CRMP if we send/receive messages not over UDP transport #6407

merged 2 commits into from
May 4, 2021

Conversation

yufengwangca
Copy link
Contributor

Problem

I found CRMP is always enabled regardless of low level transport for application dispatch. This is regression from recent changes.

Currently, we use IsTransportReliable() to decide if to enable CRMP, this logic is wrong since TransportMgr is a tuple which can support TCP and UDP simultaneously.

We need to use the destination address instead to decide if use CRMP:

  1. For PASE/CASE message exchange, we don't have PeerConnectionState, we can relay on the SessionEstablishmentExchangeDispatch to do this check since it has the peer destination address.

  2. For Application message exchange, we can get the peer address form PeerConnectionState, and do the check.

Summary of Changes

Disable CRMP if we send/receive messages not over UDP transport

Fixes #6026

@woody-apple
Copy link
Contributor

@pan-apple ?

How is this different than this? #6317

@yufengwangca yufengwangca requested a review from pan-apple April 30, 2021 19:07
@yufengwangca
Copy link
Contributor Author

@pan-apple ?

How is this different than this? #6317

#6317 address the CASE and PASE messages over SessionEstablishmentExchangeDispatch, but not address the application message over ApplicationExchangeDispatch

@pan-apple
Copy link
Contributor

@pan-apple ?
How is this different than this? #6317

#6317 address the CASE and PASE messages over SessionEstablishmentExchangeDispatch, but not address the application message over ApplicationExchangeDispatch

Yes, this is to support Application messages over TCP (and disable CRMP for such messages).

src/messaging/ExchangeContext.cpp Show resolved Hide resolved
src/messaging/ExchangeContext.cpp Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented May 1, 2021

Size increase report for "esp32-example-build" from 6df63b4

File Section File VM
chip-all-clusters-app.elf .dram0.bss 0 8
chip-all-clusters-app.elf .flash.rodata 8 8
chip-all-clusters-app.elf .flash.text -60 -60
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,76524
.debug_line,0,9554
.debug_abbrev,0,7452
.xt.prop._ZN4chip9Messaging23ExchangeMessageDispatch29IsReliableTransmissionAllowedEv,0,156
.debug_str,0,135
.strtab,0,131
.xt.prop._ZN4chip36SessionEstablishmentExchangeDispatch29IsReliableTransmissionAllowedEv,0,76
.debug_ranges,0,56
.debug_loc,0,28
.shstrtab,0,21
.symtab,0,16
.dram0.bss,8,0
.flash.rodata,8,8
.xt.prop._ZTVN4chip24LifetimePersistedCounterE,0,-1
.debug_aranges,0,-8
[Unmapped],0,-8
.debug_frame,0,-24
.flash.text,-60,-60
.xt.prop._ZN4chip36SessionEstablishmentExchangeDispatch19IsTransportReliableEv,0,-76
.xt.prop._ZN4chip9Messaging23ExchangeMessageDispatch19IsTransportReliableEv,0,-156


@github-actions
Copy link

github-actions bot commented May 1, 2021

Size increase report for "nrfconnect-example-build" from 6df63b4

File Section File VM
chip-lighting.elf text 332 332
chip-lighting.elf rodata 184 184
chip-lighting.elf bss 0 32
chip-lighting.elf init_array 4 4
chip-shell.elf text 12 12
chip-shell.elf device_handles 4 4
chip-lock.elf text 336 336
chip-lock.elf rodata 184 184
chip-lock.elf bss 0 32
chip-lock.elf device_handles 12 12
chip-lock.elf init_array 4 4
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-lighting.elf and ./pull_artifact/chip-lighting.elf:

sections,vmsize,filesize
.debug_info,0,407163
.debug_line,0,24599
.debug_abbrev,0,14674
.debug_loc,0,2205
.debug_str,0,1919
.strtab,0,676
.symtab,0,480
.debug_ranges,0,432
.debug_frame,0,352
text,332,332
rodata,184,184
.debug_aranges,0,168
bss,32,0
init_array,4,4

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.debug_info,0,67
.debug_str,0,31
.debug_line,0,29
text,12,12
.strtab,0,10
device_handles,4,4
.shstrtab,0,-2
.debug_ranges,0,-8
.debug_loc,0,-87

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,199512
.debug_line,0,24120
.debug_abbrev,0,16064
.debug_loc,0,2209
.debug_str,0,1919
.strtab,0,676
.symtab,0,480
.debug_ranges,0,432
.debug_frame,0,352
text,336,336
rodata,184,184
.debug_aranges,0,168
bss,32,0
device_handles,12,12
init_array,4,4


@woody-apple
Copy link
Contributor

@saurabhst @msandstedt ?

@woody-apple woody-apple merged commit 9b06b05 into project-chip:master May 4, 2021
@yufengwangca yufengwangca deleted the pr/crmp/tcp branch May 4, 2021 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add exchange message dispatch support for TCP if needed
6 participants