-
Notifications
You must be signed in to change notification settings - Fork 89
Automatic Endpoint Composition Support #1668
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.
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
base: master
Are you sure you want to change the base?
Conversation
…ements functionality. Added new database queries and REST API endpoints to retrieve device type composition requirements. Enhanced loadComposition and addEndpoint functions to automatically create required endpoints based on composition constraints. Updated template to reflect new requirements in endpoint configuration.
Summary of ChangesHello @paulr34, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant feature to ZAP by automating the handling of endpoint composition. Previously, configuring devices that require other device types on separate endpoints (e.g., a refrigerator needing a temperature-controlled cabinet) involved manual setup. This change streamlines the process by automatically creating these required endpoints based on predefined composition rules, thereby simplifying configuration for complex Matter device types and reducing manual effort. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces automatic endpoint composition, a significant feature that simplifies device configuration. The implementation is well-structured, adding new database queries, REST APIs, and Vuex actions to support this functionality. My review focuses on improving the robustness and efficiency of the new logic. I've identified a critical issue related to recursion that could lead to application instability, a database query that can be optimized, and some debug logging that should be cleaned up. Addressing these points will make the feature more reliable and maintainable.
| if (res.data.deviceTypeRef) { | ||
| console.log( | ||
| `[loadComposition] Checking endpoint composition requirements for device type ref: ${res.data.deviceTypeRef}` | ||
| ) | ||
| await createRequiredEndpointsForComposition( | ||
| context, | ||
| res.data.deviceTypeRef, | ||
| rootEndpoint.id, | ||
| rootEndpoint.endpointId | ||
| ) | ||
| } | ||
|
|
||
| return rootEndpoint | ||
| } | ||
|
|
||
| // Handle non-root node compositions | ||
| // TODO: Implement if needed for other composition types | ||
| return null | ||
| } | ||
|
|
||
| /** | ||
| * Creates required endpoints for a device type's endpoint composition requirements. | ||
| * | ||
| * @param {Object} context - The Vuex context object. | ||
| * @param {number} deviceTypeRef - The device type reference ID to check for requirements. | ||
| * @param {number} parentEndpointId - The ID of the parent endpoint (for parent reference). | ||
| * @param {number} parentEndpointIdentifier - The endpoint identifier of the parent. | ||
| * @returns {Promise<Array>} - Array of created endpoints. | ||
| */ | ||
| async function createRequiredEndpointsForComposition( | ||
| context, | ||
| deviceTypeRef, | ||
| parentEndpointId, | ||
| parentEndpointIdentifier | ||
| ) { | ||
| try { | ||
| console.log( | ||
| `[createRequiredEndpointsForComposition] Querying requirements for device type ref: ${deviceTypeRef}` | ||
| ) | ||
| // Query endpoint composition requirements using device type ref | ||
| let requirementsRes = await axiosRequests.$serverGet( | ||
| restApi.uri.endpointCompositionRequirements, | ||
| { | ||
| params: { | ||
| deviceTypeRef: deviceTypeRef | ||
| } | ||
| } | ||
| ) | ||
|
|
||
| console.log( | ||
| `[createRequiredEndpointsForComposition] Requirements response:`, | ||
| requirementsRes.data | ||
| ) | ||
|
|
||
| if (!requirementsRes.data || requirementsRes.data.length === 0) { | ||
| console.log( | ||
| `[createRequiredEndpointsForComposition] No requirements found for device type ref: ${deviceTypeRef}` | ||
| ) | ||
| return [] // No requirements, return empty array | ||
| } | ||
|
|
||
| let createdEndpoints = [] | ||
| let nextEndpointId = parentEndpointIdentifier + 1 | ||
|
|
||
| // Process each requirement | ||
| // Handle constraints - deviceConstraint might indicate minimum count (e.g., "min 1" = 1) | ||
| // For now, create at least one endpoint per requirement | ||
| for (let requirement of requirementsRes.data) { | ||
| if (!requirement.requiredDeviceTypeRef) { | ||
| console.warn( | ||
| `Device type ref not available for requirement: ${requirement.requiredDeviceName}` | ||
| ) | ||
| continue | ||
| } | ||
|
|
||
| // Determine how many endpoints to create based on constraint | ||
| // deviceConstraint might be a number indicating minimum count | ||
| let minCount = 1 | ||
| if (requirement.deviceConstraint != null) { | ||
| // Try to parse constraint - could be a number or string like "min 1" | ||
| if (typeof requirement.deviceConstraint === 'number') { | ||
| minCount = Math.max(1, requirement.deviceConstraint) | ||
| } else if (typeof requirement.deviceConstraint === 'string') { | ||
| // Try to extract number from string like "min 1" | ||
| let match = requirement.deviceConstraint.match(/min\s*(\d+)/i) | ||
| if (match) { | ||
| minCount = parseInt(match[1], 10) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| console.log( | ||
| `[createRequiredEndpointsForComposition] Processing requirement: ${requirement.requiredDeviceName} (ref: ${requirement.requiredDeviceTypeRef}), minCount: ${minCount}` | ||
| ) | ||
|
|
||
| // Create the minimum required endpoints | ||
| for (let i = 0; i < minCount; i++) { | ||
| // Create endpoint type for the required device | ||
| let endpointTypeData = await addEndpointType(context, { | ||
| deviceTypeRef: [requirement.requiredDeviceTypeRef], | ||
| deviceIdentifier: requirement.requiredDeviceCode, | ||
| deviceVersion: 1, // Default version, could be enhanced to get from device type | ||
| name: requirement.requiredDeviceName || `Device ${requirement.requiredDeviceCode}` | ||
| }) | ||
|
|
||
| console.log( | ||
| `[createRequiredEndpointsForComposition] Created endpoint type: ${endpointTypeData.id} for device: ${requirement.requiredDeviceName}` | ||
| ) | ||
|
|
||
| // Create endpoint for the required device type | ||
| let endpoint = await addEndpoint(context, { | ||
| endpointId: nextEndpointId++, | ||
| parentEndpointIdentifier: parentEndpointIdentifier, | ||
| endpointType: endpointTypeData.id, | ||
| profileId: dbEnum.rootNode.profileID // Use same profile as root node | ||
| }) | ||
|
|
||
| console.log( | ||
| `[createRequiredEndpointsForComposition] Created endpoint: ${endpoint.id} (endpointId: ${endpoint.endpointId})` | ||
| ) | ||
|
|
||
| createdEndpoints.push(endpoint) | ||
|
|
||
| // Recursively check if this required device also has composition requirements | ||
| if (requirement.requiredDeviceTypeRef) { | ||
| let nestedEndpoints = | ||
| await createRequiredEndpointsForComposition( | ||
| context, | ||
| requirement.requiredDeviceTypeRef, | ||
| endpoint.id, | ||
| endpoint.endpointId | ||
| ) | ||
| createdEndpoints = createdEndpoints.concat(nestedEndpoints) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return createdEndpoints | ||
| } catch (error) { | ||
| console.error( | ||
| 'Error creating required endpoints for composition:', | ||
| error.message | ||
| ) | ||
| return [] | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Add endpoint in ZAP UI. | ||
| * @param {*} context | ||
| * @param {*} newEndpointContext | ||
| * @returns endpoint data | ||
| */ | ||
| export function addEndpoint(context, newEndpointContext) { | ||
| return axiosRequests | ||
| .$serverPost(restApi.uri.endpoint, newEndpointContext) | ||
| .then((res) => { | ||
| let arg = res.data | ||
| context.commit('addEndpoint', { | ||
| id: arg.id, | ||
| endpointId: arg.endpointId, | ||
| parentEndpointIdentifier: arg.parentEndpointIdentifier, | ||
| endpointTypeRef: arg.endpointType, | ||
| networkId: arg.networkId, | ||
| profileId: arg.profileId, | ||
| endpointIdValidationIssues: arg.validationIssues.endpointId, | ||
| networkIdValidationIssues: arg.validationIssues.networkId | ||
| }) | ||
| return arg | ||
| }) | ||
| export async function addEndpoint(context, newEndpointContext) { | ||
| let res = await axiosRequests.$serverPost(restApi.uri.endpoint, newEndpointContext) | ||
| let arg = res.data | ||
| context.commit('addEndpoint', { | ||
| id: arg.id, | ||
| endpointId: arg.endpointId, | ||
| parentEndpointIdentifier: arg.parentEndpointIdentifier, | ||
| endpointTypeRef: arg.endpointType, | ||
| networkId: arg.networkId, | ||
| profileId: arg.profileId, | ||
| endpointIdValidationIssues: arg.validationIssues.endpointId, | ||
| networkIdValidationIssues: arg.validationIssues.networkId | ||
| }) | ||
|
|
||
| // Check if this endpoint's device types have endpoint composition requirements | ||
| // and automatically create required endpoints | ||
| if (arg.endpointType) { | ||
| console.log( | ||
| `[addEndpoint] Checking endpoint composition requirements for endpoint type: ${arg.endpointType}` | ||
| ) | ||
|
|
||
| // Get device types for this endpoint type | ||
| let deviceTypesRes = await axiosRequests.$serverGet( | ||
| restApi.uri.deviceTypesByEndpointTypeId, | ||
| { | ||
| params: { | ||
| endpointTypeId: arg.endpointType | ||
| } | ||
| } | ||
| ) | ||
|
|
||
| if (deviceTypesRes.data && deviceTypesRes.data.length > 0) { | ||
| // Check each device type for endpoint composition requirements | ||
| for (let deviceType of deviceTypesRes.data) { | ||
| if (deviceType.deviceTypeRef) { | ||
| console.log( | ||
| `[addEndpoint] Checking device type ref: ${deviceType.deviceTypeRef}` | ||
| ) | ||
| await createRequiredEndpointsForComposition( | ||
| context, | ||
| deviceType.deviceTypeRef, | ||
| arg.id, | ||
| arg.endpointId | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return arg | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current recursive implementation for endpoint composition has two critical issues:
-
Redundant Recursion: The
addEndpointaction triggers composition logic. InsidecreateRequiredEndpointsForComposition,addEndpointis called, which handles recursion implicitly. However, there is also an explicit recursive call tocreateRequiredEndpointsForComposition, causing the logic to run twice for each nested requirement. This can lead to duplicated endpoints. -
No Cycle Detection: There is no mechanism to detect circular dependencies (e.g., device A requires B, and B requires A). This could lead to infinite recursion and a stack overflow, crashing the application.
I suggest refactoring loadComposition, addEndpoint, and createRequiredEndpointsForComposition to handle recursion safely and efficiently. This involves removing the redundant recursive call and passing a compositionChain set through the calls to detect cycles.
if (res.data.deviceTypeRef) {
console.log(
`[loadComposition] Checking endpoint composition requirements for device type ref: ${res.data.deviceTypeRef}`
)
await createRequiredEndpointsForComposition(
context,
res.data.deviceTypeRef,
rootEndpoint.id,
rootEndpoint.endpointId,
new Set()
)
}
return rootEndpoint
}
// Handle non-root node compositions
// TODO: Implement if needed for other composition types
return null
}
/**
* Creates required endpoints for a device type's endpoint composition requirements.
*
* @param {Object} context - The Vuex context object.
* @param {number} deviceTypeRef - The device type reference ID to check for requirements.
* @param {number} parentEndpointId - The ID of the parent endpoint (for parent reference).
* @param {number} parentEndpointIdentifier - The endpoint identifier of the parent.
* @param {Set<number>} compositionChain - Used for cycle detection.
* @returns {Promise<Array>} - Array of created endpoints.
*/
async function createRequiredEndpointsForComposition(
context,
deviceTypeRef,
parentEndpointId,
parentEndpointIdentifier,
compositionChain
) {
if (compositionChain.has(deviceTypeRef)) {
console.warn(
`Circular dependency detected for device type ref: ${deviceTypeRef}. Aborting composition.`
)
return []
}
const newCompositionChain = new Set(compositionChain).add(deviceTypeRef)
try {
console.log(
`[createRequiredEndpointsForComposition] Querying requirements for device type ref: ${deviceTypeRef}`
)
// Query endpoint composition requirements using device type ref
let requirementsRes = await axiosRequests.$serverGet(
restApi.uri.endpointCompositionRequirements,
{
params: {
deviceTypeRef: deviceTypeRef
}
}
)
console.log(
`[createRequiredEndpointsForComposition] Requirements response:`,
requirementsRes.data
)
if (!requirementsRes.data || requirementsRes.data.length === 0) {
console.log(
`[createRequiredEndpointsForComposition] No requirements found for device type ref: ${deviceTypeRef}`
)
return [] // No requirements, return empty array
}
let createdEndpoints = []
let nextEndpointId = parentEndpointIdentifier + 1
// Process each requirement
// Handle constraints - deviceConstraint might indicate minimum count (e.g., "min 1" = 1)
// For now, create at least one endpoint per requirement
for (let requirement of requirementsRes.data) {
if (!requirement.requiredDeviceTypeRef) {
console.warn(
`Device type ref not available for requirement: ${requirement.requiredDeviceName}`
)
continue
}
// Determine how many endpoints to create based on constraint
// deviceConstraint might be a number indicating minimum count
let minCount = 1
if (requirement.deviceConstraint != null) {
// Try to parse constraint - could be a number or string like "min 1"
if (typeof requirement.deviceConstraint === 'number') {
minCount = Math.max(1, requirement.deviceConstraint)
} else if (typeof requirement.deviceConstraint === 'string') {
// Try to extract number from string like "min 1"
let match = requirement.deviceConstraint.match(/min\s*(\d+)/i)
if (match) {
minCount = parseInt(match[1], 10)
}
}
}
console.log(
`[createRequiredEndpointsForComposition] Processing requirement: ${requirement.requiredDeviceName} (ref: ${requirement.requiredDeviceTypeRef}), minCount: ${minCount}`
)
// Create the minimum required endpoints
for (let i = 0; i < minCount; i++) {
// Create endpoint type for the required device
let endpointTypeData = await addEndpointType(context, {
deviceTypeRef: [requirement.requiredDeviceTypeRef],
deviceIdentifier: requirement.requiredDeviceCode,
deviceVersion: 1, // Default version, could be enhanced to get from device type
name:
requirement.requiredDeviceName ||
`Device ${requirement.requiredDeviceCode}`
})
console.log(
`[createRequiredEndpointsForComposition] Created endpoint type: ${endpointTypeData.id} for device: ${requirement.requiredDeviceName}`
)
// Create endpoint for the required device type
let endpoint = await addEndpoint(
context,
{
endpointId: nextEndpointId++,
parentEndpointIdentifier: parentEndpointIdentifier,
endpointType: endpointTypeData.id,
profileId: dbEnum.rootNode.profileID // Use same profile as root node
},
newCompositionChain
)
console.log(
`[createRequiredEndpointsForComposition] Created endpoint: ${endpoint.id} (endpointId: ${endpoint.endpointId})`
)
createdEndpoints.push(endpoint)
}
}
return createdEndpoints
} catch (error) {
console.error(
'Error creating required endpoints for composition:',
error.message
)
return []
}
}
/**
* Add endpoint in ZAP UI.
* @param {*} context
* @param {*} newEndpointContext
* @param {Set<number>} compositionChain - Used for cycle detection.
* @returns endpoint data
*/
export async function addEndpoint(
context,
newEndpointContext,
compositionChain = new Set()
) {
let res = await axiosRequests.$serverPost(
restApi.uri.endpoint,
newEndpointContext
)
let arg = res.data
context.commit('addEndpoint', {
id: arg.id,
endpointId: arg.endpointId,
parentEndpointIdentifier: arg.parentEndpointIdentifier,
endpointTypeRef: arg.endpointType,
networkId: arg.networkId,
profileId: arg.profileId,
endpointIdValidationIssues: arg.validationIssues.endpointId,
networkIdValidationIssues: arg.validationIssues.networkId
})
// Check if this endpoint's device types have endpoint composition requirements
// and automatically create required endpoints
if (arg.endpointType) {
console.log(
`[addEndpoint] Checking endpoint composition requirements for endpoint type: ${arg.endpointType}`
)
// Get device types for this endpoint type
let deviceTypesRes = await axiosRequests.$serverGet(
restApi.uri.deviceTypesByEndpointTypeId,
{
params: {
endpointTypeId: arg.endpointType
}
}
)
if (deviceTypesRes.data && deviceTypesRes.data.length > 0) {
// Check each device type for endpoint composition requirements
for (let deviceType of deviceTypesRes.data) {
if (deviceType.deviceTypeRef) {
console.log(
`[addEndpoint] Checking device type ref: ${deviceType.deviceTypeRef}`
)
await createRequiredEndpointsForComposition(
context,
deviceType.deviceTypeRef,
arg.id,
arg.endpointId,
compositionChain
)
}
}
}
}
return arg
}| async function selectEndpointCompositionRequirementsByDeviceTypeRef( | ||
| db, | ||
| deviceTypeRef | ||
| ) { | ||
| // Check if this device type exists in ENDPOINT_COMPOSITION table | ||
| const endpointCompositionQuery = ` | ||
| SELECT ENDPOINT_COMPOSITION_ID | ||
| FROM ENDPOINT_COMPOSITION | ||
| WHERE DEVICE_TYPE_REF = ? | ||
| ` | ||
| const endpointComposition = await dbApi.dbGet( | ||
| db, | ||
| endpointCompositionQuery, | ||
| [deviceTypeRef] | ||
| ) | ||
|
|
||
| if (!endpointComposition) { | ||
| return [] // No endpoint composition found for this device type | ||
| } | ||
|
|
||
| const endpointCompositionId = endpointComposition.ENDPOINT_COMPOSITION_ID | ||
|
|
||
| // Query DEVICE_COMPOSITION table by the foreign key ENDPOINT_COMPOSITION_REF | ||
| const query = ` | ||
| SELECT | ||
| DC.CONFORMANCE, | ||
| DC.DEVICE_CONSTRAINT, | ||
| DC.DEVICE_TYPE_REF as required_device_type_ref, | ||
| DT_REQ.NAME as required_device_name, | ||
| DT_REQ.CODE as required_device_code, | ||
| EC.TYPE as composition_type, | ||
| EC.ENDPOINT_COMPOSITION_ID | ||
| FROM | ||
| DEVICE_COMPOSITION DC | ||
| JOIN | ||
| ENDPOINT_COMPOSITION EC ON DC.ENDPOINT_COMPOSITION_REF = EC.ENDPOINT_COMPOSITION_ID | ||
| LEFT JOIN | ||
| DEVICE_TYPE DT_REQ ON DC.DEVICE_TYPE_REF = DT_REQ.DEVICE_TYPE_ID | ||
| WHERE | ||
| DC.ENDPOINT_COMPOSITION_REF = ? | ||
| ORDER BY | ||
| DC.DEVICE_TYPE_REF | ||
| ` | ||
| const rows = await dbApi.dbAll(db, query, [endpointCompositionId]) | ||
| return rows.map((row) => ({ | ||
| requiredDeviceCode: row.required_device_code, | ||
| requiredDeviceName: row.required_device_name, | ||
| requiredDeviceTypeRef: row.required_device_type_ref, | ||
| conformance: row.CONFORMANCE, | ||
| deviceConstraint: row.DEVICE_CONSTRAINT, | ||
| compositionType: row.composition_type, | ||
| endpointCompositionId: row.ENDPOINT_COMPOSITION_ID | ||
| })) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The selectEndpointCompositionRequirementsByDeviceTypeRef function currently performs two separate database queries: one to get the ENDPOINT_COMPOSITION_ID and another to fetch the composition details. These can be combined into a single, more efficient query. This avoids the overhead of two separate database round-trips and simplifies the code.
async function selectEndpointCompositionRequirementsByDeviceTypeRef(
db,
deviceTypeRef
) {
const query = `
SELECT
DC.CONFORMANCE,
DC.DEVICE_CONSTRAINT,
DC.DEVICE_TYPE_REF as required_device_type_ref,
DT_REQ.NAME as required_device_name,
DT_REQ.CODE as required_device_code,
EC.TYPE as composition_type,
EC.ENDPOINT_COMPOSITION_ID
FROM
DEVICE_COMPOSITION DC
JOIN
ENDPOINT_COMPOSITION EC ON DC.ENDPOINT_COMPOSITION_REF = EC.ENDPOINT_COMPOSITION_ID
LEFT JOIN
DEVICE_TYPE DT_REQ ON DC.DEVICE_TYPE_REF = DT_REQ.DEVICE_TYPE_ID
WHERE
EC.DEVICE_TYPE_REF = ?
ORDER BY
DC.DEVICE_TYPE_REF
`
const rows = await dbApi.dbAll(db, query, [deviceTypeRef])
return rows.map((row) => ({
requiredDeviceCode: row.required_device_code,
requiredDeviceName: row.required_device_name,
requiredDeviceTypeRef: row.required_device_type_ref,
conformance: row.CONFORMANCE,
deviceConstraint: row.DEVICE_CONSTRAINT,
compositionType: row.composition_type,
endpointCompositionId: row.ENDPOINT_COMPOSITION_ID
}))
}
src/store/zap/actions.js
Outdated
| console.log( | ||
| `[createRequiredEndpointsForComposition] Querying requirements for device type ref: ${deviceTypeRef}` | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Remove redundant recursion by adding skipCompositionCheck parameter to addEndpoint - Add cycle detection using compositionChain Set to prevent infinite loops - Remove all debug console.log statements - Maintain backward compatibility with default parameter value
This PR adds automatic endpoint composition handling in ZAP. When a device type requires other device types on separate endpoints (e.g., a Refrigerator requiring a Temperature Controlled Cabinet), the UI creates those endpoints automatically.
Changes
Database query: Added selectEndpointCompositionRequirementsByDeviceTypeRef in query-device-type.js to retrieve composition requirements by joining ENDPOINT_COMPOSITION and DEVICE_COMPOSITION.
REST API: Added /zcl/endpointCompositionRequirements to expose composition requirements to the frontend.
Automatic endpoint creation: Added createRequiredEndpointsForComposition in Vuex actions that:
Checks composition requirements when endpoints are created or loaded
Creates required endpoints with correct device types
Handles minimum count constraints from DEVICE_CONSTRAINT
Recursively processes nested composition requirements
Template helper: Added user_endpoint_composition_requirements for code generation templates to access composition data.
How it works
When a device type with endpoint composition requirements is added (manually or via root node loading), the system:
Queries the database for required device types that must be on separate endpoints
Creates the minimum number of required endpoints based on DEVICE_CONSTRAINT
Recursively checks if the created endpoints also have composition requirements
This simplifies configuration for Matter device types that require multiple endpoints, reducing manual setup.
Testing
Verified with Matter Refrigerator device type, which requires a Temperature Controlled Cabinet on a separate endpoint. The system automatically creates the required endpoint when the refrigerator device type is added.