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

Show file tree
Hide file tree
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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})
Expand All @@ -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})
Expand Down
6 changes: 1 addition & 5 deletions src/app/chip_data_model.gni
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,6 @@ template("chip_data_model") {
[
"zap_file",
"is_server",
"external_clusters",
])

if (!defined(sources)) {
Expand Down Expand Up @@ -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",
Expand Down
21 changes: 4 additions & 17 deletions src/app/zap_cluster_list.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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
Expand Down Expand Up @@ -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)

Expand Down
Loading