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.
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
Enable pairing for multiple devices #3630
Enable pairing for multiple devices #3630
Changes from all commits
f6af1c6
01ff710
5276eff
b942ca2
eef4af3
4109482
4477c9a
77fe0a9
4c8e760
bc28ddd
32dd372
1f2122d
9abf333
d194a2d
d7f176b
5d4854b
12d1714
1e38ed2
aa4d514
92941ef
ea46ff7
a738bbc
446d183
f1bbc69
2e715e5
305d924
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please file a followup issue to give
SecurePairingSessionSerializable
anoperator=
(which can presumably be= default
) and reference it here? Nothing obviously indicates that a serializable thing can just be memmoved...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #3761
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are both a little weird, in that those functions are defined to swap little-to-host, not host-to-little. It happens to be true that little-to-host done twice is the identity, but it's still weird to depend on that. I thought we were going to add a host-to-little thing to CHIPEncoding.h? I definitely recall that coming up in a previous PR....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would find some LittleEndian::Put16/Get16/Write16/Read16 more readable during serialization. As it is, I have a hard time figuring out the endianess of things. Not even sure what HostSwap* would do (why do we even have those named fuctions?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree on the function naming. There are other instances of code which can use the refactor. Probably better to do a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, separate PR is fine, but we should make sure it happens....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue: #3762