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

Introduction to Diagnostic Logs provider #11

Merged
merged 3 commits into from
Feb 1, 2024

Conversation

ArekBalysNordic
Copy link
Owner

No description provided.

samples/matter/common/src/Kconfig Outdated Show resolved Hide resolved
samples/matter/common/src/Kconfig Outdated Show resolved Hide resolved
samples/matter/common/src/Kconfig Outdated Show resolved Hide resolved
samples/matter/common/src/Kconfig Outdated Show resolved Hide resolved
@ArekBalysNordic ArekBalysNordic force-pushed the diagnostic_log_first branch 2 times, most recently from 2be0176 to d371fb7 Compare January 25, 2024 14:41
@@ -127,6 +127,10 @@ Matter
In |NCS| Matter samples, the default reaction to migration failure is a factory reset of the device.
To change the default reaction, set the :kconfig:option:`CONFIG_NCS_SAMPLE_MATTER_FACTORY_RESET_ON_KEY_MIGRATION_FAILURE` Kconfig option to ``n``.

* Diagnostic logs provider that collects the diagnostic logs that come from the end user, network stack, or system crashes, and sends them to the Matter controller.
Copy link
Collaborator

Choose a reason for hiding this comment

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

To be honest I'm not sure if we need a docs changes at this stage, because this component is going to evolve and the docs will be likely changed as well. Nevertheless if we really want to do that, it would be good to mention, that network stack or end user logs are not yet supported.

Choose a reason for hiding this comment

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

As long as the network stack and end user parts come before the next release, I'm fine with leaving the entry as is. If there's any chance that one or both might not make it, then I suggest changing the entry to:

Suggested change
* Diagnostic logs provider that collects the diagnostic logs that come from the end user, network stack, or system crashes, and sends them to the Matter controller.
* Diagnostic logs provider that collects the diagnostic logs and sends them to the Matter controller.
The current implementation collects logs from system crashes.

This entry can then be updated when the other parts are added, with the final expected addition changing "current implementation" to "the provider" to remove the implication that the functionality will be further expanded.

@ArekBalysNordic ArekBalysNordic force-pushed the diagnostic_log_first branch 3 times, most recently from b32b77e to 156e9a9 Compare January 29, 2024 11:56
VerifyOrExit(CHIP_NO_ERROR == err, );

exit:
outBuffer.reduce_size(0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we do that in case of success? I think we don't want to set size to 0 in other case than err != CHIP_NO_ERROR.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh, I forgot to push the recent changes

@@ -39,3 +39,19 @@ config NCS_SAMPLE_MATTER_FACTORY_RESET_ON_KEY_MIGRATION_FAILURE
help
Allow device to perform factory reset if the operational key for Fabric has not been migrated
properly to PSA ITS storage.

config NCS_SAMPLE_MATTER_DIAGNOSTIC_LOGS
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's fair to not enable it by default, but I think we should enable it for door lock in its prj.conf/Kconfig, as we are going to test it during TE.

Copy link
Owner Author

Choose a reason for hiding this comment

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

OK, enabling it by default.

Copy link
Collaborator

@kkasperczyk-no kkasperczyk-no Jan 31, 2024

Choose a reason for hiding this comment

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

I mean enabling it for door lock in its prj.conf/Kconfig not for all samples 😄 Nvm, we can merge it as is, because it's only for the test event purposes.

VerifyOrExit(CHIP_NO_ERROR == err, );

err = EndLogCollection(sessionHandle);
VerifyOrExit(CHIP_NO_ERROR == err, );
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe it's redundant, as we go to exit in the next line anyway.

Moved finite map implementation from bridge_util to common util
to make it available for all samples.

Signed-off-by: Arkadiusz Balys <[email protected]>
{
}

/* Not movable */
Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant if this class shall be movable (it it supports move semantics) and if not we can explicitly delete move assign operator and move constructor to be consistent with the "copy" part :) (the comment about copyable was fine)

This commit introduces the diagnostics logs provider and
adds new Kconfigs to manage it.

Signed-off-by: Arkadiusz Balys <[email protected]>
added CONFIG_NCS_SAMPLE_MATTER_DIAGNOSTIC_LOGS=y to lock prj.conf

Signed-off-by: Arkadiusz Balys <[email protected]>
@ArekBalysNordic ArekBalysNordic merged this pull request into nordic_matter_TE_1_3 Feb 1, 2024
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants