8000 Fixing the generation for Attribute Min Max used in endpoint_config.h by brdandu · Pull Request #1617 · project-chip/zap · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fixing the generation for Attribute Min Max used in endpoint_config.h #1617

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

Merged
merged 1 commit into from
Jul 3, 2025
Merged

Fixing the generation for Attribute Min Max used in endpoint_config.h #1617

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
67 changes: 58 additions & 9 deletions src-electron/generator/helper-endpointconfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,8 @@ function endpoint_attribute_min_max_list(options) {

if (isNaN(def)) def = 0
if (isNaN(min)) min = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the correct min for a signed type. Should at least have a comment explaining why this is here in this form and why it's OK.

If the expectation is that this case never happens, shouldn't this error out instead of silently doing something wrong?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created https://github.com/project-chip/zap/pull/1618/files

if (isNaN(max)) max = 0xffff
if (isNaN(max)) max = '0x' + 'FF'.repeat(mm.typeSize)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the correct max for a signed type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created https://github.com/project-chip/zap/pull/1618/files


let defS =
(def >= 0 ? '' : '-') + '0x' + Math.abs(def).toString(16).toUpperCase()
let minS =
Expand All @@ -627,13 +628,19 @@ function endpoint_attribute_min_max_list(options) {
.forEach((tok) => {
switch (tok) {
case 'def':
defMinMaxItems.push(`(uint16_t)${defS}`)
defMinMaxItems.push(
`(${mm.isTypeSigned ? '' : 'u'}int${mm.typeSize * 8}_t)${defS}`
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect this will completely break things, since the actual type those structs store is in fact uint16_t, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did not cross check that. Just fixed this based on the typecasting being wrong

)
break
case 'min':
defMinMaxItems.push(`(uint16_t)${minS}`)
defMinMaxItems.push(
`(${mm.isTypeSigned ? '' : 'u'}int${mm.typeSize * 8}_t)${minS}`
)
break
case 'max':
defMinMaxItems.push(`(uint16_t)${maxS}`)
defMinMaxItems.push(
`(${mm.isTypeSigned ? '' : 'u'}int${mm.typeSize * 8}_t)${maxS}`
)
break
}
})
Expand Down Expand Up @@ -854,7 +861,13 @@ async function determineAttributeDefaultValue(
* 2.) If client is included on at least one endpoint add client atts.
* 3.) If server is included on at least one endpoint add server atts.
*/
async function collectAttributes(db, sessionId, endpointTypes, options) {
async function collectAttributes(
db,
sessionId,
endpointTypes,
options,
zclPackageIds
) {
let commandMfgCodes = [] // Array of { index, mfgCode } objects
let clusterMfgCodes = [] // Array of { index, mfgCode } objects
let attributeMfgCodes = [] // Array of { index, mfgCode } objects
Expand Down Expand Up @@ -1028,14 +1041,44 @@ async function collectAttributes(db, sessionId, endpointTypes, options) {
let mask = []
if ((a.min != null || a.max != null) && a.isWritable) {
mask.push('min_max')
let type_size_and_sign = await types.getSignAndSizeOfZclType(
db,
a.type,
zclPackageIds
)
let min, max
if (a.min != null) {
min = a.min
} else {
if (type_size_and_sign.isTypeSigned) {
// Signed min: -2^(typeSize*8-1)
min = -(2 ** (typeSize * 8 - 1))
} else {
// Unsigned min: 0
min = 0
}
}
if (a.max != null) {
max = a.max
} else {
if (type_size_and_sign.isTypeSigned) {
// Signed max: 2^(typeSize*8-1) - 1
max = 2 ** (typeSize * 8 - 1) - 1
} else {
// Unsigned max: 2^(typeSize*8) - 1
max = 2 ** (typeSize * 8) - 1
}
}
Comment on lines +1049 to +1071
Copy link

Choose a reason for hiding this comment

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

high

The calculation of default min and max values using the ** operator on standard JavaScript numbers can lead to a loss of precision for integer types larger than 53 bits (e.g., 64-bit integers). This can result in incorrect boundary values being generated.

To ensure correctness for all integer sizes, these calculations should be performed using BigInt.

          let min = a.min
          let max = a.max

          if (min == null) {
            if (type_size_and_sign.isTypeSigned) {
              // Signed min: -2^(typeSize*8-1)
              min = -(2n ** (BigInt(typeSize) * 8n - 1n))
            } else {
              // Unsigned min: 0
              min = 0n
            }
          }

          if (max == null) {
            if (type_size_and_sign.isTypeSigned) {
              // Signed max: 2^(typeSize*8-1) - 1
              max = 2n ** (BigInt(typeSize) * 8n - 1n) - 1n
            } else {
              // Unsigned max: 2^(typeSize*8) - 1
              max = 2n ** (BigInt(typeSize) * 8n) - 1n
            }
          }

let minMax = {
default: attributeDefaultValue,
min: a.min,
max: a.max,
min: min,
max: max,
name: a.name,
comment: cluster.comment,
typeSize: typeSize
typeSize: typeSize,
isTypeSigned: type_size_and_sign.isTypeSigned
}

attributeDefaultValue = `ZAP_MIN_MAX_DEFAULTS_INDEX(${minMaxIndex})`
defaultValueIsMacro = true
minMaxList.push(minMax)
Expand Down Expand Up @@ -1442,7 +1485,13 @@ function endpoint_config(options) {
collectAttributeSizes(db, this.global.zclPackageIds, endpointTypes)
)
.then((endpointTypes) =>
collectAttributes(db, sessionId, endpointTypes, collectAttributesOptions)
collectAttributes(
db,
sessionId,
endpointTypes,
collectAttributesOptions,
this.global.zclPackageIds
)
)
.then((collection) => {
Object.assign(newContext, collection)
Expand Down
4 changes: 2 additions & 2 deletions test/custom-matter-xml.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ test(
' /* Endpoint: 1, Cluster: Sample Custom Cluster (server) */ \\'
)
expect(endpointConfig).toContain(
'{ (uint16_t)0x0, (uint16_t)0x0, (uint16_t)0xFFFF }, /* Sample Mfg Specific Attribute 2 */ \\'
'{ (uint8_t)0x0, (uint8_t)0x0, (uint8_t)0xFF }, /* Sample Mfg Specific Attribute 2 */ \\'
)

let endpointOut = genResult.content['endpoints.out']
Expand Down Expand Up @@ -490,7 +490,7 @@ test(
' /* Endpoint: 1, Cluster: Sample Custom Cluster (server) */ \\'
)
expect(endpointConfig).not.toContain(
'{ (uint16_t)0x0, (uint16_t)0x0, (uint16_t)0xFFFF }, /* Sample Mfg Specific Attribute 2 */ \\'
'{ (uint8_t)0x0, (uint8_t)0x0, (uint8_t)0xFF }, /* Sample Mfg Specific Attribute 2 */ \\'
)

// create state from database and session to verify contents of .zap file
Expand Down
12 changes: 4 additions & 8 deletions test/gen-matter-3-1.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,12 +126,10 @@ test(
`{ 0x00000005, ZAP_TYPE(ENUM8), 1, ZAP_ATTRIBUTE_MASK(EXTERNAL_STORAGE) | ZAP_ATTRIBUTE_MASK(NULLABLE), ZAP_EMPTY_DEFAULT() }, /* LastNetworkingStatus */`
)
expect(ept).toContain(
'{ (uint16_t)0xFF, (uint16_t)0x64, (uint16_t)0xFFFF }, /* BallastFactorAdjustment */'
'{ (uint8_t)0xFF, (uint8_t)0x64, (uint8_t)0xFF }, /* BallastFactorAdjustment */'
)
expect(ept).toContain(`6, 'C', 'o', 'f', 'f', 'e', 'e', \\`)
expect(ept).toContain(
'{ (uint16_t)-0x64, (uint16_t)-0x96, (uint16_t)0xC8 }'
)
expect(ept).toContain('{ (int16_t)-0x64, (int16_t)-0x96, (int16_t)0xC8 }')
expect(ept).toContain('#define GENERATED_MIN_MAX_DEFAULT_COUNT 51')
expect(ept).toContain('#define GENERATED_ATTRIBUTE_COUNT 739')
expect(ept).toContain(`/* EventList (index=8) */ \\
Expand Down Expand Up @@ -238,12 +236,10 @@ test(
' { 0x00000000, ZAP_TYPE(TEMPERATURE), 2, ZAP_ATTRIBUTE_MASK(NULLABLE), ZAP_SIMPLE_DEFAULT(0x8000) },'
)
expect(ept).toContain(
'{ (uint16_t)0xFF, (uint16_t)0x64, (uint16_t)0xFFFF }, /* BallastFactorAdjustment */'
'{ (uint8_t)0xFF, (uint8_t)0x64, (uint8_t)0xFF }, /* BallastFactorAdjustment */'
)
expect(ept).toContain(`6, 'C', 'o', 'f', 'f', 'e', 'e', \\`)
expect(ept).toContain(
'{ (uint16_t)-0x64, (uint16_t)-0x96, (uint16_t)0xC8 }'
)
expect(ept).toContain('{ (int16_t)-0x64, (int16_t)-0x96, (int16_t)0xC8 }')
expect(ept).toContain('#define GENERATED_MIN_MAX_DEFAULT_COUNT 51')
expect(ept).toContain('#define GENERATED_ATTRIBUTE_COUNT 739')
expect(ept).toContain(`/* EventList (index=8) */ \\
Expand Down
2 changes: 1 addition & 1 deletion test/resource/custom-cluster/matter-conflict.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ limitations under the License.
</cluster>

<clusterExtension code="0x0006">
<attribute side="server" code="0xFFF10000" define="SAMPLE_MFG_SPECIFIC_TRANSITION_TIME_2" type="INT8U" min="0x0000" max="0xFFFF" writable="true" default="0x0000" optional="true">Sample Mfg Specific Attribute 2 Conflict</attribute>
<attribute side="server" code="0xFFF10000" define="SAMPLE_MFG_SPECIFIC_TRANSITION_TIME_2" type="INT8U" min="0x0000" max="0xFF" writable="true" default="0x0000" optional="true">Sample Mfg Specific Attribute 2 Conflict</attribute>
<command source="client" code="0xFFF100" name="SampleMfgSpecificOnWithTransition2Conflict" optional="true">
<description>Client command that turns the device on with a transition given
by the transition time in the Ember Sample transition time attribute.</description>
Expand Down
2 changes: 1 addition & 1 deletion test/resource/custom-cluster/matter-custom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ limitations under the License.

<!-- Use the cluster extension Extend the on/off cluster -->
<clusterExtension code="0x0006">
<attribute side="server" code="0xFFF10000" define="SAMPLE_MFG_SPECIFIC_TRANSITION_TIME_2" type="INT8U" min="0x0000" max="0xFFFF" writable="true" default="0x0000" optional="true">Sample Mfg Specific Attribute 2</attribute>
<attribute side="server" code="0xFFF10000" define="SAMPLE_MFG_SPECIFIC_TRANSITION_TIME_2" type="INT8U" min="0x0000" max="0xFF" writable="true" default="0x0000" optional="true">Sample Mfg Specific Attribute 2</attribute>
<attribute side="server" code="0xFFF10001" define="SAMPLE_MFG_SPECIFIC_TRANSITION_TIME_4" type="INT16U" min="0x0000" max="0xFFFF" writable="true" default="0x0000" optional="true">Sample Mfg Specific Attribute 4</attribute>
<command source="client" code="0xFFF100" name="SampleMfgSpecificOnWithTransition2">
<description>Client command that turns the device on with a transition given
Expand Down
2 changes: 1 addition & 1 deletion zcl-builtin/dotdot/BallastConfiguration.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ applicable to this document can be found in the LICENSE.md file.
<!-- PowerOnFadeTime is deprecated -->
<attribute id="0013" name="PowerOnFadeTime" type="uint16" writable="true" max="65534" default="0" deprecated="true" />
<attribute id="0014" name="IntrinsicBallastFactor" type="uint8" max="254" writable="true" />
<attribute id="0015" name="BallastFactorAdjustment" type="uint8" min="100" max="255" writable="true" default="255" />
<attribute id="0015" name="BallastFactorAdjustment" type="uint8" min="100" writable="true" default="255" />
<!-- Lamp Information Attribute Set -->
<attribute id="0020" name="LampQuantity" type="uint8" max="254" />
<!-- Lamp Settings Attribute Set -->
Expand Down
0