query attributes with edited global attributes #1162

paulr34 · 2023-10-04T18:23:50Z

No description provided.

bzbarsky-apple

This does not seem to fix the output of zcl_attributes....

src-electron/upgrade/upgrade.js

codecov-commenter · 2023-10-09T18:30:11Z

Codecov Report

Merging #1162 (4b83332) into master (a8d5c6d) will increase coverage by 0.02%.
The diff coverage is 91.89%.

❗ Current head 4b83332 differs from pull request most recent head 1f3617c. Consider uploading reports for the commit 1f3617c to get more accurate results

@@            Coverage Diff             @@
##           master    #1162      +/-   ##
==========================================
+ Coverage   65.38%   65.41%   +0.02%     
==========================================
  Files         171      171              
  Lines       18366    18384      +18     
  Branches     3969     3971       +2     
==========================================
+ Hits        12008    12025      +17     
- Misses       6358     6359       +1

Files	Coverage Δ
src-electron/upgrade/upgrade.js	`94.44% <94.11%> (-0.56%)`	⬇️
src-electron/generator/helper-zcl.js	`70.86% <90.00%> (+0.11%)`	⬆️

paulr34 · 2023-10-11T01:51:46Z

@tecimovic @brdandu could you guys please take a look at the Zigbee regen failures when you have a moment? Thanks

src-electron/generator/helper-zcl.js

src-electron/upgrade/upgrade.js

bzbarsky-apple · 2023-10-12T04:59:07Z

src-electron/upgrade/upgrade.js

  let byName = data?.attributeAccessInterfaceAttributes
  let lists = data?.listsUseAttributeAccessInterface
  let forcedExternal = { byName, lists }
  return forcedExternal
 }

+/**
+ * This function takes a clusterId and an array of attributes associated with that clusterID from the database
+ * and changes the global attributes (attributes with clusterId = null) to represent storage policy


Nothing below is restricted to global attributes...

Thanks, I just added a check "if(attribute.clusterId == null)". The "non global attributes" should already be computed from the loading process.

This function should only change global attributes.

bzbarsky-apple · 2023-10-12T16:25:52Z

src-electron/upgrade/upgrade.js

+  try {
+    return JSON.parse(json)
+  } catch (err) {
+    return 'undefined'


Suggested change

return 'undefined'

return undefined

bzbarsky-apple · 2023-10-12T16:26:49Z

src-electron/upgrade/upgrade.js

+ * @returns an array of objects representing attributes in the database
+ */
+
+async function computeStoragePolicy(db, clusterId, attributes) {


Suggested change

async function computeStoragePolicy(db, clusterId, attributes) {

async function computeStoragePolicyForGlobalAttributes(db, clusterId, attributes) {

if that's what it does.

tecimovic · 2023-10-12T19:02:12Z

src-electron/generator/helper-zcl.js

+      this.id,
+      packageIds
+    )
+    attributes = await upgrade.computeStoragePolicyForGlobalAttributes(


This doesn't feel right, layer-wise. The point of upgrade.js is to perform upgrades when you READ IN the .zap files.
You are here using upgrade.js when you're generating stuff.

Not saying that it's the wrong thing, functionally, but I'd then go and move the logic out of upgrade.js, put it somewhere else, and maybe have both places call the common place.

will make follow up PR, thanks

tecimovic · 2023-10-12T19:03:26Z

src-electron/upgrade/upgrade.js

+  try {
+    return JSON.parse(json)
+  } catch (err) {
+    return undefined


? You are hiding exception here. Why do we need this? This feels wrong. I don't want for people to blindly start using this function, and then be confused why they get "error: undefined", instead of "error: JSON parsing error".

So this function makes no sense to me.

thanks will fix

tecimovic · 2023-10-12T19:04:26Z

src-electron/upgrade/upgrade.js

-  let data = JSON.parse(obj)
+  zcl = zcl?.path
+  let obj = await fsp.readFile(zcl, 'utf-8')
+  let data = await parseJson(obj)


I don't see why you wrap JSON parsin into a "fake asynchronous call"... (See my comment above).

src-electron/upgrade/upgrade.js

tecimovic · 2023-10-12T19:11:13Z

src-electron/upgrade/upgrade.js

-  zcl = zcl.path
-  let obj = await fsp.readFile(zcl)
-  let data = JSON.parse(obj)
+  zcl = zcl?.path


Somewhere along the way this part is also eating up an "exception condition" (zcl being null or undefined).

src-electron/upgrade/upgrade.js

paulr34 marked this pull request as ready for review October 5, 2023 17:15

paulr34 requested a review from bzbarsky-apple October 5, 2023 17:26

bzbarsky-apple reviewed Oct 5, 2023

View reviewed changes

src-electron/upgrade/upgrade.js Outdated Show resolved Hide resolved

paulr34 force-pushed the generate_zcl_attributes branch 2 times, most recently from 9d6ea90 to c06ff95 Compare October 11, 2023 00:57

paulr34 requested review from tecimovic and brdandu October 11, 2023 01:51

paulr34 force-pushed the generate_zcl_attributes branch 3 times, most recently from e7b1fab to 6b50b3c Compare October 11, 2023 22:42

bzbarsky-apple reviewed Oct 12, 2023

View reviewed changes

paulr34 force-pushed the generate_zcl_attributes branch 3 times, most recently from 047586c to 182092d Compare October 12, 2023 02:56

bzbarsky-apple reviewed Oct 12, 2023

View reviewed changes

paulr34 changed the title ~~query the attributes with the edited global attributes~~ query attributes with the edited global attributes Oct 12, 2023

paulr34 changed the title ~~query attributes with the edited global attributes~~ query attributes with edited global attributes Oct 12, 2023

bzbarsky-apple approved these changes Oct 12, 2023

View reviewed changes

paulr34 force-pushed the generate_zcl_attributes branch 3 times, most recently from ab8c376 to 935e58a Compare October 12, 2023 17:57

tecimovic reviewed Oct 12, 2023

View reviewed changes

tecimovic requested changes Oct 12, 2023

View reviewed changes

tecimovic reviewed Oct 12, 2023

View reviewed changes

src-electron/upgrade/upgrade.js Show resolved Hide resolved

paulr34 added 2 commits October 12, 2023 15:55

query the attributes with the edited global attributes

ea9aca7

remove unnecessary await

4b83332

paulr34 force-pushed the generate_zcl_attributes branch from 9e7f8b0 to 4b83332 Compare October 12, 2023 19:55

remove async from sync function

1f3617c

paulr34 merged commit 318db74 into project-chip:master Oct 13, 2023
11 of 12 checks passed

paulr34 mentioned this pull request Oct 16, 2023

zcl_attributes does not produce the correct data #1156

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

query attributes with edited global attributes #1162

query attributes with edited global attributes #1162

paulr34 commented Oct 4, 2023

bzbarsky-apple left a comment

codecov-commenter commented Oct 9, 2023 •

edited

Loading

paulr34 commented Oct 11, 2023 •

edited

Loading

bzbarsky-apple Oct 12, 2023

paulr34 Oct 12, 2023

bzbarsky-apple Oct 12, 2023

bzbarsky-apple Oct 12, 2023

tecimovic Oct 12, 2023

paulr34 Oct 12, 2023

tecimovic Oct 12, 2023

paulr34 Oct 12, 2023

tecimovic Oct 12, 2023

tecimovic Oct 12, 2023

	async function computeStoragePolicy(db, clusterId, attributes) {
	async function computeStoragePolicyForGlobalAttributes(db, clusterId, attributes) {

query attributes with edited global attributes #1162

query attributes with edited global attributes #1162

Conversation

paulr34 commented Oct 4, 2023

bzbarsky-apple left a comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 9, 2023 • edited Loading

Codecov Report

paulr34 commented Oct 11, 2023 • edited Loading

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

codecov-commenter commented Oct 9, 2023 •

edited

Loading

paulr34 commented Oct 11, 2023 •

edited

Loading