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

[ICD] kICDMonitoringBufferSize is too small to handle full "full width" node ID #35735

Closed
yunhanw-google opened this issue Sep 23, 2024 · 3 comments · Fixed by #35737
Closed
Assignees
Labels
bug Something isn't working

Comments

@yunhanw-google
Copy link
Contributor

Reproduction steps

When using "full width" node ID, for example, 0xffff000000000000, We cannot registerClient successfully using the lit example app, it would complain the buffer is too small,
https://github.com/project-chip/connectedhomeip/blob/17f25a1772f7ba7535d57cd4dfa43ac1fc295843/src/app/icd/server/ICDMonitoringTable.h#L37C25-L37C49

[1727120175.806454][15188:15188] CHIP:SVR: debug 63
0x08, tag[Fully Qualified (6 Bytes)]: 0xd973::0xffa3::0xff, type: Octet String (0x10), length: 0, value: hex:
[1727120175.806476][15188:15188] CHIP:SPT: debug>>>152701000000000000FFFF270201000100FDFFFFFF300310D0E9EB3B00000000
[1727120175.806486][15188:15188] CHIP:SPT: debug>>>0000000000000000300410D0E9EB3B00000000000000000000000024050018

We may do TLV::EstimateStructOverhead(sizeof(uint64_t), sizeof(uint64_t), sizeof(xxxx), etc) * 3 / 2

Bug prevalence

always

GitHub hash of the SDK that was being used

8750c55

Platform

core

Platform Version(s)

1.4

Anything else?

No response

@yunhanw-google yunhanw-google added bug Something isn't working needs triage labels Sep 23, 2024
@yunhanw-google yunhanw-google self-assigned this Sep 23, 2024
@bzbarsky-apple
Copy link
Contributor

Why * 3 / 2?

@yunhanw-google
Copy link
Contributor Author

yunhanw-google commented Sep 23, 2024

@bzbarsky-apple we need slack of at least 50% to make sure we can grow and then if there is a firmware upgrade/downgrade, that it doesn't brick

@bzbarsky-apple
Copy link
Contributor

Where is the "50%" coming from? What if the new firmware grows by more than that in terms of what it's storing (since presumbly it's the "downgrade" case that needs the extra space)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants