Add a SDK part of IM Cluster Status Success to CHIP_ERROR
#34597
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. |
pullapprove
bot
requested review from
andyg-apple,
anush-apple,
arkq,
axelnxp,
bauerschwan,
bzbarsky-apple,
carol-apple,
chapongatien,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
doru91,
fessehaeve and
harimau-qirex
pullapprove
bot
requested review from
mkardous-silabs,
mspang,
nivi-apple,
p0fi,
pidarped,
plauric,
rcasallas-silabs,
ReneJosefsen,
robszewczyk,
saurabhst,
selissia,
sharadb-amazon,
tcarmelveilleux,
tecimovic,
tehampson,
tima-q,
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
|
PR #34597: Size comparison from 37fc757 to 5695fa4 Increases above 0.2%:
Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
| kIMGlobalStatus = 5, ///< Interaction Model global status code. | ||
| kIMClusterStatusFailure = 6, ///< Interaction Model cluster-specific status code (failure) | ||
| kApplication = 7, ///< Application-defined errors; see CHIP_APPLICATION_ERROR | ||
| kIMClusterStatusSuccess = 8, ///< Interaction Model cluster-specific status code (success) |
There was a problem hiding this comment.
We sort of purposefully did not do this, because encoding a "success" status as a CHIP_ERROR is really weird and brittle: various callers have to know that your error is not really an error and that things should be propagated properly.
This is particularly bad when the thing returning this is not in tail-call position, obviously.
For cases when we might have a cluster-specific success we really need to be using a different return type than CHIP_ERROR, or use CHIP_ERROR to indicate actualy errors but have an out-param for the status or something.
There was a problem hiding this comment.
The difficulty was that conceptually Read/Write/Invoke are supposed to be StatusIB (so ClusterStatus) however for chunking we are also expecting a CHIP_ERROR to encode the out of space errors.
I am concerned that the ripple effect of that. The existing code logic is not very clean on single-return because AddStatus is separated from error code return and that allowed our types to diverge.
There was a problem hiding this comment.
One thing we could do:
- Add an
mOutOfSpacemember toAttributeValueEncoder. - Set this member as needed in the relevant public
Encodemethods (we can do this without too much codesize hit, maybe). - Have whatever created the AttributeValueEncoder check that state (like it already checks the
mTriedEncodestate) and handle it appropriately. - Change
AttributeAccessInterface::Readto returnClusterStatus - Change
DataModel::Provider::ReadAttributeto returnClusterStatus
That does mean not being able to use ReturnErrorOnFailure() all over in Read, though....
Another option is to just hang the ClusterStatus off the AttributeValueEncoder and the few consumers that want a success status can do that, and we check for it in the one chokepoint at the callsite.
|
closing for #34708 |
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.
CHIP_ERROR currently covers global IM statuses as well as cluster-specific failures, however we did not cover cluster specific successes.
This PR adds a new part to CHIP_ERROR that encodes cluster statuses and updates StatusIB to be able to convert from them. Note that this still considers such
successstatuses as a CHIP_ERROR since the overhead of converting all tests to not check CHIP_NO_ERROR and have code inside IsSuccess would be quite expensive.Background of this change:
Working on DataModel interface split, where Writes may return a CHIP_ERROR currently however that fails to encode ClusterSuccess.
I also considered making the DM provider interfaces return
ClusterStatushowever that requires more work since ReadAttribute relies on non-status errors like CHIP_NO_MEMORY to control code paths. As such, having a better return (e.g. astd::variant<ClusterStatus, ProcessingError>where ProcessingError would be out of space) requires more thought. Also a lot of SDK code returns CHIP_ERROR, so a CHIP_ERROR based interface seems slightly more convenient to use at this point).