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.

Sign up for GitHub

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

Jump to bottom

Align Spake2 protocol message type definition with spec #4167

Merged
merged 1 commit into from
Dec 18, 2020
Merged

Align Spake2 protocol message type definition with spec #4167

merged 1 commit into from
Dec 18, 2020

Conversation

yufengwangca
Copy link
Contributor

Problem

Currently, transport layer still use the internal temporary message type definition for protocol Spake2

Summary of Changes

Align Spake2 protocol message type definition with spec 0.7.

andy31415
andy31415 approved these changes Dec 11, 2020
View reviewed changes
src/transport/SecurePairingSession.cpp Outdated
payloadHeader
.SetMessageType(msgType) //
.SetProtocolID(Protocols::kProtocol_SecureChannel);
payloadHeader.SetMessageType(static_cast<uint8_t>(msgType)).SetProtocolID(Protocols::kProtocol_SecureChannel);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it was slightly easier to read before even though // was awkward

pan-apple
pan-apple reviewed Dec 11, 2020
View reviewed changes
Copy link
Contributor

@pan-apple pan-apple left a comment

Choose a reason for hiding this comment

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

LGTM. I have this PR which updates the pairing state machine, and associated message types: #4166.

Maybe you can rebase the current PR on top of #4166 once it merges. Or, if this one merges first, I can rebase the other PR.

@yufengwangca
Copy link
Contributor Author

LGTM. I have this PR which updates the pairing state machine, and associated message types: #4166.

Maybe you can rebase the current PR on top of #4166 once it merges. Or, if this one merges first, I can rebase the other PR.

Got it, thanks!

@bzbarsky-apple
Copy link
Contributor

@yufengwangca Please merge on top of #4166 ?

@yufengwangca
Copy link
Contributor Author

@yufengwangca Please merge on top of #4166 ?

Done

@github-actions
Copy link

Size increase report for "esp32-example-build" from 4e78f50

File Section File VM
chip-all-clusters-app.elf .dram0.bss 0 8
chip-all-clusters-app.elf .flash.rodata 4 4
chip-all-clusters-app.elf .flash.text -44 -44
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_line,0,978
.debug_info,0,156
.debug_str,0,97
.strtab,0,36
.dram0.bss,8,0
.flash.rodata,4,4
.debug_loc,0,2
.xt.prop._ZTVN4chip11DeviceLayer37DeviceNetworkProvisioningDelegateImplE,0,1
[Unmapped],0,-4
.debug_abbrev,0,-18
.debug_ranges,0,-24
.flash.text,-44,-44

bzbarsky-apple
bzbarsky-apple approved these changes Dec 18, 2020
View reviewed changes
src/transport/SecurePairingSession.cpp
@@ -63,7 +63,7 @@ void SecurePairingSession::Clear()
memset(&mPoint[0], 0, sizeof(mPoint));
memset(&mWS[0][0], 0, sizeof(mWS));
memset(&mKe[0], 0, sizeof(mKe));
mNextExpectedMsg = Spake2pMsgType::kSpake2pMsgError;
mNextExpectedMsg = Protocols::SecureChannel::MsgType::PASE_Spake2pError;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether using namespace Protocols::SecureChannel; might be helpful to reduce the typing. Or using MsgType = Protocols::SecureChannel::MsgType?

Copy link
Contributor

Choose a reason for hiding this comment

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

Followup for this one as desired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using MsgType = Protocols::SecureChannel::MsgType

Now we have taken out the prefix of MsgType, Then how we differentiate "using MsgType = Protocols::Common::MsgType" and "using MsgType = Protocols::SecureChannel::MsgType" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we have to deal with both of them here, then we can't do that, of course. In that case maybe the best thing to do is using namespace chip::Protocols; or namespace SecureChannel = Protocols::SecureChannel;. Again, just to make things a bit shorter; it's not a big deal either way.

@bzbarsky-apple bzbarsky-apple merged commit 0e55cf8 into project-chip:master Dec 18, 2020
@yufengwangca yufengwangca deleted the pr/transport/sc branch December 22, 2020 19:27
kpschoedel pushed a commit to kpschoedel/connectedhomeip that referenced this pull request Jan 8, 2021
@yufengwangca @kpschoedel
Align Spake2 protocol message type definition with spec (project-chip…
53476c9 
…#4167)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bzbarsky-apple bzbarsky-apple bzbarsky-apple approved these changes

@pan-apple pan-apple pan-apple left review comments

@jelderton jelderton jelderton approved these changes

@andy31415 andy31415 andy31415 approved these changes

@mspang mspang mspang approved these changes

@BroderickCarlin BroderickCarlin Awaiting requested review from BroderickCarlin

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@hawk248 hawk248 Awaiting requested review from hawk248

@rwalker-apple rwalker-apple Awaiting requested review from rwalker-apple

@saurabhst saurabhst Awaiting requested review from saurabhst

Assignees
No one assigned
Labels
transport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yufengwangca @bzbarsky-apple @mspang @andy31415 @pan-apple @jelderton