8000 Handle Thread Association Request in rendezvous session by Damian-Nordic · Pull Request #3303 · project-chip/connectedhomeip · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Handle Thread Association Request in rendezvous session #3303

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
Merged
merged 7 commits into from
Oct 26, 2020
Merged

Handle Thread Association Request in rendezvous session #3303

merged 7 commits into from
Oct 26, 2020

Conversation

Damian-Nordic
Copy link
Contributor

Problem

Currently, NetworkProvisioning class, which takes part in Rendezvous over BLE session, only supports WiFi provisioning and Thread provisioning is partially implemented in RendezvousServer (Application layer).

Proposed Solution

  • move the Thread provisioning code to the BLE transport layer to process both network types in the same/similar way
  • reply on Thread Association request with SLAAC/On-Mesh IPv6 Address once that is assigned so that the controller knows that provisioning completed successfully and how to reach the device
  • call MemoryInit() in nrfconnect examples

Fixes #3302

@Damian-Nordic Damian-Nordic changed the title Feature/provision Handle Thread Association Request in rendezvous session Oct 19, 2020
andy31415
andy31415 approved these changes Oct 19, 2020
View reviewed changes
src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp Outdated
}
}

return CHIP_ERROR_INCORRECT_STATE;
Copy link
Contributor

Choose a reason for hiding this comment

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

would NOT_FOUND be a more accurate return code? or is this really about state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I also felt CHIP_ERROR_INCORRECT_STATE might not be accurate as it suggests an application error, but I couldn't find a generic NOT_FOUND in the error list. Do you think I should add CHIP_ERROR_NOT_FOUND?

Copy link
Contributor
@andy31415 andy31415 Oct 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I thought one existed (almost any error code list has such a thing, along with things like 'invalid argument' or 'out of range'). I am very surprised we do not have one :( maybe we should add it, since really this is a search that returns not found.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found CHIP_DEVICE_ERROR_CONFIG_NOT_FOUND which seems suitable so I used it here. Let me know if that's better.

@mspang mspang requested a review from gjc13 October 20, 2020 01:09
gjc13
gjc13 reviewed Oct 20, 2020
View reviewed changes
examples/common/chip-app-server/RendezvousServer.cpp Outdated
@@ -34,7 +33,16 @@ using namespace ::chip::DeviceLayer;

namespace chip {

RendezvousServer::RendezvousServer() : mRendezvousSession(this) {}
void AccessoryNetworkProvisioningDelegate::ProvisionThread(const DeviceLayer::Internal::DeviceNetworkInfo & threadData)
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to AccessoryNetworkProvisioningDelegate.cpp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand. Would you like me to move the class to a new header/source file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modified it a bit 8000 so that RendezvousServer implements DeviceNetworkProvisioningDelegate directly. Is it OK now?

bukepo
bukepo approved these changes Oct 20, 2020
View reviewed changes
Copy link
Contributor
@bukepo bukepo left a comment

Choose a reason for hiding this comment

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

LGTM:+1:

wgtdkp
wgtdkp approved these changes Oct 20, 2020
View reviewed changes
Copy link
Contributor
@wgtdkp wgtdkp left a comment

Choose a reason for hiding this comment

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

LGTM 👍

examples/common/chip-app-server/include/RendezvousServer.h Outdated
@@ -22,6 +22,11 @@

namespace chip {

class AccessoryNetworkProvisioningDelegate : public DeviceNetworkProvisioningDelegate
{
void ProvisionThread(const DeviceLayer::Internal::DeviceNetworkInfo & threadData) override;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
void ProvisionThread(const DeviceLayer::Internal::DeviceNetworkInfo & threadData) override;
public:
void ProvisionThread(const DeviceLayer::Internal::DeviceNetworkInfo & threadData) override;

@Damian-Nordic Damian-Nordic mentioned this pull request Oct 20, 2020
Closed
@github-actions
Copy link

Size increase report for "nrfconnect-example-build"

File Section File VM
chip-lock.elf text 288 288
chip-lock.elf rodata 120 120
chip-lighting.elf text 288 288
chip-lighting.elf rodata 120 120
chip-lighting.elf [LOAD #3 [RW]] 0 31
chip-lighting.elf bss 0 1
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,15465
.debug_line,0,1103
.debug_str,0,882
.debug_loc,0,774
.strtab,0,597
.debug_abbrev,0,436
.symtab,0,304
text,288,288
.debug_ranges,0,256
.debug_frame,0,172
rodata,120,120
.debug_aranges,0,72
.shstrtab,0,3

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

sections,vmsize,filesize
.debug_info,0,777
.debug_str,0,252
.debug_loc,0,120
.debug_line,0,115
.debug_ranges,0,40
.debug_frame,0,32
.debug_abbrev,0,8
.debug_aranges,0,8

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

sections,vmsize,filesize
.debug_info,0,15465
.debug_line,0,1099
.debug_str,0,882
.debug_loc,0,778
.strtab,0,597
.debug_abbrev,0,436
.symtab,0,304
text,288,288
.debug_ranges,0,256
.debug_frame,0,172
rodata,120,120
.debug_aranges,0,72
[LOAD #3 [RW]],31,0
bss,1,0
.shstrtab,0,-1

@Damian-Nordic Damian-Nordic mentioned this pull request Oct 22, 2020
Merged
@BroderickCarlin
Copy link
Contributor

@Damian-Nordic Can you address the merge conflicts? Should be good to go after that

@Damian-Nordic
[transport] Handle Thread Association request
353e1fa 
Currently, NetworkProvisioning class, which takes part in
rendezvous over BLE session, only supports WiFi provisioning
and Thread provisioning is partially implemented in
RendezvousServer (Application layer). Move the Thread
provisioning code to the BLE transport layer to process
both network types in the same way.

Also reply to Thread Association request with SLAAC/On-Mesh
IPv6 Address once that is assigned so that the controller
knows that provisioning completed successfully and how to
reach the device.
@Damian-Nordic
[nrfconnect] Initialize memory allocator
6200257 
More and more CHIP components rely on CHIP memory allocator.
Also PlatformManager::AddEventHandler() used in this PR
needs dynamic memory allocation. Run MemoryInit() on
the app startup.
@Damian-Nordic
[esp32] Fix build
5959793
@Damian-Nordic
Fix typo and use more accurate error code
2bbfbda
@Damian-Nordic
Let RendezvousServer implement DeviceNetworkProvisioningDelegate
20bce10
@Damian-Nordic
Fix after rebase
bf4739e
@Damian-Nordic
Copy link
Contributor Author

@Damian-Nordic Can you address the merge conflicts? Should be good to go after that

Yes. I've rebased the changes.

@github-actions
Copy link

Size increase report for "nrfconnect-example-build" from 5ba0597

File Section File VM
chip-lighting.elf text 344 344
chip-lighting.elf rodata 120 120
chip-lighting.elf bss 0 19
chip-lighting.elf shell_root_cmds_sections -8 -8
chip-lighting.elf [LOAD #3 [RW]] 0 -19
chip-lock.elf text 344 344
chip-lock.elf rodata 120 120
chip-lock.elf bss 0 4
chip-lock.elf [LOAD #3 [RW]] 0 -4
chip-lock.elf shell_root_cmds_sections -8 -8
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,27019
.debug_line,0,3075
.debug_abbrev,0,2330
.debug_loc,0,999
.debug_str,0,997
.strtab,0,699
.symtab,0,400
text,344,344
.debug_ranges,0,304
.debug_frame,0,244
rodata,120,120
.debug_aranges,0,112
bss,19,0
.shstrtab,0,1
shell_root_cmds_sections,-8,-8
[LOAD #3 [RW]],-19,0

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

sections,vmsize,filesize
.debug_info,0,27019
.debug_line,0,3079
.debug_abbrev,0,2330
.debug_str,0,997
.debug_loc,0,995
.strtab,0,699
.symtab,0,400
text,344,344
.debug_ranges,0,304
.debug_frame,0,244
rodata,120,120
.debug_aranges,0,112
bss,4,0
.shstrtab,0,1
[LOAD #3 [RW]],-4,0
shell_root_cmds_sections,-8,-8

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

sections,vmsize,filesize
.debug_info,0,777
.debug_str,0,252
.debug_loc,0,120
.debug_line,0,115
.debug_ranges,0,40
.debug_frame,0,32
.debug_abbrev,0,8
.debug_aranges,0,8

@BroderickCarlin BroderickCarlin merged commit 960cfe0 into project-chip:master Oct 26, 2020
@Damian-Nordic Damian-Nordic deleted the feature/provision branch October 28, 2020 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andy31415 andy31415 andy31415 approved these changes

@mspang mspang 9E88 mspang approved these changes

@bukepo bukepo bukepo approved these changes

@wgtdkp wgtdkp wgtdkp approved these changes

@gjc13 gjc13 gjc13 approved these changes

@BroderickCarlin BroderickCarlin BroderickCarlin approved these changes

@rwalker-apple rwalker-apple rwalker-apple approved these changes

@bzbarsky-apple bzbarsky-apple Awaiting requested review from bzbarsky-apple

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@hawk248 hawk248 Awaiting requested review from hawk248

@jelderton jelderton Awaiting requested review from jelderton

@saurabhst saurabhst Awaiting requested review from saurabhst

Assignees
No one assigned
Labels
examples linux openthread platform transport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handle Thread Association Request in Rendezvous session
8 participants
@Damian-Nordic @BroderickCarlin @mspang @andy31415 @bukepo @wgtdkp @gjc13 @rwalker-apple
0