Feature Page Phase 2 #1482

ethanzhouyc · 2024-11-13T15:15:08Z

Related ZAP PR: Feature Page phase 1 #1409
Related Issue: Matter Device Type Features page #1380
JIRA: GHM_ZAP-440 and 10 subtasks under it
CHIP PR to add conform data to XML: Revert "Revert Generate Conformance Data with Alchemy" and Remove Attribute Description connectedhomeip#36233

New Features:

1. Enable toggle of device type features

automatically toggle elements (attributes, commands, and events) to correct conformance
update featureMap attribute for the cluster of toggled feature
add warning notification if a mandatory feature is disabled or a provisional one is enabled
disable feature updates and set warnings if conformance is too complex to manage

2. Add conformance warning to elements

display icon with warning messages and notifications for elements with incorrect conformance
sync warnings across locations and ensure compatibility with existing warnings from device type requirements
update warnings after feature and element toggles

3. Other improvements:

show Device Type Feature button and element conform warnings only if conformance data exist, ensuring compatibility with older Matter SDK
disable user from changing featureMap attribute
add unit tests for backend functions
update svg file

Example Use Cases:

1. Enable HS feature in an endpoint with Extended Color Light device type

2. After enabling HS, disabling attribute CurrentHue that conforms to HS will cause conformance warnings

paulr34

Great job @ethanzhouyc !!!!!

src/components/ZclDeviceTypeFeatureManager.vue

brdandu

Will need some more time to go through this because of its size.

src-electron/db/db-mapping.js

brdandu · 2024-11-13T20:00:37Z

src-electron/db/query-command.js

@@ -2046,6 +2048,50 @@ async function selectNonManufacturerSpecificCommandDetailsFromAllEndpointTypesAn
  )
 }

+/**
+ * Get all commands in an endpoint type cluster
+ * Non-required commands are not loaded into ENDPOINT_TYPE_COMMAND table,


Are non-required attributes loaded into endpoint_type_attribute table? I don't see deviceTypeClusterId being used in selectAttributesByEndpointTypeClusterId

Yes, if you create an endpoint and check ENDPOINT_TYPE_ATTRIBUTE table, there will be attributes with INCLUDED = 0, so deviceTypeClusterId is not needed here

brdandu · 2024-11-13T20:33:32Z

src-electron/db/query-config.js

-      mandatoryFeaturesOnEndpointTypeAndCluster.map((f) => f.featureBit)
+    // only set featureMap bit to 1 for mandatory features
+    let featureMapBitsToBeEnabled = featuresOnEndpointTypeAndCluster
+      .filter((f) => f.conformance == 'M')


Why do we filter for just Mandatory? Were we doing something wrong before?

Before, we only loaded mandatory features to the database so there is no need to filter. Now features with all types of conformance exist

brdandu · 2024-11-13T20:35:18Z

src-electron/db/query-feature.js

-        f.BIT,
-        f.DEFAULT_VALUE,
-        f.DESCRIPTION
+			d.DESCRIPTION AS DEVICE_TYPE_NAME,


Fix the formatting here.

The format is in good shape if you view the actual file instead of PR diff. Github is displaying it weirdly

ethanzhouyc · 2024-11-14T19:02:12Z

New changes:

Rebased with master to include changes from PR Restructure getEndpointIds function to pass ui.test.js #1483 and fix failed checks
add extended db-mapping to endpoint type elements

tecimovic

Lost of stuff..... I marked some obvious things, including some typos.

src-electron/db/query-config.js

src-electron/db/query-session-notification.js

src-electron/db/query-config.js

src-electron/db/query-feature.js

tecimovic · 2024-11-20T20:33:59Z

src-electron/db/query-feature.js

+async function getFeaturesByDeviceTypeRefs(
+  db,
+  deviceTypeRefs,
+  endpointTypeRef


I don't understand why "get feature by device type" needs an "endpoint type" all of suddenly?

ENDPOINT_TYPE_ATTRIBUTE and ATTRIBUTE table is joined to get info on:

featureMap value related to the feature, deciding if the feature is enabled

attribute id of featureMap, which is needed when updating the featureMap after toggling a feature

tecimovic · 2024-11-20T20:44:50Z

src-electron/rest/user-data.js

+ * @param {*} db
+ * @returns callback for the express uri registration
+ */
+function httpPostSetRequiredElements(db) {


I'm confused with the name of this function..... Why do you need to POST the required elements? Aren't the required elements calculated on the back-end and at best sent over to the front-end as result of some "POST" that sets feature bits or something like that?

You are right. I changed it to a GET request as it only gets resources from backend and not change them

src-electron/rest/user-data.js

tecimovic · 2024-11-20T20:46:55Z

src-electron/rest/user-data.js

+  return async (request, response) => {
+    let resp = await querySessionNotification.setRequiredElementWarning(
+      db,
+      request.body,


It seems unsafe and suspicious to post the whole request body straight into the query. This feels wrong somehow.

I improved this by only getting relevant variables passed from frontend, creating a new object out of them, and passing the new object to backend

tecimovic · 2024-11-20T20:49:05Z

src-electron/zcl/zcl-loader-silabs.js

  if (operand.mandatoryConform) {
    let insideTerm = operand.mandatoryConform[0]
    // Recurse further if insideTerm is not empty
    if (insideTerm && Object.keys(insideTerm).toString() != '$') {
-      return parseFeatureConformance(operand.mandatoryConform[0])
+      return parseConformanceRecursively(operand.mandatoryConform[0])


I generally like to pass a "depth" to any recursive function. You start with "depth = 0" and then whenever you call recursively, you call it with "depth+1". When depth gets to like 200, you know you're doing something wrong, you blow up with a clear exception, instead of some stupid stack overflow which is hard to track down.

But I'm possibly old school....

That's great advice for keeping recursive functions safe, but this function is already safeguarded against infinite recursion levels, as:

each recursive call moves one layer deeper into the xml data object

the if/else if/else structure ensures that all cases have a return value

You are not guarding against yourself. You are guarding against a future developer, who might add things to this function, that would mistakenly cause this to end up in stack overflow.

Generally, be always aware who you're developing for:
1.) For you, TODAY.
2.) For you, TOMORROW.
3.) For a random guy that has no clue and shows up here TOMORROW.

Not a terribly big deal though, not something to hold this up for.

I added a max depth check on the recursive function as you said so someone in the future would get an error if they blow up

src/components/ZclDeviceTypeFeatureManager.vue

ethanzhouyc · 2024-11-21T15:09:12Z

All typos and SQL related comments are addressed

tecimovic · 2024-11-21T23:25:38Z

src-electron/zcl/zcl-loader-silabs.js

+    // process andTerm and orTerm in the same logic
+    // when joining multiple orTerms inside andTerms, we need to
+    // surround them with '()', vice versa for andTerms inside orTerms
+    // e.g. A & (B | C) or A | (B & C)


You should use antlr for these kid of things in the future.

Yes we could switch to ANTLR if conformance parsing became too complicated to be parsed one day

tecimovic

Merge this in. There are things that are a bit weird, but we shouldn't hold this up.

You can clean up later.

Generally:
1.) Break up these large reworks into multiple PRs. There is nothing, for example, stopping you to do a smaller PR, that just updates the DB schema, then push that in. While we review that, you work on the next PR for middleware, and then up to the UI. Or whatever direction you want, but layer by layer.

2.) Large things like this are nicely developed if you have a toplevel preference option to turn these features on and off. That protects you, for example, in Zigbee case, where you can simply keep this off, and not even enable it, then Zigbee doesn't fall victim to your bugs.

ethanzhouyc · 2024-11-22T01:13:39Z

There will be future cleanup PR to separate feature related code from Zigbee

…nform

ethanzhouyc requested review from paulr34, tecimovic and brdandu as code owners November 13, 2024 15:15

paulr34 approved these changes Nov 13, 2024

View reviewed changes

src/components/ZclDeviceTypeFeatureManager.vue Show resolved Hide resolved

brdandu requested changes Nov 13, 2024

View reviewed changes

ethanzhouyc mentioned this pull request Nov 14, 2024

Restructure getEndpointIds function to pass ui.test.js #1483

Merged

ethanzhouyc force-pushed the feature-page2 branch from cb06253 to 665b0f7 Compare November 15, 2024 16:48

tecimovic requested changes Nov 20, 2024

View reviewed changes

tecimovic reviewed Nov 21, 2024

View reviewed changes

tecimovic approved these changes Nov 21, 2024

View reviewed changes

ethanzhouyc added 16 commits November 22, 2024 09:53

phase 2: add attribute conform

41e3462

fix parser bug

970a968

add command conform

dbed05c

change parser to universal

79c9a36

fix FPG typo in xml

89cc588

add toggle button simple

5fe0715

remove debug code

a4e8f0b

fix bug when create/edit endpoint

5f9469e

enable toggle, update featureMap bit

c060de7

fix undefined deviceTypeRef bug

05883b4

finalize update featureMap

32d1e8c

demo version

b28db2f

remove unused function

37fba6b

add unit test for all backend function except xml parser function

52ad67a

safety net, update test, disable change when elements have complex co…

271b6fe

…nform

disable featureMap defaultValue change

4d0cc53

ethanzhouyc added 21 commits November 22, 2024 09:53

support toggle events

56e9d7a

minor fix

dd3f596

fix parser bug after Matter xml update

3fcf817

only show desc if relevant

c2af98b

clean

6fbaa96

only show button if dt feature exists

7c8a4c7

add element warning

f46a948

multiple warnings, only add when feature exists

6c72039

fix bug in element warning

62dbc4b

test version, need cleanup

4eb86e1

small fix

91782c1

clean up, add info to warning

0f93494

only show button if conform data exist

449d2d8

enable featureMap change and show warning

7ce859d

create extended dbmapping

87da536

update svg

b02839c

fix typo and remove .js

80202ea

improve sql format and uppercase alias

b3fc769

fix restapi

836fd2a

add parallel promises

1da11bf

add max depth to recursion

c92cdc6

paulr34 force-pushed the feature-page2 branch from 7984454 to c92cdc6 Compare November 22, 2024 14:53

paulr34 merged commit c72231e into project-chip:master Nov 22, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature Page Phase 2 #1482

Feature Page Phase 2 #1482

ethanzhouyc commented Nov 13, 2024 •

edited

Loading

paulr34 left a comment

brdandu left a comment

brdandu Nov 13, 2024

ethanzhouyc Nov 13, 2024 •

edited

Loading

brdandu Nov 13, 2024

ethanzhouyc Nov 13, 2024 •

edited

Loading

brdandu Nov 13, 2024

ethanzhouyc Nov 13, 2024 •

edited

Loading

ethanzhouyc commented Nov 14, 2024 •

edited

Loading

tecimovic left a comment

tecimovic Nov 20, 2024

ethanzhouyc Nov 21, 2024

tecimovic Nov 20, 2024

ethanzhouyc Nov 21, 2024

tecimovic Nov 20, 2024

ethanzhouyc Nov 21, 2024

tecimovic Nov 20, 2024

ethanzhouyc Nov 21, 2024

tecimovic Nov 21, 2024

tecimovic Nov 21, 2024

ethanzhouyc Nov 22, 2024

ethanzhouyc commented Nov 21, 2024

tecimovic Nov 21, 2024

ethanzhouyc Nov 22, 2024

tecimovic left a comment

ethanzhouyc commented Nov 22, 2024 •

edited

Loading

Feature Page Phase 2 #1482

Feature Page Phase 2 #1482

Conversation

ethanzhouyc commented Nov 13, 2024 • edited Loading

New Features:

Example Use Cases:

paulr34 left a comment

Choose a reason for hiding this comment

brdandu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanzhouyc Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanzhouyc Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanzhouyc Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

ethanzhouyc commented Nov 14, 2024 • edited Loading

tecimovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanzhouyc commented Nov 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tecimovic left a comment

Choose a reason for hiding this comment

ethanzhouyc commented Nov 22, 2024 • edited Loading

ethanzhouyc commented Nov 13, 2024 •

edited

Loading

ethanzhouyc Nov 13, 2024 •

edited

Loading

ethanzhouyc Nov 13, 2024 •

edited

Loading

ethanzhouyc Nov 13, 2024 •

edited

Loading

ethanzhouyc commented Nov 14, 2024 •

edited

Loading

ethanzhouyc commented Nov 22, 2024 •

edited

Loading