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

[app] Remove external_clusters flag #37817

Closed
wants to merge 1 commit into from
Closed

[app] Remove external_clusters flag #37817

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 3 additions & 10 deletions src/app/chip_data_model.cmake
View file
Original file line number Diff line number Diff line change
@@ -39,14 +39,10 @@ endfunction()
#
# Configure ${APP_TARGET} with source files associated with clusters enabled in the ${ZAP_FILE}
#
function(chip_configure_zap_file APP_TARGET ZAP_FILE EXTERNAL_CLUSTERS)
function(chip_configure_zap_file APP_TARGET ZAP_FILE)
find_package(Python3 REQUIRED)
set(args --zap_file ${ZAP_FILE})

if(EXTERNAL_CLUSTERS)
list(APPEND args --external-clusters ${EXTERNAL_CLUSTERS})
endif()

execute_process(
COMMAND ${Python3_EXECUTABLE} ${CHIP_APP_BASE_DIR}/zap_cluster_list.py ${args}
OUTPUT_VARIABLE CLUSTER_LIST
@@ -74,13 +70,10 @@ endfunction()
# supported by the application.
# IDL .matter IDL file to use for codegen. Inferred from ZAP_FILE
# if not provided
# EXTERNAL_CLUSTERS Clusters with external implementations. The default implementations
# will not be used nor required for these clusters.
# Format: MY_CUSTOM_CLUSTER'.
#
function(chip_configure_data_model APP_TARGET)
set(SCOPE PRIVATE)
cmake_parse_arguments(ARG "" "SCOPE;ZAP_FILE;IDL" "EXTERNAL_CLUSTERS" ${ARGN})
cmake_parse_arguments(ARG "" "SCOPE;ZAP_FILE;IDL" "" ${ARGN})

if(ARG_SCOPE)
set(SCOPE ${ARG_SCOPE})
@@ -104,7 +97,7 @@ function(chip_configure_data_model APP_TARGET)
)

if(ARG_ZAP_FILE)
chip_configure_zap_file(${APP_TARGET} ${ARG_ZAP_FILE} "${ARG_EXTERNAL_CLUSTERS}")
chip_configure_zap_file(${APP_TARGET} ${ARG_ZAP_FILE})

if(NOT ARG_IDL)
string(REPLACE ".zap" ".matter" ARG_IDL ${ARG_ZAP_FILE})
6 changes: 1 addition & 5 deletions src/app/chip_data_model.gni
View file
Original file line number Diff line number Diff line change
@@ -171,7 +171,6 @@ template("chip_data_model") {
[
"zap_file",
"is_server",
"external_clusters",
])

if (!defined(sources)) {
@@ -221,10 +220,7 @@ template("chip_data_model") {
"--zap_file",
_zap_file,
]
if (defined(invoker.external_clusters)) {
_script_args += [ "--external-clusters" ]
_script_args += invoker.external_clusters
}

_cluster_sources = exec_script("${_app_root}/zap_cluster_list.py",
_script_args,
"list lines",
21 changes: 4 additions & 17 deletions src/app/zap_cluster_list.py
View file
Original file line number Diff line number Diff line change
@@ -21,18 +21,14 @@ def get_cluster_sources(clusters: typing.Set[str],
cluster_sources: typing.Set[str] = set()

for cluster in clusters:
if cluster not in source_map:
raise ValueError("Unhandled %s cluster: %s"
" (hint: add to src/app/zap_cluster_list.json)" % (side, cluster))

cluster_sources.update(source_map[cluster])
if cluster in source_map:
Copy link
Contributor

Choose a reason for hiding this comment

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

we seem to lose an exception here and silently ignore a cluster that is not in a source map.

Silently skipping things will be hard to debug.

Copy link
Contributor

Choose a reason for hiding this comment

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

we need a warning at a minimum or a flag that makes it "these are known non-sdk clusters, skip" (which I imagine is what the old flag was supposed to do)

Copy link
Contributor Author

@maciejbaczmanski maciejbaczmanski Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @andy31415 . In my opinion the exception is redundant:

  • if vendor/end user adds a custom cluster, they must modify the .json file, even if they source custom cluster separately (for example they don't want to put the implementation in "${_app_root}/clusters/${cluster}/${cluster}.cpp")
  • if new common cluster is created, but not added to zap_cluster_list.json, build will fail as files are not sourced. I aggree that it would be worth adding a note, but I'm not sure if Build GN can parse python's stderr output in exec_script ?

Alternatively I could convert the external_clusters flag from list to just boolean flag, so that it should be only set if we use external clusters, but without a burden of listing all of them every time?

cluster_sources.update(source_map[cluster])

return cluster_sources


def dump_zapfile_clusters(zap_file_path: pathlib.Path,
implementation_data_path: pathlib.Path,
external_clusters: typing.List[str]):
implementation_data_path: pathlib.Path):
"""Prints all of the source directories to build for a given ZAP file.

Arguments:
@@ -58,8 +54,6 @@ def dump_zapfile_clusters(zap_file_path: pathlib.Path,

for endpoint_type in zap_json.get('endpointTypes'):
for cluster in endpoint_type.get('clusters'):
if cluster.get('define') in external_clusters:
continue
side: str = cluster.get('side')
if side == 'client':
clusters_set = client_clusters
@@ -94,17 +88,10 @@ def main():
required=False,
type=pathlib.Path,
default=os.path.join(os.path.dirname(__file__), "zap_cluster_list.json"))
parser.add_argument('--external-clusters',
help='Clusters with external implementations. ' +
'The default implementations will not be used nor required for these clusters. ' +
'Format: MY_CUSTOM_CLUSTER',
nargs='+',
metavar='EXTERNAL_CLUSTER',
default=[])

args = parser.parse_args()

dump_zapfile_clusters(args.zap_file, args.cluster_implementation_data, args.external_clusters)
dump_zapfile_clusters(args.zap_file, args.cluster_implementation_data)

sys.exit(0)

Loading