Chime server #35892
Open
Chime server #35892
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review changes with SemanticDiff. Analyzed 3 of 13 files.
|
|
PR #35892: Size comparison from c058d60 to 3bf7248 Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #35892: Size comparison from cc6c773 to 6137a59 Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #35892: Size comparison from 9ae53c5 to 2e48f00 Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andyg-apple
force-pushed
the
ChimeServer
branch
from
2e48f00
to
0c69f62
Compare
|
PR #35892: Size comparison from 965a377 to 4ba7a35 Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Comment on lines
+1
to
+3
|
|
||
| /* | ||
| * |
There was a problem hiding this comment.
Suggested change
| /* | |
| * | |
| /* |
| // runs through all the Chime sounds available one by one | ||
| do | ||
| { | ||
| Chime::Structs::ChimeSoundStruct::Type chimeSound; |
There was a problem hiding this comment.
Suggested change
| Chime::Structs::ChimeSoundStruct::Type chimeSound; | |
| Structs::ChimeSoundStruct::Type chimeSound; |
| do | ||
| { | ||
| Chime::Structs::ChimeSoundStruct::Type chimeSound; | ||
| if ((err = this->mDelegate.GetChimeSoundByIndex(index, chimeSound)) == CHIP_NO_ERROR) |
There was a problem hiding this comment.
So what's the ownership of the "Name" field of that struct? This code assumes that it's pointing into some callee-allocated buffer that the callee can then free ... when?
It would be better to have a struct inheriting from ChimeSoundStruct::Type that has storage for the name so the callee can just copy into that. See what the mode base bits do for an example, or some of the thermostat bits.
| }); | ||
|
|
||
| case ActiveChimeID::Id: | ||
| return aEncoder.Encode(mDelegate.GetActiveChimeId()); |
There was a problem hiding this comment.
So if something that's not en explicit write changes the active chime id or the enabled state, the delegate is responsible for dealing with the dirty marking? See also #35729 (comment)
| } | ||
| } | ||
|
|
||
| CHIP_ERROR ChimeServer::SetActiveChimeId(uint8_t soundId) |
There was a problem hiding this comment.
So this is a private method... who calls it?
| class ChimeServer : private AttributeAccessInterface, private CommandHandlerInterface | ||
| { | ||
| public: | ||
| ChimeServer(EndpointId endpointId, ChimeDelegate & delegate); |
There was a problem hiding this comment.
So this assumes the delegate exists before this is constructed, right?
That means this will be destroyed before the delegate, and our destructor should presumably null out the pointer to us on the delegate...
| * CHIP_ERROR_NOT_FOUND if the index in beyond the list of available chime sounds. | ||
| */ | ||
| virtual CHIP_ERROR GetChimeSoundByIndex(uint8_t index, Chime::Structs::ChimeSoundStruct::Type & chimeSound) = 0; | ||
| virtual uint8_t GetActiveChimeId() = 0; |
There was a problem hiding this comment.
Suggested change
| virtual uint8_t GetActiveChimeId() = 0; | |
| virtual uint8_t GetActiveChimeID() = 0; |
would be better to match the spec? Same for set.
| * It should report Status::Success if successful and may | ||
| * return other Status codes if it fails | ||
| */ | ||
| virtual Protocols::InteractionModel::Status playChimeSound() = 0; |
There was a problem hiding this comment.
Suggested change
| virtual Protocols::InteractionModel::Status playChimeSound() = 0; | |
| virtual Protocols::InteractionModel::Status PlayChimeSound() = 0; |
| @@ -193,6 +193,7 @@ | |||
| "MaxPathsPerInvoke" | |||
| ], | |||
| "Bridged Device Basic Information": ["ProductAppearance"], | |||
| "Chime": ["InstalledChimeSounds"], | |||
There was a problem hiding this comment.
Not needed; lists are automatically handled.
On the other hand, you probably do want to add the two non-list attributes handled via AttributeAccessInterface here.
| @@ -187,6 +187,7 @@ | |||
| "MaxPathsPerInvoke" | |||
| ], | |||
| "Bridged Device Basic Information": ["ProductAppearance"], | |||
| "Chime": ["InstalledChimeSounds"], | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Server Code for Chime cluster and associated file changes