Move WakeIOThread() functionality to WatchableEventManager #8456

kpschoedel · 2021-07-16T14:36:00Z

Problem

System::Layer contains WakeIOThread() and mWakeEvent which are
really implementation details of WatchableEventManager.

Change overview

Replace System::Layer::WakeIOThread() with
WatchableEventManager::Signal(), because not every implementation
will necessarily implement the nudge by waking a thread.
Split SystemSockets.h into WatchableEventManager.h,
WatchableSocket.h, and WakeEvent.h; likewise split the
various implementation files.
Make the System::WakeEvent public API independent of the
file-descriptor implementation.

For the present, mHandleSelectThread remains in System::Layer
until timers are integrated and the transitional HandleTimeout()
is removed.

Testing

Sanity check with chip-tool. CI should verify that functionality does
not regress.

#### Problem System::Layer contains `WakeIOThread()` and `mWakeEvent` which are really implementation details of `WatchableEventManager`. #### Change overview - Replace `System::Layer::WakeIOThread()` with `WatchableEventManager::Signal()`, because not every implementation will necessarily implement the nudge by waking a thread. - Split `SystemSockets.h` into `WatchableEventManager.h`, `WatchableSocket.h`, and `WakeEvent.h`; likewise split the various implementation files. - Make the `System::WakeEvent` public API independent of the file-descriptor implementation. For the present, `mHandleSelectThread` remains in `System::Layer` until timers are integrated and the transitional `HandleTimeout()` is removed. #### Testing Sanity check with chip-tool. CI should verify that functionality does not regress.

todo · 2021-07-16T14:36:03Z

(#5556): default to off

connectedhomeip/src/system/WatchableEventManagerLibevent.cpp

Lines 41 to 51 in 26a7e98

    
           #define CHIP_CONFIG_LIBEVENT_DEBUG_CHECKS 1 // TODO(#5556): default to off 
        
           #endif 
        
           namespace chip { 
        
           namespace System { 
        
           namespace { 
        
           System::SocketEvents SocketEventsFromLibeventFlags(short eventFlags) 
        
           { 
        
               return System::SocketEvents()

This comment was generated by todo based on a `TODO` comment in `26a7e98` in #8456. cc @kpschoedel.

todo · 2021-07-16T14:36:06Z

(#5556): Integrate timer platform details with WatchableEventManager.

connectedhomeip/src/system/WatchableEventManagerLibevent.cpp

Lines 83 to 93 in 26a7e98

    
               // TODO(#5556): Integrate timer platform details with WatchableEventManager. 
        
               timeval nextTimeout = { 0, 0 }; 
        
               PrepareEventsWithTimeout(nextTimeout); 
        
           } 
        
           void WatchableEventManager::PrepareEventsWithTimeout(struct timeval & nextTimeout) 
        
           { 
        
               // TODO(#5556): Integrate timer platform details with WatchableEventManager. 
        
               mSystemLayer->GetTimeout(nextTimeout); 
        
               if (nextTimeout.tv_sec || nextTimeout.tv_usec) 
        
               {

This comment was generated by todo based on a `TODO` comment in `26a7e98` in #8456. cc @kpschoedel.

todo · 2021-07-16T14:36:09Z

(#5556): Integrate timer platform details with WatchableEventManager.

connectedhomeip/src/system/WatchableEventManagerLibevent.cpp

Lines 90 to 100 in 26a7e98

    
               // TODO(#5556): Integrate timer platform details with WatchableEventManager. 
        
               mSystemLayer->GetTimeout(nextTimeout); 
        
               if (nextTimeout.tv_sec || nextTimeout.tv_usec) 
        
               { 
        
                   evtimer_add(mTimeoutEvent, &nextTimeout); 
        
               } 
        
           } 
        
           void WatchableEventManager::WaitForEvents() 
        
           { 
        
               VerifyOrDie(mEventBase != nullptr);

This comment was generated by todo based on a `TODO` comment in `26a7e98` in #8456. cc @kpschoedel.

todo · 2021-07-16T14:36:12Z

(#5556): Some unit tests supply a timeout at low level, due to originally using select(); these should a proper timer.

connectedhomeip/src/system/WatchableEventManagerLibevent.h

Lines 50 to 60 in 26a7e98

    
               // TODO(#5556): Some unit tests supply a timeout at low level, due to originally using select(); these should a proper timer. 
        
               void PrepareEventsWithTimeout(timeval & nextTimeout); 
        
           private: 
        
               /* 
        
                * In this implementation, libevent invokes LibeventCallbackHandler from beneath WaitForEvents(), 
        
                * which means that the CHIP stack is unlocked. LibeventCallbackHandler adds the WatchableSocket 
        
                * to a queue (implemented as a simple intrusive list to avoid dynamic memory allocation), and 
        
                * then HandleEvents() invokes the WatchableSocket callbacks. 
        
                */ 
        
               friend class WatchableSocket;

This comment was generated by todo based on a `TODO` comment in `26a7e98` in #8456. cc @kpschoedel.

todo · 2021-07-16T14:36:15Z

(#5556): Integrate timer platform details with WatchableEventManager.

connectedhomeip/src/system/WatchableEventManagerSelect.cpp

Lines 186 to 196 in 26a7e98

    
               // TODO(#5556): Integrate timer platform details with WatchableEventManager. 
        
               mSystemLayer->GetTimeout(nextTimeout); 
        
           #if CHIP_DEVICE_CONFIG_ENABLE_MDNS && !__ZEPHYR__ && !__MBED__ 
        
               chip::Mdns::GetMdnsTimeout(nextTimeout); 
        
           #endif // CHIP_DEVICE_CONFIG_ENABLE_MDNS && !__ZEPHYR__ 
        
               mSelected = mRequest; 
        
           } 
        
           void WatchableEventManager::WaitForEvents()

This comment was generated by todo based on a `TODO` comment in `26a7e98` in #8456. cc @kpschoedel.

todo · 2021-07-16T14:36:18Z

(#5556): Some unit tests supply a timeout at low level, due to originally using select(); these should a proper timer.

connectedhomeip/src/system/WatchableEventManagerSelect.h

Lines 54 to 64 in 26a7e98

    
               // TODO(#5556): Some unit tests supply a timeout at low level, due to originally using select(); these should a proper timer. 
        
               void PrepareEventsWithTimeout(timeval & nextTimeout); 
        
               static SocketEvents SocketEventsFromFDs(int socket, const fd_set & readfds, const fd_set & writefds, const fd_set & exceptfds); 
        
           protected: 
        
               friend class WatchableSocket; 
        
               void Set(int fd, fd_set * fds); 
        
               void Clear(int fd, fd_set * fds);

This comment was generated by todo based on a `TODO` comment in `26a7e98` in #8456. cc @kpschoedel.

todo · 2021-07-16T14:36:21Z

(#5556): Integrate timer platform details with WatchableEventManager.

connectedhomeip/src/system/WatchableEventManagerSelect.h

Lines 68 to 78 in 26a7e98

    
           // TODO(#5556): Integrate timer platform details with WatchableEventManager. 
        
           struct timeval mNextTimeout; 
        
           // Members for select loop 
        
           struct SelectSets 
        
           { 
        
               fd_set mReadSet; 
        
               fd_set mWriteSet; 
        
               fd_set mErrorSet; 
        
           }; 
        
           SelectSets mRequest;

This comment was generated by todo based on a `TODO` comment in `26a7e98` in #8456. cc @kpschoedel.

kpschoedel · 2021-07-16T14:47:01Z

Reviewers — github's presentation of this change obscures what's going on, since you can't tell which parts of the split files are actually new. ~~Give me a moment to make gists with readable diffs.~~ See links to diffs below.

src/system/WatchableEventManagerLibevent.h

kpschoedel · 2021-07-16T15:07:05Z

SystemSockets.h is split into
WatchableSocketLibevent.h spawns WatchableEventManagerLibevent.h
WatchableSocketLibevent.cpp spawns WatchableEventManagerLibevent.cpp
WatchableSocketSelect.h spawns WatchableEventManagerSelect.h
WatchableSocketSelect.cpp spawns WatchableEventManagerSelect.cpp

src/system/WakeEvent.h

src/system/WatchableEventManager.h

src/system/WatchableEventManagerLibevent.cpp

src/system/WatchableEventManagerLibevent.h

src/system/WatchableEventManagerSelect.cpp

src/system/WatchableEventManagerSelect.h

github-actions · 2021-07-16T18:17:14Z

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

File	Section	File	VM
chip-all-clusters-app.elf	.flash.text	16	16
chip-lock-app.elf	.flash.text	64	64
chip-temperature-measurement-app.elf	.flash.text	-60	-60

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.xt.lit._ZN4chip6System5Mutex6UnlockEv,0,128
.xt.lit._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE10_PostEventEPKNS0_15ChipDeviceEventE,0,80
.xt.prop._ZN4chip11DeviceLayer8Internal26GenericPlatformManagerImplINS0_19PlatformManagerImplEE14_InitChipStackEv,0,40
.flash.text,16,16
.debug_loc,0,4
.debug_info,0,-5
.debug_abbrev,0,-7
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE14_LockChipStackEv,0,-12
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE15_StartChipTimerEj,0,-12
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE16_UnlockChipStackEv,0,-12
[Unmapped],0,-16
.xt.lit._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE14_LockChipStackEv,0,-48
.xt.lit._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE15_StartChipTimerEj,0,-48
.xt.lit._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE16_UnlockChipStackEv,0,-48
.xt.prop._ZN4chip6System5Mutex6UnlockEv,0,-108

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

sections,vmsize,filesize
.debug_loc,0,-4
.debug_info,0,-5
.debug_abbrev,0,-7

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

sections,vmsize,filesize
.flash.text,64,64
.debug_loc,0,4
.debug_info,0,-5
.debug_abbrev,0,-7
[Unmapped],0,-64

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize
.debug_loc,0,-4
.debug_info,0,-5
.debug_abbrev,0,-7
.flash.text,-60,-60
[Unmapped],0,-4036

github-actions · 2021-07-16T18:18:17Z

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

File	Section	File	VM
chip-lock.elf	device_handles	12	12
chip-lock.elf	text	-12	-12
chip-shell.elf	text	-16	-16

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,10929
.debug_line,0,5624
.debug_abbrev,0,1939
.debug_loc,0,338
.debug_frame,0,68
.strtab,0,52
.debug_aranges,0,48
.debug_ranges,0,48
.symtab,0,16
device_handles,12,12
.debug_str,0,-10
text,-12,-12

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

sections,vmsize,filesize
.debug_info,0,12231
.debug_line,0,3972
.debug_abbrev,0,1917
.debug_loc,0,310
.debug_frame,0,68
.strtab,0,52
.debug_aranges,0,48
.debug_ranges,0,48
.symtab,0,16
.debug_str,0,-10
text,-16,-16

bzbarsky-apple

Thank you for the links to actually-useful diffs!

…leEventManager #### Problem TestPlatformMgr reports errors, ``` CHIP:CSL: System wake event notify failed: OS Error 0x02000009: Bad file descriptor ``` (although it still passes, which obscured the problem). This might be behind intermittent core dumps in CI. #### Change overview Revert project-chip#8456 Move WakeIOThread() functionality to WatchableEventManager #### Testing No error reported by TestPlatformMgr after reverting.

…hip#8456) #### Problem System::Layer contains `WakeIOThread()` and `mWakeEvent` which are really implementation details of `WatchableEventManager`. #### Change overview - Replace `System::Layer::WakeIOThread()` with `WatchableEventManager::Signal()`, because not every implementation will necessarily implement the nudge by waking a thread. - Split `SystemSockets.h` into `WatchableEventManager.h`, `WatchableSocket.h`, and `WakeEvent.h`; likewise split the various implementation files. - Make the `System::WakeEvent` public API independent of the file-descriptor implementation. For the present, `mHandleSelectThread` remains in `System::Layer` until timers are integrated and the transitional `HandleTimeout()` is removed. #### Testing Sanity check with chip-tool. CI should verify that functionality does not regress.

boring-cyborg bot added controller inet linux platform system labels Jul 16, 2021

pullapprove bot requested review from andy31415, chrisdecenzo, gerickson, jelderton, jmartinez-silabs, LuDuda, mspang, saurabhst, vivien-apple and woody-apple July 16, 2021 14:37

pullapprove bot added the review - pending label Jul 16, 2021

andy31415 approved these changes Jul 16, 2021

View reviewed changes

src/system/WatchableEventManagerLibevent.h Show resolved Hide resolved

kpschoedel commented Jul 16, 2021

View reviewed changes

bzbarsky-apple approved these changes Jul 16, 2021

View reviewed changes

jmartinez-silabs approved these changes Jul 19, 2021

View reviewed changes

pullapprove bot added review - approved and removed review - pending labels Jul 19, 2021

mspang approved these changes Jul 19, 2021

View reviewed changes

mspang merged commit fa6033f into project-chip:master Jul 19, 2021

todo bot mentioned this pull request Jul 19, 2021

(#5556): Some unit tests supply a timeout at low level, due to originally using ... #8484

Closed

kpschoedel deleted the x7725-system-event-3 branch July 19, 2021 20:37

kpschoedel mentioned this pull request Jul 21, 2021

Revert #8456 Move WakeIOThread() functionality to WatchableEventManager #8542

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move WakeIOThread() functionality to WatchableEventManager #8456

Move WakeIOThread() functionality to WatchableEventManager #8456

kpschoedel commented Jul 16, 2021

todo bot commented Jul 16, 2021

todo bot commented Jul 16, 2021

todo bot commented Jul 16, 2021

todo bot commented Jul 16, 2021

todo bot commented Jul 16, 2021

todo bot commented Jul 16, 2021

todo bot commented Jul 16, 2021

kpschoedel commented Jul 16, 2021 •

edited

Loading

kpschoedel commented Jul 16, 2021

github-actions bot commented Jul 16, 2021

github-actions bot commented Jul 16, 2021

bzbarsky-apple left a comment

Move WakeIOThread() functionality to WatchableEventManager #8456

Move WakeIOThread() functionality to WatchableEventManager #8456

Conversation

kpschoedel commented Jul 16, 2021

Problem

Change overview

Testing

todo bot commented Jul 16, 2021

(#5556): default to off

This comment was generated by todo based on a TODO comment in 26a7e98 in #8456. cc @kpschoedel.

todo bot commented Jul 16, 2021

(#5556): Integrate timer platform details with WatchableEventManager.

This comment was generated by todo based on a TODO comment in 26a7e98 in #8456. cc @kpschoedel.

todo bot commented Jul 16, 2021

(#5556): Integrate timer platform details with WatchableEventManager.

This comment was generated by todo based on a TODO comment in 26a7e98 in #8456. cc @kpschoedel.

todo bot commented Jul 16, 2021

(#5556): Some unit tests supply a timeout at low level, due to originally using select(); these should a proper timer.

This comment was generated by todo based on a TODO comment in 26a7e98 in #8456. cc @kpschoedel.

todo bot commented Jul 16, 2021

(#5556): Integrate timer platform details with WatchableEventManager.

This comment was generated by todo based on a TODO comment in 26a7e98 in #8456. cc @kpschoedel.

todo bot commented Jul 16, 2021

(#5556): Some unit tests supply a timeout at low level, due to originally using select(); these should a proper timer.

This comment was generated by todo based on a TODO comment in 26a7e98 in #8456. cc @kpschoedel.

todo bot commented Jul 16, 2021

(#5556): Integrate timer platform details with WatchableEventManager.

This comment was generated by todo based on a TODO comment in 26a7e98 in #8456. cc @kpschoedel.

kpschoedel commented Jul 16, 2021 • edited Loading

kpschoedel commented Jul 16, 2021

github-actions bot commented Jul 16, 2021

github-actions bot commented Jul 16, 2021

bzbarsky-apple left a comment

Choose a reason for hiding this comment

This comment was generated by todo based on a `TODO` comment in `26a7e98` in #8456. cc @kpschoedel.

This comment was generated by todo based on a `TODO` comment in `26a7e98` in #8456. cc @kpschoedel.

This comment was generated by todo based on a `TODO` comment in `26a7e98` in #8456. cc @kpschoedel.

This comment was generated by todo based on a `TODO` comment in `26a7e98` in #8456. cc @kpschoedel.

This comment was generated by todo based on a `TODO` comment in `26a7e98` in #8456. cc @kpschoedel.

This comment was generated by todo based on a `TODO` comment in `26a7e98` in #8456. cc @kpschoedel.

This comment was generated by todo based on a `TODO` comment in `26a7e98` in #8456. cc @kpschoedel.

kpschoedel commented Jul 16, 2021 •

edited

Loading