Skip to content
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 support for diagnostic logs server cluster and add the log provid… #29976

Closed
Closed
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
aa2ade5
Add support for diagnostic logs server cluster and add the log provid…
nivi-apple Oct 25, 2023
86e01c7
Clean up code
nivi-apple Oct 31, 2023
38930dc
Merge branch 'master' into diagnostic_logs_implementation
nivi-apple Oct 31, 2023
0349632
Restyled by whitespace
restyled-commits Oct 31, 2023
1ce1262
Restyled by clang-format
restyled-commits Oct 31, 2023
a5ec507
Merge branch 'master' into diagnostic_logs_implementation
nivi-apple Oct 31, 2023
426d697
Revert changes to BDXTransferSession
nivi-apple Nov 1, 2023
69f3dd9
Fix the MTRDeviceTests. generate a log file and update darwin.yaml
nivi-apple Nov 2, 2023
bc2cbc0
regenerate zap files
nivi-apple Nov 2, 2023
dcadb91
Revert incorrect changes to MTRCommandPayloadsObjc.h
nivi-apple Nov 2, 2023
4a67f96
Revert TransferFacilitator changes to PollOutput
nivi-apple Nov 2, 2023
76aa772
Test clean up
nivi-apple Nov 2, 2023
0028414
Clean up AppOptions
nivi-apple Nov 2, 2023
52ec06e
Fix the path for all-clusters-app for CI
nivi-apple Nov 2, 2023
baca50e
Fix clang tidy issue
nivi-apple Nov 3, 2023
3776d12
Merge branch 'master' into diagnostic_logs_implementation
nivi-apple Nov 3, 2023
b5e2f4e
Restyled by clang-format
restyled-commits Nov 3, 2023
7294b5a
Call reset before deleting the MTRDiagnosticLogsTransferHandler object
nivi-apple Nov 5, 2023
e0fc837
Fix MTRDevice tests for diagnostic logs
nivi-apple Nov 7, 2023
7fadbd3
Merge branch 'master' into diagnostic_logs_implementation
nivi-apple Nov 7, 2023
7cf9d6d
Remove the boolean that tracks if we are in a BDX session
nivi-apple Nov 7, 2023
2aaaf14
Add MTR_NEWLY_AVAILABLE to the new interfaces added for downloading d…
nivi-apple Nov 7, 2023
502b476
Fix the crash seen when cleaning up MTRDiagnosticsLogHandler
nivi-apple Nov 9, 2023
a9ff69c
Merge branch 'master' into diagnostic_logs_implementation
nivi-apple Nov 9, 2023
56b73e2
Remove commented lines of code
nivi-apple Nov 9, 2023
c259633
Move the diagnostic logs delegate code from all clusters app common code
nivi-apple Nov 14, 2023
646679e
Fix the include path for the diagnostics log provider delegate header
nivi-apple Nov 14, 2023
56a9cfa
Apply suggestions from code review
nivi-apple Nov 14, 2023
a9e6dc6
Merge branch 'master' into diagnostic_logs_implementation
nivi-apple Nov 17, 2023
1078039
Address review comments
nivi-apple Nov 17, 2023
3151b6c
fix CI errors
nivi-apple Nov 18, 2023
f3494cf
Merge branch 'master' into diagnostic_logs_implementation
nivi-apple Nov 30, 2023
9cc3f56
Add comments to ResetTransfer
nivi-apple Nov 30, 2023
e5340b4
Restyled by whitespace
restyled-commits Nov 30, 2023
5a37be1
Apply suggestions from code review
nivi-apple Dec 2, 2023
707a677
Addressed all comments except the Darwin threading model comments
nivi-apple Dec 6, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/darwin.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ jobs:
# target versions instead?
run: |
mkdir -p /tmp/darwin/framework-tests
../../../out/debug/chip-all-clusters-app --interface-id -1 > >(tee /tmp/darwin/framework-tests/all-cluster-app.log) 2> >(tee /tmp/darwin/framework-tests/all-cluster-app-err.log >&2) &
../../../out/debug/chip-all-clusters-app --interface-id -1 --end_user_support_log /tmp/endusersupportlog.txt --network_diagnostics_log /tmp/networkdiagnosticslog.txt --crash_log /tmp/crashlog.txt > >(tee /tmp/darwin/framework-tests/all-cluster-app.log) 2> >(tee /tmp/darwin/framework-tests/all-cluster-app-err.log >&2) &
../../../out/debug/chip-all-clusters-app --interface-id -1 --dac_provider ../../../credentials/development/commissioner_dut/struct_cd_origin_pid_vid_correct/test_case_vector.json --product-id 32768 --discriminator 3839 --secured-device-port 5539 --KVS /tmp/chip-all-clusters-app-kvs2 > >(tee /tmp/darwin/framework-tests/all-cluster-app-origin-vid.log) 2> >(tee /tmp/darwin/framework-tests/all-cluster-app-origin-vid-err.log >&2) &
# Disable BLE because the app does not have the permission to use
# it and that may crash the CI.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6195,10 +6195,15 @@ endpoint 0 {
}

server cluster DiagnosticLogs {
callback attribute generatedCommandList;
callback attribute acceptedCommandList;
callback attribute eventList;
callback attribute attributeList;
ram attribute featureMap default = 0;
ram attribute clusterRevision default = 1;

handle command RetrieveLogsRequest;
handle command RetrieveLogsResponse;
}

server cluster GeneralDiagnostics {
Expand Down
72 changes: 72 additions & 0 deletions examples/all-clusters-app/all-clusters-common/all-clusters-app.zap
Original file line number Diff line number Diff line change
Expand Up @@ -2675,9 +2675,81 @@
"source": "client",
"isIncoming": 1,
"isEnabled": 1
},
{
"name": "RetrieveLogsResponse",
"code": 1,
"mfgCode": null,
"source": "server",
"isIncoming": 0,
"isEnabled": 1
}
],
"attributes": [
{
"name": "GeneratedCommandList",
"code": 65528,
"mfgCode": null,
"side": "server",
"type": "array",
"included": 1,
"storageOption": "External",
"singleton": 0,
"bounded": 0,
"defaultValue": "",
"reportable": 1,
"minInterval": 1,
"maxInterval": 65534,
"reportableChange": 0
},
{
"name": "AcceptedCommandList",
"code": 65529,
"mfgCode": null,
"side": "server",
"type": "array",
"included": 1,
"storageOption": "External",
"singleton": 0,
"bounded": 0,
"defaultValue": "",
"reportable": 1,
"minInterval": 1,
"maxInterval": 65534,
"reportableChange": 0
},
{
"name": "EventList",
"code": 65530,
"mfgCode": null,
"side": "server",
"type": "array",
"included": 1,
"storageOption": "External",
"singleton": 0,
"bounded": 0,
"defaultValue": "",
"reportable": 1,
"minInterval": 1,
"maxInterval": 65534,
"reportableChange": 0
},
{
"name": "AttributeList",
"code": 65531,
"mfgCode": null,
"side": "server",
"type": "array",
"included": 1,
"storageOption": "External",
"singleton": 0,
"bounded": 0,
"defaultValue": "",
"reportable": 1,
"minInterval": 1,
"maxInterval": 65534,
"reportableChange": 0
},
{
"name": "FeatureMap",
"code": 65532,
Expand Down
64 changes: 62 additions & 2 deletions examples/all-clusters-app/linux/AppOptions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,35 @@

#include "AppOptions.h"

#include <app/clusters/diagnostic-logs-server/diagnostic-logs-server.h>
#include <app/server/CommissioningWindowManager.h>
#include <app/server/Server.h>

using namespace chip;
using namespace chip::ArgParser;
using namespace chip::app::Clusters::DiagnosticLogs;

using chip::ArgParser::OptionDef;
using chip::ArgParser::OptionSet;
using chip::ArgParser::PrintArgError;

constexpr uint16_t kOptionDacProviderFilePath = 0xFF01;
constexpr uint16_t kOptionMinCommissioningTimeout = 0xFF02;
constexpr uint16_t kOptionDacProviderFilePath = 0xFF01;
constexpr uint16_t kOptionMinCommissioningTimeout = 0xFF02;
constexpr uint16_t kOptionEndUserSupportFilePath = 0xFF03;
constexpr uint16_t kOptionNetworkDiagnosticsFilePath = 0xFF04;
constexpr uint16_t kOptionCrashFilePath = 0xFF05;

static chip::Credentials::Examples::TestHarnessDACProvider mDacProvider;

chip::Optional<std::string> mEndUserSupportLogFilePath;
chip::Optional<std::string> mNetworkDiagnosticsLogFilePath;
chip::Optional<std::string> mCrashLogFilePath;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::string members of AppOptions

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand that third bit with strncmp. When would that ever be true if the previous two checks are false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checking for an empty string. it might not be needed actually.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checking for an empty string

strlen == 0 checked that already.

}

bool AppOptions::HandleOptions(const char * program, OptionSet * options, int identifier, const char * name, const char * value)
{
bool retval = true;
Expand All @@ -45,6 +60,27 @@ bool AppOptions::HandleOptions(const char * program, OptionSet * options, int id
commissionMgr.OverrideMinCommissioningTimeout(chip::System::Clock::Seconds16(static_cast<uint16_t>(atoi(value))));
break;
}
case kOptionEndUserSupportFilePath: {
if (!IsNullOrEmpty(value))
{
mEndUserSupportLogFilePath.SetValue(std::string{ value });
}
break;
}
case kOptionNetworkDiagnosticsFilePath: {
if (!IsNullOrEmpty(value))
{
mNetworkDiagnosticsLogFilePath.SetValue(std::string{ value });
}
break;
}
case kOptionCrashFilePath: {
if (!IsNullOrEmpty(value))
{
mCrashLogFilePath.SetValue(std::string{ value });
}
break;
}
default:
PrintArgError("%s: INTERNAL ERROR: Unhandled option: %s\n", program, name);
retval = false;
Expand All @@ -59,6 +95,9 @@ OptionSet * AppOptions::GetOptions()
static OptionDef optionsDef[] = {
{ "dac_provider", kArgumentRequired, kOptionDacProviderFilePath },
{ "min_commissioning_timeout", kArgumentRequired, kOptionMinCommissioningTimeout },
{ "end_user_support_log", kArgumentRequired, kOptionEndUserSupportFilePath },
{ "network_diagnostics_log", kArgumentRequired, kOptionNetworkDiagnosticsFilePath },
{ "crash_log", kArgumentRequired, kOptionCrashFilePath },
{},
};

Expand All @@ -68,6 +107,12 @@ OptionSet * AppOptions::GetOptions()
" A json file with data used by the example dac provider to validate device attestation procedure.\n"
" --min_commissioning_timeout <value>\n"
" The minimum time in seconds during which commissioning session establishment is allowed by the Node.\n"
" --end_user_support_log <value>\n"
" The end user support log file to be used for diagnostic logs transfer.\n"
" --network_diagnostics_log <value>\n"
" The network diagnostics log file to be used for diagnostic logs transfer.\n"
" --crash_log <value>\n"
" The crash log file to be used for diagnostic logs transfer\n"
};

return &options;
Expand All @@ -77,3 +122,18 @@ chip::Credentials::DeviceAttestationCredentialsProvider * AppOptions::GetDACProv
{
return &mDacProvider;
}

Optional<std::string> AppOptions::GetEndUserSupportLogFilePath()
{
return mEndUserSupportLogFilePath;
}

Optional<std::string> AppOptions::GetNetworkDiagnosticsLogFilePath()
{
return mNetworkDiagnosticsLogFilePath;
}

Optional<std::string> AppOptions::GetCrashLogFilePath()
{
return mCrashLogFilePath;
}
7 changes: 6 additions & 1 deletion examples/all-clusters-app/linux/AppOptions.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
*
* Copyright (c) 2022 Project CHIP Authors
* Copyright (c) 2022-2023 Project CHIP Authors
* All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
Expand All @@ -27,8 +27,13 @@ class AppOptions
public:
static chip::ArgParser::OptionSet * GetOptions();
static chip::Credentials::DeviceAttestationCredentialsProvider * GetDACProvider();
static chip::Optional<std::string> GetEndUserSupportLogFilePath();
static chip::Optional<std::string> GetNetworkDiagnosticsLogFilePath();
static chip::Optional<std::string> GetCrashLogFilePath();

private:
static bool HandleOptions(const char * program, chip::ArgParser::OptionSet * options, int identifier, const char * name,
const char * value);

static bool IsNullOrEmpty(const char * value);
};
1 change: 1 addition & 0 deletions examples/all-clusters-app/linux/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ source_set("chip-all-clusters-common") {
"${chip_root}/examples/all-clusters-app/all-clusters-common/src/static-supported-modes-manager.cpp",
"${chip_root}/examples/all-clusters-app/all-clusters-common/src/static-supported-temperature-levels.cpp",
"${chip_root}/examples/all-clusters-app/all-clusters-common/src/tcc-mode.cpp",
"${chip_root}/examples/all-clusters-app/linux/diagnostic-logs-provider-delegate-impl.cpp",
"AllClustersCommandDelegate.cpp",
"AppOptions.cpp",
"WindowCoveringManager.cpp",
Expand Down
Loading
Loading