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

Add CRMP test for retransmission of session establishment messages #7234

Merged
merged 7 commits into from
Jun 1, 2021

Conversation

pan-apple
Copy link
Contributor

Problem

Need a unit test to ensure session establishment messages (that are not encrypted) are able to use CRMP functionality.

Change overview

Added a unit test and fix the problem with the retransmission of session establishment messages.

Testing

The new unit test CheckResendSessionEstablishmentMessageWithPeerExchange confirms the failure and fixes.
The existing TestReliableMessageProtocol test suite ensures the rest of functionality is intact.
Also, tested the code using Python controller app, and iOS CHIP Tool app.

src/messaging/tests/TestReliableMessageProtocol.cpp Outdated Show resolved Hide resolved
src/messaging/tests/TestReliableMessageProtocol.cpp Outdated Show resolved Hide resolved
@@ -36,13 +36,16 @@ class SessionEstablishmentExchangeDispatch : public Messaging::ExchangeMessageDi

virtual ~SessionEstablishmentExchangeDispatch() {}

CHIP_ERROR Init(TransportMgrBase * transportMgr)
CHIP_ERROR Init(Messaging::ReliableMessageMgr * reliableMessageMgr, TransportMgrBase * transportMgr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is OK for now, but as a followup: Why do we need to have a ReliableMessageMgr here at all? I just checked, and all uses of ExchangeMessageDispatch::mReliableMessageMgr seem to be inside ExchangeMessageDispatch::SendMessage and in there it could get it from the ReliableMessageContext, no? We'd need to make the getter public, but I think that should be ok....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, once we change the getter to public, we can remove it from this API.

@pan-apple pan-apple requested a review from bzbarsky-apple May 28, 2021 21:15
@pan-apple pan-apple force-pushed the crmp-sess-est-tests branch from 670ad73 to 2318b74 Compare May 29, 2021 16:31
@pan-apple
Copy link
Contributor Author

rebased

@github-actions
Copy link

Size increase report for "esp32-example-build" from e08afe7

File Section File VM
chip-all-clusters-app.elf .flash.text 172 172
chip-all-clusters-app.elf .flash.rodata 48 48
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,2322
.debug_loc,0,1255
.debug_line,0,983
.debug_ranges,0,296
.flash.text,172,172
.debug_str,0,162
.debug_abbrev,0,107
.flash.rodata,48,48
.debug_frame,0,24
.debug_aranges,0,8
.strtab,0,3
.xt.prop._ZTVN4chip11DeviceLayer37DeviceNetworkProvisioningDelegateImplE,0,-1
.symtab,0,-16
[Unmapped],0,-48
.xt.prop._ZNK4chip9Messaging23ExchangeMessageDispatch13ResendMessageENS_19SecureSessionHandleEONS_27EncryptedPacketBufferHandleEPS3_,0,-116
.shstrtab,0,-267

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

sections,vmsize,filesize


@github-actions
Copy link

Size increase report for "gn_qpg6100-example-build" from e08afe7

File Section File VM
chip-qpg6100-lighting-example.out .text 248 248
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.debug_info,0,2396
.debug_loc,0,1113
.debug_line,0,443
.debug_ranges,0,408
.text,248,248
.debug_str,0,160
.debug_abbrev,0,100
.debug_frame,0,64
.symtab,0,64
.debug_aranges,0,8
.strtab,0,3
.shstrtab,0,1
[Unmapped],0,-248


@pan-apple pan-apple force-pushed the crmp-sess-est-tests branch from 76c74a2 to e751b45 Compare June 1, 2021 16:42
@pan-apple
Copy link
Contributor Author

rebased to pick up Darwin test failures fix.

@bzbarsky-apple bzbarsky-apple merged commit 0f94665 into project-chip:master Jun 1, 2021
@pan-apple pan-apple deleted the crmp-sess-est-tests branch June 1, 2021 18:34
@github-actions
Copy link

github-actions bot commented Jun 1, 2021

Size increase report for "nrfconnect-example-build" from 872051e

File Section File VM
chip-lock.elf text 188 188
chip-lock.elf rodata 48 52
chip-lock.elf device_handles 4 4
chip-shell.elf rodata 48 48
chip-shell.elf text 12 12
chip-shell.elf device_handles 4 4
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,2392
.debug_loc,0,1114
.debug_line,0,446
.debug_ranges,0,440
text,188,188
.debug_str,0,162
.debug_abbrev,0,102
.debug_frame,0,64
.symtab,0,64
rodata,52,48
.debug_aranges,0,8
device_handles,4,4
.strtab,0,3
.shstrtab,0,1

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

sections,vmsize,filesize
rodata,48,48
.symtab,0,32
.debug_loc,0,28
.debug_info,0,25
.debug_abbrev,0,17
.debug_frame,0,12
text,12,12
device_handles,4,4
.debug_line,0,-90


nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
…roject-chip#7234)

* Add CRMP test for retransmission of session establishment messages

* Fix test

* Add packet loss test to TestPASESession

* add some comments

* reduce stack usage due to nrfconnect platform limits

* clear transport state before running test

* fixes for testing on nrfconnect
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants