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.

Sign up for GitHub

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

Jump to bottom

[logging] Introduced log modules and categories selection #130

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

[logging] Introduced log modules and categories selection #130

wants to merge 21 commits into from

Conversation

kkasperczyk-no
Copy link
Owner

@kkasperczyk-no kkasperczyk-no commented Dec 19, 2022 •
edited
Loading

Currently it is possible to disable some part of logs based on their level, but not on their origin module (region). After
introducing specific log modules selection it would be convenient to make controlling those modules log categories
possible as well.

Summary of changes:

  • Added definitions for every existing log module that are by default set to 1, but can be ovewritten to 0.
  • Introduced IsModuleCategoryEnabled macro that returns category state (0/1) for specific log module.
  • To avoid creating dozens of new definitions for every module-category combination, IsModuleCategoryEnabled macro
    by default returns corresponding global category value (CHIP_DETAIL/PROGRESS/ERROR/AUTOMATION_LOGGING). If any
    category value was defined for specific module, it is used instead of global one.
  • To make macro magic work properly, it was necessary to change all kLogCategory_* constants case to upper.
  • Added filtering based on the mentioned definitions, so ChipInternalLog and ChipInternalLogByteSpan has definitions
    provided only if corresponding origin module is enabled and the specific category for this module is enabled.
  • Disabled some modules for nrfconnect platform that saved ~10,5k of flash.

bzbarsky-apple and others added 7 commits December 19, 2022 14:17
@bzbarsky-apple
Explicitly list out renames due to acronym casing changes. (project-c…
ca086f2 
…hip#24137)

This will let us avoid generating incorrect backwards-compat shims for
things that get added after our big API update but have a name that
includes acronyms.
@bzbarsky-apple
Update ZAP to tip. (project-chip#24141)
50e7c19 
Adds ability to remove APIs in Darwin codegen.
@bzbarsky-apple
Remove the MfgSpecificPing command from Darwin API. (project-chip#24139)
fb18595 
* Remove the MfgSpecificPing command from Darwin API.

Except for the backwards-compat shims, of course.

The template refactorings are to reduce repetition and to enable
handling for removal and renaming of commands.

* Fix darwin-framework-tool compile.
@bzbarsky-apple
Update ZAP to tip. (project-chip#24160)
07fe190 
Pulls in Darwin old name information.
@wqx6
basic_information: change the name of emberAfBasicInformationClusterS…
e101730 
…erverInitCallback (project-chip#24162)
@bzbarsky-apple
Go back to using the storage key 1.0 used for group fabric info. (pro…
4088a77 
…ject-chip#24163)

project-chip#23409 changed the key under
which we store group fabric info, which breaks reception of group messages if a
node updates from before that change to after that change.

We should just keep using the same key name, especially because sharing a single
storage key for all possible fabric lists (groups and whatever else will want to
store lists of fabric indices) does not necessarily make sense.

Fixes project-chip#24161
@vivien-apple
[darwin] Add back the colored output with additional informations for…
74296b8 
… chip-tool (project-chip#24094)
@kkasperczyk-no kkasperczyk-no changed the title [logging] Introduced log modules definitions [logging] Introduced log modules and categories selection Dec 22, 2022
Damian-Nordic
Damian-Nordic reviewed Dec 22, 2022
View reviewed changes
Copy link
Collaborator

@Damian-Nordic Damian-Nordic left a comment

Choose a reason for hiding this comment

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

Looking good, but note that there are tons of code like:

#if CHIP_DETAIL_LOGGING
...

which will not take advantage of the possibility to override enabled category for a given module. It's fine with me for now, but just saying.

examples/common/pigweed/RpcService.cpp Outdated Show resolved Hide resolved
src/lib/support/logging/Constants.h
@@ -63,6 +63,169 @@ enum LogModule
kLogModule_Max
};

/* Log modules enablers. Those definitions can be overwritten with 0 to disable
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these still needed? IsModuleCategoryEnabled allows one to effectively disable any module, right?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, but you would need to do that by defining all possible log categories for this module 0. Actually I just realized that in the same way we can disable global log categories and nobody uses _CHIP_USE_LOGGING, which is quite weird for me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, but it would simplify the configuration architecture. At least I think these macros should be taken into account in IsModuleCategoryEnabled so that one can easily cut off the code for log generation.

@kkasperczyk-no
Copy link
Owner Author

Looking good, but note that there are tons of code like

Ehh that will affect only case if CHIP_DETAIL_LOGGING is 0, but we want to set category to Progress for some module. But still it's sad, I don't have good idea how to tackle that without modifying dozen of files.

@kkasperczyk-no kkasperczyk-no force-pushed the log_regions branch 3 times, most recently from 6e86e27 to b22a0c5 Compare December 22, 2022 13:42
chirag-silabs and others added 13 commits December 22, 2022 09:11
@chirag-silabs
[Silabs] BLE bringup on the SiWx917 board (project-chip#24108)
0b8cf4f 
* BLE bringup on the CCP board

* Restyle the PR
@bzbarsky-apple
Align naming in Identify cluster XML with the spec. (project-chip#24173)
6d692e7 
* Align naming in Identify cluster XML with the spec.

* Update ZAP files to the new naming.

* Regenerate generated files.
@arkq
Add flake8 checker to chip-build Docker image (project-chip#24171)
323c4f4
@bzbarsky-apple
Parallelize zap_convert_all.py. (project-chip#24181)
20cd80f 
It's 4-5x faster for me this way.

Also sets ZAP_SKIP_REAL_VERSION so that once
project-chip/zap#876 merges we stop messing with the ZAP
repo just to run it.
@bzbarsky-apple
Add Descriptor cluster to ota-provider example app. (project-chip#24152)
eafbaae 
Fixes project-chip#24144
@s07641069
[Telink] Update Telink Docker image (Zephyr update) (project-chip#24131)
443128a 
* [Telink] Update Telink Docker image (Zephyr update)

* [Telink] update ZEPHYR_REVISION
@bzbarsky-apple
Improve network commissioning setup in bridge-app. (project-chip#24087)
f89640e 
* Make it work on Darwin.
* Pay attention to the wifi/thread command-line flags.
* Ensure we always have a sane network commissioning cluster set up
  (falling back to the Ethernet one if nothing else is enabled).

Fixes project-chip#23987
@krypton36
Resolve issue when adding same index with different credential type. (p…
fba45d8 
…roject-chip#24156)
@emargolis
chip-cert tool: Fix OpenSSL Object Reuse and Double-Free (project-chi…
26bd9d6 
…p#24166)

Don't rely on d2i_X509 object reuse and fix double-free

The chip-cert tool is relying on OpenSSL's "object reuse" mode in
d2i_X509. d2i_X509 has a very bizarre type signature:

X509 *d2i_X509(X509 **out, const unsigned char **inp, long len);

The safest way to call this function is to pass NULL into out. The
function then straightforwardly hands you a new X509 on success, or
NULL on error. However, if out and *out are both NULL, OpenSSL tries
to reuse the existing X509 object.

This does not work, particular not in the way that chip-cert uses it.
When d2i_X509 fails, even in this mode, it will free what's at *out
and set *out to NULL. So when ReadCert's d2i_X509 call fails, it will
silently free the cert parameter. But the caller doesn't know this
and will double-free it!
@Damian-Nordic
[zephyr] Kconfig cleanup (project-chip#24196)
63803aa 
Put Kconfig definitions shared between nRF Connect and
Telink platforms in the common Zephyr's Kconfig.
Group and reorder the config variables for easier search.

Signed-off-by: Damian Krolik <[email protected]>

Signed-off-by: Damian Krolik <[email protected]>
@sharadb-amazon
Fixes in tv-casting-app to allow setting custom Setup Passcodes and b…
19c8ea4 
…ackgrounding the app on iOS (project-chip#24046)

* tv-casting-app: Making getDiscoveredCommissioner API synchronous

* iOS MatterTvCastingBridge: Generate spake2pSalt (and verifier) if required in CommissionableDataProviderImpl

* tv-casting-app/darwin: Stopping/restarting Matter server when app becomes inactive/active. Also, disabling BLE

* Addressing cliffamzn@'s feedback
@kkasperczyk-no
[logging] Introduced log modules and categories selection
e94b8a9 
Currently it is possible to disable some part of logs based on
their level, but not on their origin module (region). After
introducing specific log modules selection it would be
convenient to make controlling those modules log categories
possible as well.

Summary of changes:
* Added definitions for every existing log module that are by
default set to 1, but can be ovewritten to 0.
* Introduced IsModuleCategoryEnabled macro that returns
category state (0/1) for specific log module.
* To avoid creating dozens of new definitions for every
module-category combination, IsModuleCategoryEnabled macro
by default returns corresponding global category value
(CHIP_DETAIL/PROGRESS/ERROR/AUTOMATION_LOGGING). If any
category value was defined for specific module, it is used
instead of global one.
* Added filtering based on the mentioned definitions, so
ChipInternalLog and ChipInternalLogByteSpan has definitions
provided only if corresponding origin module is enabled and the
specific category for this module is enabled.

nRFConnect:
* Disabled some modules for nrfconnect platform that saved ~11,8k
of flash.
* Switched from using kconfig value to filer out logs levels
to relying on Matter logging layer filtering in Zephyr logging
implementation.
@kkasperczyk-no
Fixed logs for Darwin platform
c8403cb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Damian-Nordic Damian-Nordic Damian-Nordic approved these changes

@LuDuda LuDuda Awaiting requested review from LuDuda

@markaj-nordic markaj-nordic Awaiting requested review from markaj-nordic

Assignees
No one assigned
Labels
app config controller darwin examples github integrations lib nrf connect platform scripts support test driver tests tools workflows zephyr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.