DiagnosticLogs: Add CHIP_ERROR Parameter to Reset and EndLogCollection Methods #36775
+51
−14
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 |
|
esp seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
PR #36775: Size comparison from 5d42d92 to 6e17037 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
@vivien-apple Please review |
pimpalemahesh
force-pushed
the
modify/bdx-reset
branch
from
6e17037 to
5f8b982
Compare
|
PR #36775: Size comparison from 5d42d92 to 5f8b982 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
pimpalemahesh
force-pushed
the
modify/bdx-reset
branch
from
5f8b982 to
9e7dc2d
Compare
|
PR #36775: Size comparison from 8606290 to 9e7dc2d Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
src/app/clusters/diagnostic-logs-server/DiagnosticLogsProviderDelegate.h
Outdated
Show resolved
Hide resolved
src/app/clusters/diagnostic-logs-server/DiagnosticLogsProviderDelegate.h
Outdated
Show resolved
Hide resolved
pimpalemahesh
force-pushed
the
modify/bdx-reset
branch
from
acb0ac5 to
5ca918e
Compare
|
PR #36775: Size comparison from 2c11741 to 5ca918e Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
shubhamdp
approved these changes
Dec 11, 2024
src/app/clusters/diagnostic-logs-server/BDXDiagnosticLogsProvider.h
Outdated
Show resolved
Hide resolved
|
PR #36775: Size comparison from 2c11741 to 2697dd9 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
approved these changes
Dec 12, 2024
There was a problem hiding this comment.
@pimpalemahesh even though it is explained in the linked issue, please update the summary of the issue to explain WHY these changes are being made and how it makes life better.
The reason for asking this is that these messages make their way in git log, and I would much rather understand the changes from the log than needing to also open an issue to follow up.
cecille
approved these changes
Dec 12, 2024
bzbarsky-apple
requested changes
Dec 12, 2024
| * | ||
| */ | ||
| virtual CHIP_ERROR EndLogCollection(LogSessionHandle sessionHandle) = 0; | ||
| virtual CHIP_ERROR EndLogCollection(LogSessionHandle sessionHandle, CHIP_ERROR error = CHIP_NO_ERROR) = 0; |
There was a problem hiding this comment.
This is still a breaking change. Like I asked: do we want to cause a break here, or do we want to do this in a backwards-compatible manner? That question was not answered.
There was a problem hiding this comment.
@bzbarsky-apple I have overloaded the method in a backward-compatible manner. Please review.
Note: the PR description does not end up in the git log. The commit messages from the actual commits do.... Those should be describing why the change is being made. And they are not. |
…tion overload - Provides a default implementation for the EndLogCollection method with an additional error parameter. - This ensures backward compatibility, reduces the need for overriding in derived classes, and supports scenarios requiring context for log collection termination.
pimpalemahesh
force-pushed
the
modify/bdx-reset
branch
from
2697dd9 to
38bf32b
Compare
|
PR #36775: Size comparison from 9e203e2 to 38bf32b Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
src/app/clusters/diagnostic-logs-server/DiagnosticLogsProviderDelegate.h
Show resolved
Hide resolved
…tion - The one-argument EndLogCollection method is retained for backward compatibility. - The new two-argument overloaded EndLogCollection method will serve as the primary method to be implemented in delegates.
| @@ -65,7 +65,7 @@ CHIP_ERROR LogProvider::GetLogForIntent(IntentEnum intent, MutableByteSpan & out | |||
| err = CollectLog(sessionHandle, outBuffer, unusedOutIsEndOfLog); | |||
| VerifyOrReturnError(CHIP_NO_ERROR == err, err, outBuffer.reduce_size(0)); | |||
|
|
|||
| err = EndLogCollection(sessionHandle); | |||
| err = EndLogCollection(sessionHandle, err); | |||
There was a problem hiding this comment.
So just to be clear: this always passes CHIP_NO_ERROR for the second arg, right?
| err = mDelegate->EndLogCollection(mLogSessionHandle, err); | ||
| if (err == CHIP_ERROR_NOT_IMPLEMENTED) | ||
| { | ||
| ChipLogError(DeviceLayer, "EndLogCollection not implemented in the delegate. Falling back to default behavior."); |
There was a problem hiding this comment.
Which default behavior? We do exactly the same thing no matter what EndLogCollection returns, raising questions about why it returns anything at all.....
| CHIP_ERROR err = mDelegate->EndLogCollection(mLogSessionHandle, error); | ||
| if (err == CHIP_ERROR_NOT_IMPLEMENTED) | ||
| { | ||
| ChipLogError(DeviceLayer, "EndLogCollection not implemented in the delegate. Falling back to default behavior."); |
There was a problem hiding this comment.
Again, this does not seem to describe what we really do, which is ignore the return value entirely.
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.
Fixes #36776
Change Overview: