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

Use ByteSpan for string arguments in command handlers and attribute #5542

Closed
erjiaqing opened this issue Mar 22, 2021 · 4 comments · Fixed by #10745
Closed

Use ByteSpan for string arguments in command handlers and attribute #5542

erjiaqing opened this issue Mar 22, 2021 · 4 comments · Fixed by #10745
Assignees

Comments

@erjiaqing
Copy link
Contributor

Problem

The Zigbee protocol uses Pascal style string, which has 1/2 bytes of length in the front of data instead of NULL terminated. The code in cluster handlers, the attribute managements should accept a ByteSpan.

Proposed Solution

Use ByteSpan for string arguments in command handlers and attribute. Update codegen.

@bzbarsky-apple
Copy link
Contributor

We should use Span<char>, not ByteSpan, imo.... It would more accurately represent what's actually going on.

@erjiaqing
Copy link
Contributor Author

We should use Span<char>, not ByteSpan, imo.... It would more accurately represent what's actually going on.

I would still suggest using ByteSpan, per spec, its encoding is not specified, thus we may encounter issues on some platforms if Span<char> is used, the uint8_t is 8bit unsiged integer type, but char can be signed or unsigned integer per C++ spec.

@bzbarsky-apple
Copy link
Contributor

per spec, its encoding is not specified

Per CHIP spec the encoding of a string is specified to be UTF-8.

but char can be signed or unsigned integer per C++ spec.

That is a good point, because code units are of course unsigned. Nevertheless, the typical API that accepts UTF-8 strings uses char *; if we use something else we're going to need a bunch of casts in either SDK code that interfaces with app APIs or in app code that tries to do something with these strings.... Note that the TLV PutString/GetString APIs use char *.

I would also be in favor of adding a TLVReader::GetDataPtr (or other name) that hands out char * for the UTF-8 string type and fails for all other types, and perhaps restricting the current GetDataPtr to just the octet string type.

Another option for now would be to just add a DataModelString type, typedefed to whatever we want at the moment (though we still have to decide that), but then we can change the typedef later as needed if we discover that our choice was wrong. That would at least avoid us making changes in all the places where it's just passed through.

But I pretty strongly suspect that we should follow common convention and use char * for UTF-8 strings.

@holbrookt
Copy link
Contributor

Any update on this? Still blocking OTA

woody-apple pushed a commit that referenced this issue Oct 21, 2021
…ING (#10745)

* Fix various OCTET_STRING bits that should be CHAR_STRING.

Fixes #5542
Fixes #6112
Fixes #7112
Fixes #7322
Fixes #7654
Fixes #7655
Fixes #8704
Fixes #8705
Fixes #8706
Fixes #8707
Fixes #9797
Fixes #9798
Fixes #10508
Fixes #10509

* Regenerate generated files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants