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.

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

Set element conformance notifications on file opening #1513

Merged
merged 4 commits into from
Feb 4, 2025

Conversation

ethanzhouyc
Copy link
Collaborator

@ethanzhouyc ethanzhouyc commented Jan 23, 2025

JIRA: ZAPP-1565

  • Add element conformance warnings to the notification system when opening a ZAP file.
  • Print warnings to the console
  • Relocate several functions to improve code structure, will do a clean up PR later for further improvement

* @param {*} deviceTypeClusterId
* @returns all attributes in an endpoint type cluster
*/
async function selectAttributesByEndpointTypeClusterIdAndDeviceTypeClusterId(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please provide an explanation for moving these functions

Copy link
Collaborator Author

@ethanzhouyc ethanzhouyc Jan 25, 2025

Choose a reason for hiding this comment

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

The getEndpointTypeElements function was moved from user-data.js because it is unrelated to API calls. Since it queries the attribute, command, and event tables, I could not find a perfect location for it, and query-zcl.js is the most suitable location to me. The 3 functions called inside getEndpointTypeElements were moved to align with it.

I could revert this if you think the location does not fit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say before we merge we should figure out and define for good the difference between query-attribute/event/command etc and query-zcl and clean up accordingly.

I think what you're doing is a good thing and I think we should take it further and fully flush it out.

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. Let's discuss in detail tomorrow

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do a cleanup PR that relocates getEndpointTypeElements along with other conformance-related functions to a new folder.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We are extracting attribute specific details here so it does make sense to keep this in this in the query-attribute.js file. Same for events and commands

@ethanzhouyc ethanzhouyc requested a review from dhchandw January 28, 2025 18:40
@paulr34 paulr34 self-requested a review January 28, 2025 18:40
Copy link
Collaborator

@paulr34 paulr34 left a comment

Choose a reason for hiding this comment

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

Approving because we agreed to create a follow up PR to cleanup the files

* @param {*} deviceTypeClusterId
* @returns all attributes in an endpoint type cluster
*/
async function selectAttributesByEndpointTypeClusterIdAndDeviceTypeClusterId(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are extracting attribute specific details here so it does make sense to keep this in this in the query-attribute.js file. Same for events and commands

@@ -134,7 +134,7 @@ function evaluateConformanceExpression(expression, elementMap) {
*/
function evaluateBooleanExpression(expr) {
// Replace terms with their actual values from elementMap
expr = expr.replace(/[A-Za-z][A-Za-z0-9]*/g, (term) => {
expr = expr.replace(/[A-Za-z][A-Za-z0-9_]*/g, (term) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does '_' do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The conformance expression could contain '_', for example: AB_CD & EF. This change will make sure to capture AB_CD as a single term

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok does '_' signify anything in terms of the logical expression here like the &? Didn't see this part of the expression in the spec before so definitely do not understand it. Could you point me to the spec where this is used?

Copy link
Collaborator Author

@ethanzhouyc ethanzhouyc Feb 3, 2025

Choose a reason for hiding this comment

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

The '_' does not function as a logical operator like & (AND) or | (OR). Instead, it is simply a part of certain term name.
For example: PA_LF is the name of a feature from the spec

}
}
return false
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these warnings which are populated when the .zap file is opened? Should these also go into the console so that the CSA folks see these warnings when they generate their sample applications?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, will add to warnings to the console too

* @returns elements object containing all attributes, commands and events
* in an endpoint type cluster
*/
async function getEndpointTypeElements(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this function never used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The function is used, and this PR is not deleting it, but moving the function to a new place, as it is not related to API calls and should not stay in user-data.js. I will create a new PR later to create a new folder and move all conformance related functions to there

Copy link
Collaborator

Choose a reason for hiding this comment

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

Add tests corresponding to your new changes

Copy link
Collaborator Author

@ethanzhouyc ethanzhouyc Feb 3, 2025

Choose a reason for hiding this comment

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

Unit tests for "conformance warnings on import" require adding attribute/command/event conformance to related XMLs.

We could:
1.add these tests after aligning ZAP XMLs with CHIP XMLs.
OR
2.manually updates 1-2 related ZAP XMLs

I prefer adding the tests later

@ethanzhouyc
Copy link
Collaborator Author

Improvements:

  1. Log warnings added to the notification table in the console too.
  2. Revert previously relocated functions back to query-feature.js; a follow-up clean-up PR will reorganize all related functions.

Copy link
Collaborator

@brdandu brdandu left a comment

Choose a reason for hiding this comment

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

Create an issue where this issue is linked and you mentioned the remaining items that need to be done before closing this.
Tests for import and for notifications pane both.

@ethanzhouyc
Copy link
Collaborator Author

Issue #1515 and #1516 created and will be resolved later.

@ethanzhouyc ethanzhouyc merged commit d2e48db into project-chip:master Feb 4, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants