[controller] Create ControllerInitParams class init params #5747
Conversation
pullapprove
bot
requested review from
andy31415,
bzbarsky-apple,
chrisdecenzo,
Damian-Nordic,
hawk248,
jepenven-silabs and
msandstedt
| System::Layer * systemLayer = nullptr, Inet::InetLayer * inetLayer = nullptr); | ||
| CHIP_ERROR Init(NodeId localDeviceId, ControllerInitParams params); | ||
|
|
||
| [[deprecated("Use ControllerInitParams instead")]] CHIP_ERROR Init(NodeId localDeviceId, |
There was a problem hiding this comment.
Please don't add this without re-enabling -Wdeprecated-declarations:
https://github.com/project-chip/connectedhomeip/blob/master/build/config/compiler/BUILD.gn#L147
(i.e., We should not use our own deprecated interfaces. To do so breaks any project that includes CHIP, enables -Wdeprecated-declarations, and prohibits non-fatal diagnostics using -Werror)
To be honest, I don't think we need to worry about using [[deprecated]] at all prior to 1.0.
There was a problem hiding this comment.
So maybe we can just left this function as is, with a comment to indicate future changes to Init should use ControllerInitParams?
There was a problem hiding this comment.
Sure, that sounds good. My concern is just breaking builds.
| System::Layer * systemLayer = nullptr; | ||
| Inet::InetLayer * inetLayer = nullptr; | ||
|
|
||
| ControllerInitParams() {} |
There was a problem hiding this comment.
I realize this pattern is elsewhere, but I don't understand why we need all this.
What's wrong with
struct ControllerInitParams
{
PersistentStorageDelegate * storageDelegate = nullptr;
System::Layer * systemLayer = nullptr;
Inet::InetLayer * inetLayer = nullptr;
};
?
(maybe I just don't understand the desire to build a whole object as a one-liner. Personally, I think the lack of line breaks is a negative for readability)
There was a problem hiding this comment.
I found even if we dont use the setter, we can still write code in one-liner style.
Since this is just a wrapper of several params (and to avoid merge conflicts), I think the bare struct without getter/setter is ok.
…roject-chip#5747)" This reverts commit 3cb69ff.
…roject-chip#5747)" This reverts commit 3cb69ff.
…hip#5747) * [controller] Create InitParams class * Remove Setter and Deprecated
Problem
The arguments for calling Controller::Init looks not clean.
Summary of Changes
Wrap them in seperare struct.
Further considerations: The SystemLayer, the InetLayer are singleton infact and their life cycle should be the same as the program and we wrap them in classes just for convenience,
However, there do exist some seneros that we don't use full device layer, for example, the Android app.
Anyway, when using a wrapper class, we can reduce the work for adding / removing params in the future.