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

fix(QPG): Enable -Wundef and fix any errors for qpg based builds #29617

Merged
merged 2 commits into from
Oct 6, 2023
Merged

fix(QPG): Enable -Wundef and fix any errors for qpg based builds #29617

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ac9b5b
fix(QPG): Enable -Wundef and fix any errors for qpg based builds (Add…
tima-q Oct 6, 2023
ecabdb6
Restyled by gn
restyled-commits Oct 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
5 changes: 1 addition & 4 deletions examples/lighting-app/qpg/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,7 @@ qpg_sdk("sdk") {
"${qpg_project_dir}/include",
]

if (chip_enable_pw_rpc) {
defines = [ "PW_RPC_ENABLED" ]
}
defines = [ "PW_RPC_ENABLED=${chip_enable_pw_rpc}" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the semantics of this macro (boolean value vs just-defined-or-not) are per-platform, so it's OK that this is not necessarily going to be consistent with other platforms?

}

qpg_executable("lighting_app") {
Expand Down Expand Up @@ -92,7 +90,6 @@ qpg_executable("lighting_app") {

if (chip_enable_pw_rpc) {
defines += [
"PW_RPC_ENABLED",
"PW_RPC_ATTRIBUTE_SERVICE=1",
"PW_RPC_BUTTON_SERVICE=1",
"PW_RPC_DEVICE_SERVICE=1",
Expand Down
9 changes: 9 additions & 0 deletions examples/lighting-app/qpg/include/CHIPProjectConfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,15 @@
#define CHIP_DEVICE_CONFIG_ENABLE_SED 0
#endif

/**
* @def CHIP_DEVICE_CONFIG_ENABLE_SSED
*
* @brief Defines if a matter device is acting as a Synchronized Sleepy End Device(SSED)
*/
#ifndef CHIP_DEVICE_CONFIG_ENABLE_SSED
#define CHIP_DEVICE_CONFIG_ENABLE_SSED 0
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine, but really this should have gone in CHIPDeviceConfig.h (the core one)... I suspect it will get added there as part of #29613

#endif

/**
* @def CHIP_DEVICE_CONFIG_THREAD_FTD
*
Expand Down
5 changes: 1 addition & 4 deletions examples/lock-app/qpg/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,7 @@ qpg_sdk("sdk") {
"${qpg_project_dir}/include",
]

if (chip_enable_pw_rpc) {
defines = [ "PW_RPC_ENABLED" ]
}
defines = [ "PW_RPC_ENABLED=${chip_enable_pw_rpc}" ]
}

qpg_executable("lock_app") {
Expand Down Expand Up @@ -89,7 +87,6 @@ qpg_executable("lock_app") {

if (chip_enable_pw_rpc) {
defines += [
"PW_RPC_ENABLED",
"PW_RPC_ATTRIBUTE_SERVICE=1",
"PW_RPC_BUTTON_SERVICE=1",
"PW_RPC_DEVICE_SERVICE=1",
Expand Down
14 changes: 14 additions & 0 deletions examples/lock-app/qpg/include/CHIPProjectConfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,20 @@
#define CHIP_DEVICE_CONFIG_ENABLE_SED 1
#endif

/**
* @def CHIP_DEVICE_CONFIG_ENABLE_SSED
*
* @brief Defines if a matter device is acting as a Synchronized Sleepy End Device(SSED)
*/
#ifndef CHIP_DEVICE_CONFIG_ENABLE_SSED
#define CHIP_DEVICE_CONFIG_ENABLE_SSED 0
#endif

/**
* @def CHIP_DEVICE_CONFIG_THREAD_FTD
*
* @brief Defines if a matter device is acting as Full Thread Device (FTD)
*/
#ifndef CHIP_DEVICE_CONFIG_THREAD_FTD
#define CHIP_DEVICE_CONFIG_THREAD_FTD 0
#endif
10 changes: 5 additions & 5 deletions examples/platform/qpg/app/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,11 @@
#include <lib/support/logging/CHIPLogging.h>
#include <platform/CHIPDeviceLayer.h>

#if PW_RPC_ENABLED
#if defined(PW_RPC_ENABLED) && PW_RPC_ENABLED
#include "Rpc.h"
#endif // PW_RPC_ENABLED

#if ENABLE_CHIP_SHELL
#if defined(ENABLE_CHIP_SHELL) && ENABLE_CHIP_SHELL
#include "shell_common/shell.h"
#endif // ENABLE_CHIP_SHELL

Expand Down Expand Up @@ -142,7 +142,7 @@ CHIP_ERROR CHIP_Init(void)
{
CHIP_ERROR ret = CHIP_NO_ERROR;

#if PW_RPC_ENABLED
#if defined(PW_RPC_ENABLED) && PW_RPC_ENABLED
ret = (CHIP_ERROR) chip::rpc::Init();
if (ret != CHIP_NO_ERROR)
{
Expand All @@ -158,7 +158,7 @@ CHIP_ERROR CHIP_Init(void)
goto exit;
}

#if ENABLE_CHIP_SHELL
#if defined(ENABLE_CHIP_SHELL) && ENABLE_CHIP_SHELL
ret = (CHIP_ERROR) ShellTask::Start();
if (ret != CHIP_NO_ERROR)
{
Expand All @@ -184,7 +184,7 @@ CHIP_ERROR CHIP_Init(void)
goto exit;
}

#if CONFIG_CHIP_THREAD_SSED
#if defined(CHIP_DEVICE_CONFIG_ENABLE_SSED) && CHIP_DEVICE_CONFIG_ENABLE_SSED
Copy link
Contributor

Choose a reason for hiding this comment

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

This on the other hand looks wrong. There is no CHIP_DEVICE_CONFIG_ENABLE_SSED so far (before this PR). What we do have is CHIP_DEVICE_CONFIG_THREAD_SSED on some platforms and CONFIG_CHIP_THREAD_SSED on some platforms, and it's a bit of a mess.

But generally:

  1. All CHIP_*CONFIG* macros must have a specific config header responsible for ensuring they are defined (and pulling in all the headers that might define them).
  2. Consumers of such macros should test them with #if and include the responsible header.

The setup here, where defined is tested, makes it too easy to fail to include the header that actually defines the config and then build different parts of the code with incompatible configurations...

I know the config header setup is really weird; I am only now starting to finally understand how it works. But the above is how it should work. ;)

ret = ConnectivityMgr().SetThreadDeviceType(ConnectivityManager::kThreadDeviceType_SynchronizedSleepyEndDevice);
qvIO_EnableSleep(true);
#elif CHIP_DEVICE_CONFIG_ENABLE_SED
Expand Down
4 changes: 4 additions & 0 deletions examples/platform/qpg/project_include/OpenThreadConfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@
#define OPENTHREAD_CONFIG_DHCP6_CLIENT_ENABLE 0
#define OPENTHREAD_CONFIG_DHCP6_SERVER_ENABLE 0
#define OPENTHREAD_CONFIG_TCP_ENABLE 0
#define OPENTHREAD_CONFIG_TLS_ENABLE 0

#define OPENTHREAD_ENABLE_VENDOR_EXTENSION 0
#define OPENTHREAD_EXAMPLES_SIMULATION 0

// Use the Qorvo-supplied default platform configuration for remainder
// of OpenThread config options.
Expand Down
2 changes: 1 addition & 1 deletion src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1710,7 +1710,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread<ImplClass>::DoInit(otInstanc
VerifyOrExit(otErr == OT_ERROR_NONE, err = MapOpenThreadError(otErr));

// Enable automatic assignment of Thread advertised addresses.
#if OPENTHREAD_CONFIG_IP6_SLAAC_ENABLE
#if defined(OPENTHREAD_CONFIG_IP6_SLAAC_ENABLE) && OPENTHREAD_CONFIG_IP6_SLAAC_ENABLE
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, ideally there would be a specific config header responsible for this that we ensured we included. In practice people put this define in all sorts of random places: project-specific OpenThreadConfig.h, platform-specific CHIPDevicePlatformConfig.h, some openthread-core-cc13x2_26x2-config.h and bl702-openthread-core-bl-config.h headers, etc, etc. I have zero confidence this file is ending up including those headers. For example, lots of places are setting openthread_project_core_config_file but I don't see that being used anywhere.

So as far as I can tell, this change is just covering up a bug: we are not including the things that might define OPENTHREAD_CONFIG_IP6_SLAAC_ENABLE in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow up created - #29624

otIp6SetSlaacEnabled(otInst, true);
#endif

Expand Down
4 changes: 0 additions & 4 deletions src/platform/qpg/Logging.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,6 @@
#include <openthread/platform/memory.h>
#endif // CHIP_DEVICE_CONFIG_ENABLE_THREAD

#ifdef PW_RPC_ENABLED
#include "PigweedLogger.h"
#endif

constexpr uint8_t kPrintfModuleLwip = 0x01;
constexpr uint8_t kPrintfModuleOpenThread = 0x02;
constexpr uint8_t kPrintfModuleLogging = 0x03;
Expand Down
5 changes: 4 additions & 1 deletion third_party/qpg_sdk/qpg_sdk.gni
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,10 @@ template("qpg_sdk") {
defines += [ "MBEDTLS_SW_ONLY" ]
}

cflags = []
#Wundef fix - to be updated in SDK
defines += [ "GP_SCHED_NR_OF_IDLE_CALLBACKS=0" ]

cflags = [ "-Wundef" ]

# Allow warning due to mbedtls
cflags += [
Expand Down
Loading