Add support for diagnostic logs server cluster and add the log provid… #29976
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
nivi-apple
force-pushed
the
diagnostic_logs_implementation
branch
from
bb90d85 to
d18f8bc
Compare
|
PR #29976: Size comparison from 3aac08f to d18f8bc Increases above 0.2%:
Increases (14 builds for bl602, bl702, bl702l, cc13x4_26x4, mbed)
Decreases (7 builds for bl702, bl702l)
Full report (16 builds for bl602, bl702, bl702l, cc13x4_26x4, mbed)
|
nivi-apple
force-pushed
the
diagnostic_logs_implementation
branch
2 times, most recently
from
3a1ba37 to
6faf371
Compare
…er delegate to all-clusters-app - Add darwin code to download logs of differnt types and get the logs transferred via BDX
nivi-apple
force-pushed
the
diagnostic_logs_implementation
branch
from
6faf371 to
aa2ade5
Compare
|
@nivi-apple - for summary |
|
Need to add the log files to darwin CI and add the timeout test case in MTRDevice. |
nivi-apple
force-pushed
the
diagnostic_logs_implementation
branch
from
a102200 to
b9fbb0b
Compare
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple and
chrisdecenzo
nivi-apple
commented
Nov 14, 2023
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| #include "diagnostic-logs-provider-delegate-impl.h" |
There was a problem hiding this comment.
need to fix this include.
bzbarsky-apple
requested changes
Nov 14, 2023
examples/all-clusters-app/linux/diagnostic-logs-provider-delegate-impl.cpp
Show resolved
Hide resolved
examples/all-clusters-app/linux/diagnostic-logs-provider-delegate-impl.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Boris Zbarsky <[email protected]>
nivi-apple
force-pushed
the
diagnostic_logs_implementation
branch
from
c84d36c to
1078039
Compare
|
PR #29976: Size comparison from c6b6fb3 to 1078039 Increases above 0.2%:
Increases (46 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (4 builds for psoc6)
Full report (59 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, mbed, nrfconnect, psoc6, qpg, telink)
|
|
PR #29976: Size comparison from c6b6fb3 to 23b91a5 Increases above 0.2%:
Increases (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (5 builds for linux, psoc6)
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
nivi-apple
force-pushed
the
diagnostic_logs_implementation
branch
from
23b91a5 to
8cb17b6
Compare
nivi-apple
force-pushed
the
diagnostic_logs_implementation
branch
from
8cb17b6 to
3151b6c
Compare
|
PR #29976: Size comparison from b5b5b27 to 3151b6c Increases above 0.2%:
Increases (52 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (6 builds for linux, psoc6)
Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
PR #29976: Size comparison from f6e2cfc to e5340b4 Increases above 0.2%:
Increases (54 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (5 builds for linux, psoc6)
Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
bzbarsky-apple
requested changes
Dec 1, 2023
Comment on lines
41
to
43
| chip::Optional<std::string> mEndUserSupportLogFilePath; | ||
| chip::Optional<std::string> mNetworkDiagnosticsLogFilePath; | ||
| chip::Optional<std::string> mCrashLogFilePath; |
There was a problem hiding this comment.
Why are these named "m" of they are global variables (and why are they application globals instead of file statics or class members or something)? My previous review said:
I would suggest just making these
std::stringmembers ofAppOptions
and given how you use them, static members should be the way to go... Probably named "sEndUser...." and whatnot.
And why the extra chip:: prefixes?
Also, I suggested using std::optional. Did that not work for some reason?
There was a problem hiding this comment.
prefixed file path names with s, made them file statics. replaced chip::Optional with std::optional.
|
|
||
| bool AppOptions::IsNullOrEmpty(const char * value) | ||
| { | ||
| return (value == nullptr || strlen(value) == 0 || strncmp(value, "", strlen(value)) == 0); |
There was a problem hiding this comment.
I don't understand that third bit with strncmp. When would that ever be true if the previous two checks are false?
There was a problem hiding this comment.
checking for an empty string. it might not be needed actually.
There was a problem hiding this comment.
checking for an empty string
strlen == 0 checked that already.
| } | ||
| else | ||
| { | ||
| mLogSessionHandle = kInvalidLogSessionHandle; |
There was a problem hiding this comment.
OK, so this will return kInvalidLogSessionHandle but leave mIsInALogCollectionSession set to true, right? Why is that OK?
Same for the places above that return kInvalidLogSessionHandle on errors.
There was a problem hiding this comment.
not ok. this is a bug. fixed.
There was a problem hiding this comment.
i moved mIsInALogCollectionSession to the bottom of the function once the file has been successfully opened
Comment on lines
88
to
89
| if ((remainingBytesToBeRead >= kMaxLogContentSize && outBuffer.size() < kMaxLogContentSize) || | ||
| (remainingBytesToBeRead < kMaxLogContentSize && static_cast<long long>(outBuffer.size()) < remainingBytesToBeRead)) |
There was a problem hiding this comment.
I don't get this. The caller is asking you for the next outBuffer.size() bytes. Why would you fail if that's smaller than the "bytes to be read"? What is this check trying to accomplish?
There was a problem hiding this comment.
This is not required here. i fixed the code.
| } | ||
|
|
||
| mFileStream.seekg(static_cast<long long>(mTotalNumberOfBytesConsumed)); | ||
| mFileStream.read(reinterpret_cast<char *>(outBuffer.data()), kMaxLogContentSize); |
There was a problem hiding this comment.
This should be reading the min of remainingBytesToBeRead and outBuffer.size(), no?
|
|
||
| dispatch_queue_t queue = dispatch_get_main_queue(); | ||
|
|
||
| dispatch_async(queue, ^{ |
There was a problem hiding this comment.
Why is this dispatch_async here?
Comment on lines
2696
to
2698
| if (timeout > 0) { | ||
| XCTAssertNotNil(error); | ||
| XCTAssertNil(logResult); |
There was a problem hiding this comment.
Is this because the timeout > 0 case is buggy? Needs comment if so.
Co-authored-by: Boris Zbarsky <[email protected]>
|
PR #29976: Size comparison from f6e2cfc to 5a37be1 Increases above 0.2%:
Increases (49 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (7 builds for efr32, esp32, psoc6)
Full report (58 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, mbed, nrfconnect, psoc6, qpg, telink)
|
nivi-apple
force-pushed
the
diagnostic_logs_implementation
branch
from
ce3351e to
34766b0
Compare
nivi-apple
force-pushed
the
diagnostic_logs_implementation
branch
from
34766b0 to
707a677
Compare
|
PR #29976: Size comparison from 6907eaa to 707a677 Increases above 0.2%:
Increases (46 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (4 builds for psoc6)
Full report (60 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
Replaced by #31198 |
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.
…er delegate to all-clusters-app
Fixes: #29829
This change adds support for the diagnostic logs server cluster and uses BDX to transfer the logs.
Also adds support to all-clusters-app that implements the log provider delegate which provides the logs to transfer to the requestor