-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add laundry-washer-controls-cluster server implementation #27503
Conversation
Signed-off-by: Chin-Ran Lo <[email protected]>
src/app/clusters/laundry-washer-controls-server/laundry-washer-controls-delegate.h
Outdated
Show resolved
Hide resolved
src/app/clusters/laundry-washer-controls-server/laundry-washer-controls-delegate.h
Outdated
Show resolved
Hide resolved
src/app/clusters/laundry-washer-controls-server/laundry-washer-controls-delegate.h
Show resolved
Hide resolved
src/app/clusters/laundry-washer-controls-server/laundry-washer-controls-server.h
Outdated
Show resolved
Hide resolved
src/app/clusters/laundry-washer-controls-server/laundry-washer-controls-server.h
Outdated
Show resolved
Hide resolved
src/app/clusters/laundry-washer-controls-server/laundry-washer-controls-server.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/laundry-washer-controls-server/laundry-washer-controls-server.cpp
Show resolved
Hide resolved
src/app/clusters/laundry-washer-controls-server/laundry-washer-controls-server.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/laundry-washer-controls-server/laundry-washer-controls-server.cpp
Outdated
Show resolved
Hide resolved
src/app/clusters/laundry-washer-controls-server/laundry-washer-controls-server.cpp
Outdated
Show resolved
Hide resolved
…-controls-server.cpp Co-authored-by: Boris Zbarsky <[email protected]>
…-controls-server.h Co-authored-by: Boris Zbarsky <[email protected]>
…-controls-server.h Co-authored-by: Boris Zbarsky <[email protected]>
…-controls-server.cpp Co-authored-by: Boris Zbarsky <[email protected]>
…-controls-server.cpp Co-authored-by: Boris Zbarsky <[email protected]>
* Add to calculate the delegate id from endpoint Signed-off-by: Chin-Ran Lo <[email protected]>
src/app/clusters/laundry-washer-controls-server/laundry-washer-controls-delegate.h
Show resolved
Hide resolved
Signed-off-by: Chin-Ran Lo <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, but please do fix the documentation. In particular "list of supported spin_speed list" does not make sense, and similar for rinses.
src/app/clusters/laundry-washer-controls-server/laundry-washer-controls-delegate.h
Outdated
Show resolved
Hide resolved
src/app/clusters/laundry-washer-controls-server/laundry-washer-controls-server.h
Outdated
Show resolved
Hide resolved
src/app/clusters/laundry-washer-controls-server/laundry-washer-controls-server.h
Outdated
Show resolved
Hide resolved
src/app/clusters/laundry-washer-controls-server/laundry-washer-controls-server.h
Outdated
Show resolved
Hide resolved
src/app/clusters/laundry-washer-controls-server/laundry-washer-controls-server.h
Show resolved
Hide resolved
…-controls-server.h Co-authored-by: Boris Zbarsky <[email protected]>
…-controls-server.h Co-authored-by: Boris Zbarsky <[email protected]>
…-controls-server.h Co-authored-by: Boris Zbarsky <[email protected]>
* | ||
* @param endpointId ID of the endpoint | ||
* @param SpinSpeedCurrent attribute SpinSpeedCurrent | ||
* @return true on success, false on failure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commenting post merge, so I do not block the PR: these return EmberAfStatus, so probably not true on success/false on failure. Also the comment seems redundant - when something returns CHIP_ERROR or EmberAfStatus the return is reasonably clear.
Overall we can save vertical space a lot (it is obvious GetSpinSpeedCurrent
gets the speed speed current). I guess it is unclear what newSpinSpeedCurrent
is (what is new
) but the comment does not help me much ... it seems everything is called newSpinSpeedCurrent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestions. I will change these in the next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I add the change in #28264
Please review. Thank you
PR #27503: Size comparison from 0760d13 to 3703648 Increases (6 builds for k32w, nrfconnect, telink)
Decreases (13 builds for bl602, bl702, cc32xx, nrfconnect, psoc6, telink)
Full report (60 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
virtual ~Delegate() = default; | ||
|
||
/** | ||
* Get the list of supported spin_speed list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crlonxp This was merged without addressing review comments (e.g. #27503 (comment)), even though those comments got marked resolved.
I am sorry, but I will no longer be marking your PRs as "approved if you address these comments". All comments will need to be addressed before I can approve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I create another PR to address these remaining comments? Or should I keep modify the code I have and push?
Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I create another PR to address these remaining comments?
Yes, please, since this PR is now merged...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I add the change in #28264
Please review. Thank you
* @param void | ||
* @return CHIP_ERROR CHIP_NO_ERROR on success, or corresponding error code. | ||
*/ | ||
// CHIP_ERROR Init(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this commented-out thing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I registered the LaundryWasherControlServer instance to registerAttributeAccessOverride() before. But it's moved to MatterLaundryWasherControlsPluginServerInitCallback() that it's no specific behavior needed for now.
Should I keep the empty function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestion.
No description provided.