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

controller shouldn't start the Platform Manager event loop #6544

Closed
msandstedt opened this issue May 6, 2021 · 3 comments
Closed

controller shouldn't start the Platform Manager event loop #6544

msandstedt opened this issue May 6, 2021 · 3 comments

Comments

@msandstedt
Copy link
Contributor

Problem

The existence of this interface creates no value:

CHIP_ERROR DeviceController::ServiceEvents()
{
    VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE);

#if CONFIG_DEVICE_LAYER
    ReturnErrorOnFailure(DeviceLayer::PlatformMgr().StartEventLoopTask());
#endif // CONFIG_DEVICE_LAYER

    return CHIP_NO_ERROR;
}

Externally, there's no indication here that calling into this DeviceController instance will actually start a thread in the Platform Manager singleton. Furthermore, calling twice leads to undefined behavior in at least the Posix platform, due to a double-call to pthread_create.

It's just as simple to call DeviceLayer::PlatformMgr().StartEventLoopTask() directly, and with such a call, it is very obvious that an event loop is being started in the Platform Manager singleton.

Proposed Solution

  • remove CHIP_ERROR DeviceController::ServiceEvents()
  • replace all calls with DeviceLayer::PlatformMgr().StartEventLoopTask()
  • bonus points: add locking and flags to posix PlatformMgr so it can't double-call pthread_create
@msandstedt
Copy link
Contributor Author

msandstedt commented May 27, 2021

Along these same lines, the Controller shouldn't be calling Shutdown or destructors for SystemLayer and InetLayer. These are either passed into the controller, or are Device Layer singletons. In either case, the Controller does not own them. This is similar to the complaint in #6544.

@msandstedt
Copy link
Contributor Author

This seems somewhat related to #7429 #7430.

General observations about DeviceController and how it interacts with the stack:

  • Why is the PlatformMgr singleton manipulated in DeviceController in the first place? For instance, in posix platform:
    • DeviceController::Init calls PlatformMgr().Init(). This reinitializes the global mutex.
    • DeviceController::Shutdown calls PlatformMgr().Shutdown(). This pthread-joins the event loop thread.
    • DeviceController::ServiceEvents wraps PlatformMgr().StartEventLoopTask(). This is purely obfuscation.
  • For #ifndef CONFIG_DEVICE_LAYER, DeviceController doesn't construct mInetLayer and mSystemLayer, but it does delete them. Why the asymmetry?
  • For #ifdef CONFIG_DEVICE_LAYER, DeviceController::Init manipulates the DeviceLayer::InetLayer singleton. If DeviceController must always own an InetLayer, why doesn't it just have its own as a private member?
  • Why is the existence of the PlatformMgr singleton apparently dependent on CONFIG_DEVICE_LAYER? What does CONFIG_DEVICE_LAYER mean?
  • more...

DeviceController is being treated somewhat as a singleton, but it is not. Public methods on this object, particularly Init and Shutdown, heavily modify global state. The DeviceController object also appears to assume ownership and control of many resources that could presumably just be private members. If it needs all of these layers for its basic function, why doesn't it have its own?

@msandstedt
Copy link
Contributor Author

Solved with the controller factory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant