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.

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

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

Closed
wants to merge 1 commit into from

Conversation

kpschoedel
Copy link
Contributor

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 #8456 Move WakeIOThread() functionality to WatchableEventManager

Testing

No error reported by TestPlatformMgr after reverting.

@todo
Copy link

todo bot commented Jul 21, 2021

(#5556): default to off

#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 45b23ba in #8542. cc @kpschoedel.

@todo
Copy link

todo bot commented Jul 21, 2021

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

// 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 45b23ba in #8542. cc @kpschoedel.

@todo
Copy link

todo bot commented Jul 21, 2021

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

// 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 45b23ba in #8542. cc @kpschoedel.

@todo
Copy link

todo bot commented Jul 21, 2021

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

// 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 45b23ba in #8542. cc @kpschoedel.

@todo
Copy link

todo bot commented Jul 21, 2021

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

// 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 45b23ba in #8542. cc @kpschoedel.

@todo
Copy link

todo bot commented Jul 21, 2021

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

// 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 45b23ba in #8542. cc @kpschoedel.

@todo
Copy link

todo bot commented Jul 21, 2021

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

// 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 45b23ba in #8542. cc @kpschoedel.

…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.
@github-actions
Copy link

Size increase report for "esp32-example-build" from 62083da

File Section File VM
chip-lock-app.elf .flash.text -64 -64
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

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

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

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

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

sections,vmsize,filesize

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

sections,vmsize,filesize

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

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

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

sections,vmsize,filesize
.debug_abbrev,0,7
.debug_info,0,5
.debug_loc,0,4
.riscv.attributes,0,2
.debug_line,0,-2

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

sections,vmsize,filesize


@github-actions
Copy link

Size increase report for "nrfconnect-example-build" from 62083da

File Section File VM
chip-shell.elf text 16 16
chip-lock.elf text 16 16
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
text,16,16
.debug_str,0,10
.symtab,0,-16
.debug_aranges,0,-48
.strtab,0,-52
.debug_frame,0,-68
.debug_loc,0,-310
.debug_abbrev,0,-1925
.debug_line,0,-3933
.debug_info,0,-12262

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

sections,vmsize,filesize
text,16,16
.debug_str,0,10
.symtab,0,-16
.debug_aranges,0,-48
.strtab,0,-52
.debug_frame,0,-68
.debug_loc,0,-314
.debug_abbrev,0,-1947
.debug_line,0,-5585
.debug_info,0,-10960


@kpschoedel
Copy link
Contributor Author

Withdrawing this because an actual fix is imminent.

@kpschoedel kpschoedel closed this Jul 21, 2021
@kpschoedel kpschoedel deleted the x7725-revert-8456 branch July 30, 2021 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants