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 exchange context implementation #2206

Merged
merged 7 commits into from
Aug 19, 2020

Conversation

andy31415
Copy link
Contributor

Problem

Previous WML baseline port included the CHIPExchangeMgr header and cpp file, however apparently the cpp for the exchange context is separated.

Summary of Changes

Added ExchangeContext.cpp from weave-core.

@github-actions
Copy link

Size increase report for "nrf-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-nrf52840-lock-example.out and ./pull_artifact/chip-nrf52840-lock-example.out:

sections,vmsize,filesize


@github-actions
Copy link

Size increase report for "linux-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-standalone-demo.out and ./pull_artifact/chip-standalone-demo.out:

sections,vmsize,filesize


Copy link
Contributor

@woody-apple woody-apple left a comment

Choose a reason for hiding this comment

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

Looks great, a few questions/comments - and things to maybe update / sync up on.

if (sendFlags & kSendFlag_AlreadyEncoded)
msgInfo->Flags |= kChipMessageFlag_MessageEncoded;
if (sendFlags & kSendFlag_ReuseMessageId)
msgInfo->Flags |= kChipMessageFlag_ReuseMessageId;
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought secure channel didn't want message ID reuse? @RyanTheOptimist @balducci-apple

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a straight copy from weave, no attempts at updating. I think we should update in separate PRs (lots of cleanup needed in general).

I expect the entire security model needs to be plugged out and replaced with SCTT.

Copy link
Contributor

Choose a reason for hiding this comment

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

@woody-apple

This is just the strait copy of the missing parts in WML, these files are not the final version and not get linked at this moment.

We are working on integrating the existing session management logic to this baseline in separate PRs. Here is the open items list for this effort:
https://github.com/project-chip/connectedhomeip/labels/cleanup_wml_baseline

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this should be dropped. This is a bit of a vestigial code that we're not pulling into CHIP (this was aimed at a self regulating group communication pattern that implemented a trickle algorithm, and we're happy to pull that out). To that end, we should pull out:

  • The flag as identified here
  • ResendMessage implementation (in ExchangeContext only)
  • any if statements guarded with references to Trickle
  • TeardownTrickleRetransmit
  • StartTimerT
  • StartTimerTau
  • TimerT
  • TimerTau
  • HandleTrickleMessage
  • SetupTrickleRetransmit

src/lib/message/ExchangeContext.cpp Show resolved Hide resolved
src/lib/message/ExchangeContext.cpp Show resolved Hide resolved
src/lib/message/ExchangeContext.cpp Show resolved Hide resolved
src/lib/message/ExchangeContext.cpp Show resolved Hide resolved
src/lib/message/ExchangeContext.cpp Show resolved Hide resolved
src/lib/message/ExchangeContext.cpp Show resolved Hide resolved
src/lib/message/ExchangeContext.cpp Show resolved Hide resolved
src/lib/message/ExchangeContext.cpp Show resolved Hide resolved
src/lib/message/ExchangeContext.cpp Show resolved Hide resolved
@github-actions
Copy link

Size increase report for "gn_nrf-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv


@github-actions
Copy link

Size increase report for "gn_linux-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv


@github-actions
Copy link

Size increase report for "nrfconnect-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize


@github-actions
Copy link

Size increase report for "esp32-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-wifi-echo.elf and ./pull_artifact/chip-wifi-echo.elf:

sections,vmsize,filesize


@andy31415 andy31415 merged commit 99e0e3a into project-chip:master Aug 19, 2020
@andy31415 andy31415 deleted the add_exchange_context branch October 9, 2020 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants