From e0a765b31f12fc9fbf54684e72543219b721f211 Mon Sep 17 00:00:00 2001 From: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> Date: Thu, 8 Aug 2024 06:15:26 +1200 Subject: [PATCH] Add chip_logging_backend option and 'none' and 'syslog' backends (#34830) * Tidy up stdio logging target dependencies Also rename from Logging.cpp to Stdio.cpp and add license header. * Add chip_logging_backend option and 'none' backend chip_logging_backend controls which backend is pulled in by the src/platform/logging:default target. The default is 'platform', retaining the current behavior. On Darwin, remove the no-op LoggingImpl and make stdio the default backend when compiling tools or example apps. Use the new 'none' backend when building Matter.framework, retaining current behavior. Depend on logging:default instead of logging:stdio for linux:app-main examples. * Add a syslog logging backend * Fix STM32 build * Use stdio logging backend for cirque tests The stdio and linux backends use slightly different output formats and the cirque tests currently only parse the stdio format correctly. * Apply suggestions from code review Co-authored-by: Boris Zbarsky * Fix stray space in GN string comparison --------- Co-authored-by: Boris Zbarsky --- examples/platform/linux/BUILD.gn | 2 +- scripts/build/gn_gen_cirque.sh | 2 +- .../Framework/chip_xcode_build_connector.sh | 1 + src/lib/core/core.gni | 25 +++++++ src/platform/Darwin/BUILD.gn | 1 - src/platform/logging/BUILD.gn | 39 ++++++++--- .../LoggingImpl.cpp => logging/impl/None.cpp} | 11 ++-- .../impl/{stdio/Logging.cpp => Stdio.cpp} | 17 ++++- src/platform/logging/impl/Syslog.cpp | 65 +++++++++++++++++++ src/platform/stm32/BUILD.gn | 3 +- src/system/SystemConfig.h | 4 +- 11 files changed, 148 insertions(+), 22 deletions(-) rename src/platform/{Darwin/LoggingImpl.cpp => logging/impl/None.cpp} (63%) rename src/platform/logging/impl/{stdio/Logging.cpp => Stdio.cpp} (73%) create mode 100644 src/platform/logging/impl/Syslog.cpp diff --git a/examples/platform/linux/BUILD.gn b/examples/platform/linux/BUILD.gn index 410b1a189245d2..1fcee183f131b3 100644 --- a/examples/platform/linux/BUILD.gn +++ b/examples/platform/linux/BUILD.gn @@ -93,7 +93,7 @@ source_set("app-main") { "${chip_root}/src/controller:controller", "${chip_root}/src/controller:gen_check_chip_controller_headers", "${chip_root}/src/lib", - "${chip_root}/src/platform/logging:stdio", + "${chip_root}/src/platform/logging:default", ] deps = [ ":ota-test-event-trigger", diff --git a/scripts/build/gn_gen_cirque.sh b/scripts/build/gn_gen_cirque.sh index d6f6bd86905a0e..1d89d802e7ad44 100755 --- a/scripts/build/gn_gen_cirque.sh +++ b/scripts/build/gn_gen_cirque.sh @@ -36,7 +36,7 @@ echo "Setup build environment" source "./scripts/activate.sh" echo "Build: GN configure" -gn --root="$CHIP_ROOT" gen --check --fail-on-unused-args out/debug --args='target_os="all"'"chip_build_tests=false chip_enable_wifi=false chip_im_force_fabric_quota_check=true enable_default_builds=false enable_host_gcc_build=true enable_standalone_chip_tool_build=true enable_linux_all_clusters_app_build=true enable_linux_lighting_app_build=true enable_linux_lit_icd_app_build=true" +gn --root="$CHIP_ROOT" gen --check --fail-on-unused-args out/debug --args='target_os="all" chip_logging_backend="stdio" chip_build_tests=false chip_enable_wifi=false chip_im_force_fabric_quota_check=true enable_default_builds=false enable_host_gcc_build=true enable_standalone_chip_tool_build=true enable_linux_all_clusters_app_build=true enable_linux_lighting_app_build=true enable_linux_lit_icd_app_build=true' echo "Build: Ninja build" time ninja -C out/debug all check diff --git a/src/darwin/Framework/chip_xcode_build_connector.sh b/src/darwin/Framework/chip_xcode_build_connector.sh index 33d4a441bf2019..b4a8195b8e0c52 100755 --- a/src/darwin/Framework/chip_xcode_build_connector.sh +++ b/src/darwin/Framework/chip_xcode_build_connector.sh @@ -103,6 +103,7 @@ declare -a args=( 'chip_enable_python_modules=false' 'chip_device_config_enable_dynamic_mrp_config=true' 'chip_log_message_max_size=4096' # might as well allow nice long log messages + 'chip_logging_backend="none"' # os_log() is integrated via CHIP_SYSTEM_CONFIG_PLATFORM_LOG 'chip_disable_platform_kvs=true' 'enable_fuzz_test_targets=false' "target_cpu=\"$target_cpu\"" diff --git a/src/lib/core/core.gni b/src/lib/core/core.gni index ba1d91fd28ca42..f2189198e36131 100644 --- a/src/lib/core/core.gni +++ b/src/lib/core/core.gni @@ -54,7 +54,23 @@ declare_args() { # Configure chip logging to output through external logging implementation. # External code will need to provide implementation for CHIP log output # function (LogV), which is defined in "src/platform/logging/LogV.h". + # Same as setting chip_logging_backend = "external" chip_use_external_logging = false +} + +declare_args() { + # Logging backend to use for targets that don't link a specific log + # backend (e.g. command line utilites usually use 'stdio'). Options: + # 'platform' - The default log backend of the device platform + # 'external' - External LogV implementation (src/platform/logging/LogV.h) + # 'none' - Discard all log output + # 'stdio' - Print to stdout + # 'syslog' - POSIX syslog() + if (chip_use_external_logging) { + chip_logging_backend = "external" + } else { + chip_logging_backend = "platform" + } # Enable short error strings. chip_config_short_error_str = false @@ -117,6 +133,15 @@ if (chip_target_style == "") { } } +assert( + chip_logging_backend == "platform" || chip_logging_backend == "external" || + chip_logging_backend == "none" || chip_logging_backend == "stdio" || + chip_logging_backend == "syslog", + "Please select a valid logging backend: platform, external, none, stdio, syslog") +assert( + !chip_use_external_logging || chip_logging_backend == "external", + "Setting chip_use_external_logging = true conflicts with selected chip_logging_backend") + assert(chip_target_style == "unix" || chip_target_style == "embedded", "Please select a valid target style: unix, embedded") diff --git a/src/platform/Darwin/BUILD.gn b/src/platform/Darwin/BUILD.gn index df4c0774e91627..0a56ce1eac4364 100644 --- a/src/platform/Darwin/BUILD.gn +++ b/src/platform/Darwin/BUILD.gn @@ -151,7 +151,6 @@ static_library("logging") { sources = [ "Logging.h", "Logging.mm", - "LoggingImpl.cpp", ] deps = [ diff --git a/src/platform/logging/BUILD.gn b/src/platform/logging/BUILD.gn index 9b26f40569f428..eb3c62154ce436 100644 --- a/src/platform/logging/BUILD.gn +++ b/src/platform/logging/BUILD.gn @@ -8,8 +8,8 @@ import("${chip_root}/src/lib/core/core.gni") import("${chip_root}/src/lib/shell/shell_device.gni") import("${chip_root}/src/platform/device.gni") -source_set("default") { - if (!chip_use_external_logging) { +group("default") { + if (chip_logging_backend == "platform") { deps = [] if (chip_use_pw_logging) { @@ -63,7 +63,7 @@ source_set("default") { } else if (chip_device_platform == "qpg") { deps += [ "${chip_root}/src/platform/qpg:logging" ] } else if (chip_device_platform == "darwin") { - deps += [ "${chip_root}/src/platform/Darwin:logging" ] + deps += [ ":stdio" ] # For tools / examples. The framework uses "none". } else if (chip_device_platform == "mw320") { deps += [ "${chip_root}/src/platform/nxp/mw320:logging" ] } else if (chip_device_platform == "k32w0" || @@ -82,22 +82,43 @@ source_set("default") { assert(chip_device_platform == "fake" || chip_device_platform == "external" || chip_device_platform == "none") } + } else if (chip_logging_backend == "none" || + chip_logging_backend == "stdio" || + chip_logging_backend == "syslog") { + deps = [ ":${chip_logging_backend}" ] + } else { + assert(chip_logging_backend == "external") } } source_set("headers") { public = [ "LogV.h" ] + public_deps = [ + "${chip_root}/src/lib/support:attributes", + "${chip_root}/src/lib/support:logging_constants", + ] +} + +source_set("none") { + sources = [ "impl/None.cpp" ] + deps = [ + ":headers", + "${chip_root}/src/platform:platform_base", + ] } source_set("stdio") { - sources = [ "impl/stdio/Logging.cpp" ] + sources = [ "impl/Stdio.cpp" ] + deps = [ + ":headers", + "${chip_root}/src/platform:platform_base", + ] +} +source_set("syslog") { + sources = [ "impl/Syslog.cpp" ] deps = [ ":headers", - "${chip_root}/src/lib/core:chip_config_header", - "${chip_root}/src/lib/support:attributes", - "${chip_root}/src/lib/support:logging_constants", - "${chip_root}/src/platform:platform_config_header", - "${chip_root}/src/platform/logging:headers", + "${chip_root}/src/platform:platform_base", ] } diff --git a/src/platform/Darwin/LoggingImpl.cpp b/src/platform/logging/impl/None.cpp similarity index 63% rename from src/platform/Darwin/LoggingImpl.cpp rename to src/platform/logging/impl/None.cpp index 2d6c1b3b744294..10f1b4dc87f54a 100644 --- a/src/platform/Darwin/LoggingImpl.cpp +++ b/src/platform/logging/impl/None.cpp @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2021 Project CHIP Authors + * Copyright (c) 2024 Project CHIP Authors * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -21,12 +21,11 @@ namespace chip { namespace Logging { namespace Platform { -void LogV(const char * module, uint8_t category, const char * msg, va_list v) +void LogV(const char *, uint8_t, const char *, va_list) { - // ChipPlatformLog expands to an os_log call directly (see Logging.h), so - // we don't need to do anything further here. However his function and the - // call to it still exist because of scenarios where a different logging - // backend (usually stdio) is swapped in at link time, e.g. for unit tests. + // This backend discards all log messages. This is useful when all log output + // is routed via `SetLogRedirectCallback()` and/or platform logging + // integration at the log macro level (`CHIP_SYSTEM_CONFIG_PLATFORM_LOG`). } } // namespace Platform diff --git a/src/platform/logging/impl/stdio/Logging.cpp b/src/platform/logging/impl/Stdio.cpp similarity index 73% rename from src/platform/logging/impl/stdio/Logging.cpp rename to src/platform/logging/impl/Stdio.cpp index d36fe2aae2dc64..47338de500b29d 100644 --- a/src/platform/logging/impl/stdio/Logging.cpp +++ b/src/platform/logging/impl/Stdio.cpp @@ -1,4 +1,19 @@ -/* See Project CHIP LICENSE file for licensing information. */ +/* + * + * Copyright (c) 2021-2024 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ #include diff --git a/src/platform/logging/impl/Syslog.cpp b/src/platform/logging/impl/Syslog.cpp new file mode 100644 index 00000000000000..163e6d7398aefe --- /dev/null +++ b/src/platform/logging/impl/Syslog.cpp @@ -0,0 +1,65 @@ +/* + * + * Copyright (c) 2024 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +#include +#include + +#include +#include + +namespace chip { +namespace Logging { +namespace Platform { + +namespace { +int LogPriority(uint8_t category) +{ + switch (category) + { + case kLogCategory_Error: + return LOG_ERR; + case kLogCategory_Progress: + return LOG_NOTICE; + default: + return LOG_DEBUG; + } +} +} // namespace + +void LogV(const char * module, uint8_t category, const char * msg, va_list v) +{ + static std::mutex sMutex; + static bool sInitialized = false; + static char sBuffer[CHIP_CONFIG_LOG_MESSAGE_MAX_SIZE]; + std::lock_guard guard(sMutex); + + if (!sInitialized) + { + openlog(nullptr, 0, LOG_DAEMON); + sInitialized = true; + } + + // Pre-format the message so we can include the module name + vsnprintf(sBuffer, sizeof(sBuffer), msg, v); + syslog(LogPriority(category), "%s: %s", module, sBuffer); +} + +} // namespace Platform +} // namespace Logging +} // namespace chip diff --git a/src/platform/stm32/BUILD.gn b/src/platform/stm32/BUILD.gn index 9b2ed9008c858a..061a2c4c268465 100644 --- a/src/platform/stm32/BUILD.gn +++ b/src/platform/stm32/BUILD.gn @@ -39,7 +39,6 @@ static_library("stm32") { sources = [ "../FreeRTOS/SystemTimeSupport.cpp", "../SingletonConfigurationManager.cpp", - "../logging/impl/stdio/Logging.cpp", "BLEManagerImpl.cpp", "BLEManagerImpl.h", "BlePlatformConfig.h", @@ -69,7 +68,7 @@ static_library("stm32") { "SystemPlatformConfig.h", ] - deps += [ "${chip_root}/src/platform/logging:headers" ] + deps += [ "${chip_root}/src/platform/logging:stdio" ] } public = [ diff --git a/src/system/SystemConfig.h b/src/system/SystemConfig.h index b37cfe555278dd..58339df4be1e47 100644 --- a/src/system/SystemConfig.h +++ b/src/system/SystemConfig.h @@ -558,7 +558,9 @@ struct LwIPEvent; * @def CHIP_SYSTEM_CONFIG_PLATFORM_LOG * * @brief - * Defines whether (1) or not (0) the system uses a platform-specific logging implementation. + * Defines whether (1) or not (0) the system uses a platform-specific implementation of + * ChipLog* macros. Most platforms do not use this option and simply provide a logging + * backend that implements LogV. * * See CHIPLogging.h for details. */