Add CHIP stack module. #6967

kghost · 2021-05-19T13:56:41Z

Problem

CHIP stack initialization and shutdown are spread around. There are lots of duplicated code in all test cases and applications.

Change overview

This PR add a centralized class to initialize and shutdown all chip stack singletons.

Testing

This PR is a refactor, unit tests are used for verify.

I have manually tested echo requester/responder and im requester/responder. All verified are working.

src/stack/Stack.h

src/lib/core/CHIPConfig.h

src/stack/Stack.h

src/stack/Traits.h

todo · 2021-05-19T16:19:03Z

(#6931): CHIP initialization should be posted to chip thread, in order to avoid racing problems

connectedhomeip/src/stack/Stack.h

Lines 207 to 217 in 0eeefa6

    
               // TODO(#6931): CHIP initialization should be posted to chip thread, in order to avoid racing problems 
        
               CHIP_ERROR Init(const StackParameters & parameters) 
        
               { 
        
                   ReturnErrorOnFailure(chip::Platform::MemoryInit()); 
        
           #if CONFIG_DEVICE_LAYER 
        
                   // Initialize the CHIP stack. 
        
                   ReturnErrorOnFailure(chip::DeviceLayer::PlatformMgr().InitChipStack()); 
        
           #endif 
        
                   ReturnErrorOnFailure(mSystemLayer.Init(parameters)); 
        
                   ReturnErrorOnFailure(mInetLayer.Init(parameters, GetSystemLayer())); 
        
                   ReturnErrorOnFailure(mStorage.Init(parameters));

This comment was generated by todo based on a `TODO` comment in `0eeefa6` in #6967. cc @kghost.

todo · 2021-05-19T16:19:06Z

(#6931): CHIP shutdown should be posted to chip thread, in order to avoid racing problems

connectedhomeip/src/stack/Stack.h

Lines 232 to 242 in 0eeefa6

    
               // TODO(#6931): CHIP shutdown should be posted to chip thread, in order to avoid racing problems 
        
               CHIP_ERROR Shutdown() 
        
               { 
        
                   mMessageCounterManager.Shutdown(); 
        
                   mExchangeManager.Shutdown(); 
        
                   mSessionManager.Shutdown(); 
        
                   mTransport.Shutdown(); 
        
                   mBleLayer.Shutdown(); 
        
                   mInetLayer.Shutdown(); 
        
                   mSystemLayer.Shutdown(); 
        
           #if CONFIG_DEVICE_LAYER

This comment was generated by todo based on a `TODO` comment in `0eeefa6` in #6967. cc @kghost.

mspang · 2021-05-19T21:34:14Z

This PR adds more code than it removes...

mspang · 2021-05-19T21:36:55Z

This PR adds more code than it removes...

I'm also not convinced it's a step forward in maintainability. Now everyone will have to wrap their head around all of the metaprogramming to understand how to build a ChipStack. We should keep it simple.

andy31415 · 2021-05-21T15:04:19Z

src/test_driver/esp32/main/main_app.cpp

-        exit(err);
-    }
-
-    err = Crypto::add_entropy_source(app_entropy_source, NULL, 16);


crypto tests sound as if they may want an entropy source. Event loop also seems potentially important.

I would understand 'esp tests do not use wifi' because qemu, however this should be documented and probably a separate PR from the stack module unless it interferes with that...and if it does please explain why.

This are duplicated in PlatformManagerImpl::_InitChipStack

connectedhomeip/src/platform/ESP32/PlatformManagerImpl.cpp

Lines 56 to 107 in 82a8544

CHIP_ERROR PlatformManagerImpl::_InitChipStack(void)

{

CHIP_ERROR err;

wifi_init_config_t cfg;

uint8_t ap_mac[6];

wifi_mode_t mode;

// Make sure the LwIP core lock has been initialized

err = Internal::InitLwIPCoreLock();

SuccessOrExit(err);

err = esp_netif_init();

SuccessOrExit(err);

// Arrange for the ESP event loop to deliver events into the CHIP Device layer.

err = esp_event_loop_create_default();

SuccessOrExit(err);

esp_netif_create_default_wifi_ap();

esp_netif_create_default_wifi_sta();

esp_event_handler_register(WIFI_EVENT, ESP_EVENT_ANY_ID, PlatformManagerImpl::HandleESPSystemEvent, NULL);

SuccessOrExit(err);

esp_event_handler_register(IP_EVENT, ESP_EVENT_ANY_ID, PlatformManagerImpl::HandleESPSystemEvent, NULL);

SuccessOrExit(err);

// Initialize the ESP WiFi layer.

cfg = WIFI_INIT_CONFIG_DEFAULT();

err = esp_wifi_init(&cfg);

SuccessOrExit(err);

esp_wifi_get_mode(&mode);

if ((mode == WIFI_MODE_AP) || (mode == WIFI_MODE_APSTA))

{

esp_fill_random(ap_mac, sizeof(ap_mac));

/* Bit 0 of the first octet of MAC Address should always be 0 */

ap_mac[0] &= (uint8_t) ~0x01;

err = esp_wifi_set_mac(ESP_IF_WIFI_AP, ap_mac);

SuccessOrExit(err);

}

// Call _InitChipStack() on the generic implementation base class

// to finish the initialization process.

err = Internal::GenericPlatformManagerImpl_FreeRTOS<PlatformManagerImpl>::_InitChipStack();

SuccessOrExit(err);

err = chip::Crypto::add_entropy_source(app_entropy_source, NULL, 16);

SuccessOrExit(err);

exit:

return err;

}

todo · 2021-05-25T06:28:43Z

(#7076): esp_wifi_init is pretty unstable on esp32_qemu

connectedhomeip/src/include/platform/internal/GenericPlatformManagerImpl.cpp

Lines 98 to 101 in 97b1b16

    
           // TODO(#7076): esp_wifi_init is pretty unstable on esp32_qemu 
        
           // Initialize the Connectivity Manager object. 
        
           err = ConnectivityMgr().Init(); 
        
           if (err != CHIP_NO_ERROR)

This comment was generated by todo based on a `TODO` comment in `97b1b16` in #6967. cc @kghost.

todo · 2021-05-25T06:28:46Z

(#7076): esp_wifi_init is pretty unstable on esp32_qemu

connectedhomeip/src/platform/ESP32/PlatformManagerImpl.cpp

Lines 91 to 94 in 97b1b16

    
           // TODO(#7076): esp_wifi_init is pretty unstable on esp32_qemu 
        
           // Initialize the ESP WiFi layer. 
        
           cfg = WIFI_INIT_CONFIG_DEFAULT(); 
        
           err = esp_wifi_init(&cfg);

This comment was generated by todo based on a `TODO` comment in `97b1b16` in #6967. cc @kghost.

* remove duplicated initialization code, in the following files: * PlatformManagerImpl::_InitChipStack in src/platform/ESP32/PlatformManagerImpl.cpp * app_main in src/test_driver/esp32/main/main_app.cpp * Do not create socket during tests. * ESP32 frees socket async, but system layer is shutdown before socket is freed, so all socket are leaked when shutting down chip stack. * This is fine for production, but tests are running in a batch, creating too many sockets will exhaust the socket pool. * Make PlatformManagerImpl::_InitChipStack reentrant * Same reason as above, tests are running in a batch, but there is no way to shutdown PlatformManager, so _InitChipStack is skipped on the following test cases.

github-actions · 2021-05-26T10:55:27Z

Size increase report for "nrfconnect-example-build" from ebd9e0a

File	Section	File	VM
chip-lock.elf	text	92	92
chip-lock.elf	bss	0	32
chip-lock.elf	rodata	16	16
chip-lock.elf	device_handles	4	4
chip-lock.elf	datas	-4	-4
chip-lock.elf	devices	-4	-4
chip-shell.elf	bss	0	777
chip-shell.elf	[LOAD #3 [RW]]	0	23
chip-shell.elf	device_handles	-4	-4
chip-shell.elf	rodata	-24	-24
chip-shell.elf	text	-140	-140

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,7243
.debug_str,0,4190
.debug_loc,0,1974
.debug_line,0,629
.debug_ranges,0,280
.debug_abbrev,0,252
text,92,92
.strtab,0,87
bss,32,0
rodata,16,16
device_handles,4,4
.shstrtab,0,-3
datas,-4,-4
devices,-4,-4
.debug_aranges,0,-24
.debug_frame,0,-56
.symtab,0,-80

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

sections,vmsize,filesize
.debug_info,0,38199
.debug_str,0,2479
.debug_line,0,1179
bss,777,0
.debug_abbrev,0,682
.debug_ranges,0,432
.debug_loc,0,409
.strtab,0,319
.symtab,0,96
[LOAD #3 [RW]],23,0
.shstrtab,0,-3
device_handles,-4,-4
.debug_aranges,0,-16
rodata,-24,-24
.debug_frame,0,-52
text,-140,-140

github-actions · 2021-05-26T10:55:35Z

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

File	Section	File	VM
chip-all-clusters-app.elf	.flash.text	192	192
chip-all-clusters-app.elf	.dram0.bss	0	40
chip-all-clusters-app.elf	.flash.rodata	16	16
chip-all-clusters-app.elf	.dram0.data	-8	-8

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,10601
.debug_str,0,5425
.debug_loc,0,2084
.debug_line,0,1773
.debug_ranges,0,824
.debug_abbrev,0,259
.strtab,0,219
.flash.text,192,192
.debug_frame,0,72
.dram0.bss,40,0
.debug_aranges,0,24
.flash.rodata,16,16
.shstrtab,0,1
.xt.prop._ZTVN4chip11DeviceLayer37DeviceNetworkProvisioningDelegateImplE,0,-2
.dram0.data,-8,-8
[Unmapped],0,-8
.symtab,0,-16

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

sections,vmsize,filesize

github-actions · 2021-05-26T10:56:21Z

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

File	Section	File	VM
chip-qpg6100-lighting-example.out	.text	112	112
chip-qpg6100-lighting-example.out	.bss	0	24
chip-qpg6100-lighting-example.out	.data	-4	-4
chip-qpg6100-lighting-example.out	.heap	0	-24

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,7254
.debug_str,0,4202
.debug_loc,0,1765
.debug_line,0,650
.debug_ranges,0,264
.debug_abbrev,0,253
.text,112,112
.strtab,0,87
[ELF Headers],0,32
.bss,24,0
.shstrtab,0,-3
.data,-4,-4
.debug_aranges,0,-24
.heap,-24,0
.debug_frame,0,-56
.symtab,0,-80
[Unmapped],0,-144

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'

Damian-Nordic · 2021-05-26T11:07:07Z

examples/shell/shell_common/include/Globals.h

+    static constexpr size_t kMaxTcpActiveConnectionCount = 4;
+    static constexpr size_t kMaxTcpPendingPackets        = 4;
+
+    using transport = chip::TransportMgr<


nit: I think we generally start type names with a capital letter. Change to "Transport"?

Damian-Nordic · 2021-05-26T11:13:46Z

src/app/server/Server.cpp

+        CHIP_ERROR err = CHIP_NO_ERROR;
+
+#if CHIP_DEVICE_LAYER_TARGET_DARWIN
+        err = PersistedStorage::KeyValueStoreMgrImpl().Init("chip.store");


idea: Should we add Init method to the common KeyValueStoreMgr interface, add some "storageLocation" parameter to StackParameters and unify this initialization code by implementing DefaultStorageConfiguration when the device layer is present?

This is a good suggestion, but this PR is only refactoring current code, I'd like it to be changed as less as possible. We can do it in the following up PRs.

Damian-Nordic · 2021-05-26T11:28:16Z

src/stack/Stack.h

+
+#pragma once
+
+#include <stack/Stack.h>


self-reference?

Damian-Nordic · 2021-05-26T11:30:16Z

src/stack/Stack.h

+#include <transport/TransportMgr.h>
+#include <transport/raw/UDP.h>
+
+#if CONFIG_DEVICE_LAYER


I think you can just include ChipDeviceLayer.h (unconditionally) which includes all necessary configs and PlatformManager.h.

src/stack/Stack.h

Damian-Nordic · 2021-05-26T11:45:10Z

src/stack/Stack.h

+        ReturnErrorOnFailure(Platform::MemoryInit());
+#if CONFIG_DEVICE_LAYER
+        // Initialize the CHIP stack.
+        ReturnErrorOnFailure(DeviceLayer::PlatformMgr().InitChipStack());


FYI: For some reason, currently ThreadStackMgr().InitThreadStack(); is also needed for Thread devices, so it either should be started here or incorporated into InitChipStack() when CHIP_DEVICE_CONFIG_ENABLE_THREAD

I saw that ThreadStackMgr().InitThreadStack() is called after InitChipStack() for platforms with thread support. Maybe we should separate hardware initialization and chip stack initialization, and leave InitChipStack() outside chip stack.

I'm not sure why ThreadStackManager can't be initialized by InitChipStack (which initializes other platform components). Maybe it's because ThreadStackManager can post events to the CHIP thread and the intention was not to initialize ThreadStackManager before the CHIP thread is started. Anyway, in case you plan to move the CHIP thread creation to the CHIP stack it would make sense to initialize ThreadStackManager in the comon code, too.

gerickson · 2021-06-01T17:51:18Z

To the comment, "2. Create the CHIP thread", per #6561, do we absolutely need a CHIP thread? Ideally, in the simples desktop or mobile app or in the simplest RTOS system, we could have a single thread that provides the animating context for a number of different sources of work.

woody-apple · 2021-06-04T23:57:46Z

/rebase

stale · 2021-06-12T00:35:57Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale · 2021-06-28T22:41:41Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale · 2021-07-05T23:29:55Z

This stale pull request has been automatically closed. Thank you for your contributions.

Damian-Nordic · 2021-07-23T14:12:40Z

@kghost Will the work be continued at some point?

boring-cyborg bot added app examples lib labels May 19, 2021

restyled-io bot mentioned this pull request May 19, 2021

Restyle Add CHIP stack module. #6968

Closed

kghost force-pushed the stack branch 2 times, most recently from 240cd4d to 9d8fd91 Compare May 19, 2021 14:39

pullapprove bot requested review from chrisdecenzo, jelderton, jmartinez-silabs, LuDuda, mspang, saurabhst, wehale and woody-apple May 19, 2021 14:45

pullapprove bot added the review - pending label May 19, 2021

andy31415 reviewed May 19, 2021

View reviewed changes

src/stack/Stack.h Show resolved Hide resolved

andy31415 reviewed May 19, 2021

View reviewed changes

src/lib/core/CHIPConfig.h Outdated Show resolved Hide resolved

andy31415 reviewed May 19, 2021

View reviewed changes

src/stack/Stack.h Outdated Show resolved Hide resolved

andy31415 reviewed May 19, 2021

View reviewed changes

src/stack/Stack.h Outdated Show resolved Hide resolved

Damian-Nordic reviewed May 19, 2021

View reviewed changes

src/stack/Traits.h Outdated Show resolved Hide resolved

src/stack/Traits.h Outdated Show resolved Hide resolved

restyled-io bot mentioned this pull request May 19, 2021

Restyle Add CHIP stack module. #6977

Closed

kghost force-pushed the stack branch from 0eeefa6 to 50840a3 Compare May 19, 2021 16:34

jmartinez-silabs approved these changes May 19, 2021

View reviewed changes

kghost force-pushed the stack branch from 50840a3 to 308c1fe Compare May 19, 2021 20:48

restyled-io bot mentioned this pull request May 19, 2021

Restyle Add CHIP stack module. #6984

Closed

tcarmelveilleux mentioned this pull request May 19, 2021

Get the SDK's Threading model sorted out #6841

Closed

andy31415 reviewed May 21, 2021

View reviewed changes

kghost force-pushed the stack branch from c3126d5 to 6ac16ed Compare May 21, 2021 15:54

kghost mentioned this pull request May 25, 2021

esp_wifi_init is pretty unstable on esp32_qemu #7076

Closed

kghost force-pushed the stack branch from 6ac16ed to 97b1b16 Compare May 25, 2021 06:28

boring-cyborg bot added the config label May 25, 2021

kghost force-pushed the stack branch from 97b1b16 to 8c740e2 Compare May 26, 2021 01:51

kghost added 4 commits May 26, 2021 17:13

Add CHIP stack module.

f533abc

Resolve comments && Fix CI

99fd08f

Resolve comments

5384e92

kghost force-pushed the stack branch from 8c740e2 to d588496 Compare May 26, 2021 09:23

ESP32 don't initialize wifi when doing unit test on qemu

d29fe7d

kghost force-pushed the stack branch from d588496 to d29fe7d Compare May 26, 2021 10:13

Damian-Nordic reviewed May 26, 2021

View reviewed changes

stale bot added the stale Stale issue or PR label Jun 12, 2021

woody-apple added the has conflicts label Jun 21, 2021

stale bot removed the stale Stale issue or PR label Jun 21, 2021

stale bot added the stale Stale issue or PR label Jun 28, 2021

stale bot closed this Jul 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CHIP stack module. #6967

Add CHIP stack module. #6967

kghost commented May 19, 2021 •

edited

Loading

todo bot commented May 19, 2021

todo bot commented May 19, 2021

mspang commented May 19, 2021

mspang commented May 19, 2021

andy31415 May 21, 2021

kghost May 21, 2021 •

edited

Loading

todo bot commented May 25, 2021

todo bot commented May 25, 2021

github-actions bot commented May 26, 2021

github-actions bot commented May 26, 2021

github-actions bot commented May 26, 2021

Damian-Nordic May 26, 2021

Damian-Nordic May 26, 2021

kghost May 26, 2021 •

edited

Loading

Damian-Nordic May 26, 2021

Damian-Nordic May 26, 2021

Damian-Nordic May 26, 2021

kghost May 26, 2021

Damian-Nordic May 26, 2021

gerickson commented Jun 1, 2021

woody-apple commented Jun 4, 2021

stale bot commented Jun 12, 2021

stale bot commented Jun 28, 2021

stale bot commented Jul 5, 2021

Damian-Nordic commented Jul 23, 2021

	CHIP_ERROR PlatformManagerImpl::_InitChipStack(void)
	{
	CHIP_ERROR err;
	wifi_init_config_t cfg;
	uint8_t ap_mac[6];
	wifi_mode_t mode;

	// Make sure the LwIP core lock has been initialized
	err = Internal::InitLwIPCoreLock();
	SuccessOrExit(err);

	err = esp_netif_init();
	SuccessOrExit(err);

	// Arrange for the ESP event loop to deliver events into the CHIP Device layer.
	err = esp_event_loop_create_default();
	SuccessOrExit(err);

	esp_netif_create_default_wifi_ap();
	esp_netif_create_default_wifi_sta();

	esp_event_handler_register(WIFI_EVENT, ESP_EVENT_ANY_ID, PlatformManagerImpl::HandleESPSystemEvent, NULL);
	SuccessOrExit(err);
	esp_event_handler_register(IP_EVENT, ESP_EVENT_ANY_ID, PlatformManagerImpl::HandleESPSystemEvent, NULL);
	SuccessOrExit(err);

	// Initialize the ESP WiFi layer.
	cfg = WIFI_INIT_CONFIG_DEFAULT();
	err = esp_wifi_init(&cfg);
	SuccessOrExit(err);

	esp_wifi_get_mode(&mode);
	if ((mode == WIFI_MODE_AP) \|\| (mode == WIFI_MODE_APSTA))
	{
	esp_fill_random(ap_mac, sizeof(ap_mac));
	/* Bit 0 of the first octet of MAC Address should always be 0 */
	ap_mac[0] &= (uint8_t) ~0x01;
	err = esp_wifi_set_mac(ESP_IF_WIFI_AP, ap_mac);
	SuccessOrExit(err);
	}

	// Call _InitChipStack() on the generic implementation base class
	// to finish the initialization process.
	err = Internal::GenericPlatformManagerImpl_FreeRTOS<PlatformManagerImpl>::_InitChipStack();
	SuccessOrExit(err);

	err = chip::Crypto::add_entropy_source(app_entropy_source, NULL, 16);
	SuccessOrExit(err);

	exit:
	return err;
	}


		#pragma once

		#include <stack/Stack.h>

Add CHIP stack module. #6967

Add CHIP stack module. #6967

Conversation

kghost commented May 19, 2021 • edited Loading

Problem

Change overview

Testing

todo bot commented May 19, 2021

(#6931): CHIP initialization should be posted to chip thread, in order to avoid racing problems

This comment was generated by todo based on a TODO comment in 0eeefa6 in #6967. cc @kghost.

todo bot commented May 19, 2021

(#6931): CHIP shutdown should be posted to chip thread, in order to avoid racing problems

This comment was generated by todo based on a TODO comment in 0eeefa6 in #6967. cc @kghost.

mspang commented May 19, 2021

mspang commented May 19, 2021

Choose a reason for hiding this comment

kghost May 21, 2021 • edited Loading

Choose a reason for hiding this comment

todo bot commented May 25, 2021

(#7076): esp_wifi_init is pretty unstable on esp32_qemu

This comment was generated by todo based on a TODO comment in 97b1b16 in #6967. cc @kghost.

todo bot commented May 25, 2021

(#7076): esp_wifi_init is pretty unstable on esp32_qemu

This comment was generated by todo based on a TODO comment in 97b1b16 in #6967. cc @kghost.

github-actions bot commented May 26, 2021

github-actions bot commented May 26, 2021

github-actions bot commented May 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kghost May 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerickson commented Jun 1, 2021

woody-apple commented Jun 4, 2021

stale bot commented Jun 12, 2021

stale bot commented Jun 28, 2021

stale bot commented Jul 5, 2021

Damian-Nordic commented Jul 23, 2021

kghost commented May 19, 2021 •

edited

Loading

This comment was generated by todo based on a `TODO` comment in `0eeefa6` in #6967. cc @kghost.

This comment was generated by todo based on a `TODO` comment in `0eeefa6` in #6967. cc @kghost.

kghost May 21, 2021 •

edited

Loading

This comment was generated by todo based on a `TODO` comment in `97b1b16` in #6967. cc @kghost.

This comment was generated by todo based on a `TODO` comment in `97b1b16` in #6967. cc @kghost.

kghost May 26, 2021 •

edited

Loading