Impl basic information home location attribute #34276
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…namespace. Added the DeviceLocation attribute to the basic infromation cluster.
… BasicInformation cluster.
…e all-clusters-app.
…formation cluster XML.
…_information_home_location_attribute
|
PR #34276: Size comparison from 47fee48 to 0a0f521 Increases above 0.2%:
Full report (16 builds for linux)
|
hicklin
commented
Jul 11, 2024
| return CHIP_NO_ERROR; | ||
| } | ||
|
|
||
| memcpy((void *) location.Value().locationName.data(), loc.Value().locationName.data(), loc.Value().locationName.size()); |
There was a problem hiding this comment.
@bzbarsky-apple Is there a simpler way of achieving this? I was expecting the Decode method to copy the contents of the tlvReader to the location argument. Maybe there is a different Decode method that does this?
There was a problem hiding this comment.
Here's the thing. DeviceLocatioType [sic] is an alias for DeviceLocation::TypeInfo::Type, which is Nullable<Structs::HomeLocationStruct::Type>. And that's defined as:
struct Type
{
public:
chip::CharSpan locationName;
so location.Value().locationName.data() is a const char *.
The code above is hiding that "oh, I am copying to a pointer that points to const data, which is generally not OK" with that C-style cast (which I bet got added because the compiler complained about passing a const pointer to a non-const void* arg!), but there is absolutely no way to tell from the code in this function that this memcpy business is safe, much less telling it from inside the Decode method or anything like that.
The normal way I would approach this is:
- Define a struct type inheriting from
DeviceLocatioType[sic] which has storage for the data. - Define a copy assignment operator on that which takes
DeviceLocatioType[sic] and copies things correctly. - Pass in a reference to this new type.
- Decode into a
DeviceLocatioTypeon the stack and then assign into the outparam.
|
PR #34276: Size comparison from 47fee48 to b67437b Increases above 0.2%:
Full report (59 builds for cc13x4_26x4, cc32xx, esp32, linux, mbed, nrfconnect, telink, tizen)
|
bzbarsky-apple
requested changes
Jul 12, 2024
…ow for a mutabel device location.
hicklin
commented
Jul 16, 2024
| ReturnErrorOnFailure(tlvReader.Next(TLV::AnonymousTag())); | ||
| ReturnErrorOnFailure(loc->Decode(tlvReader)); | ||
|
|
||
| // This would not be needed if the Decode methed proprly decodes a null value. |
There was a problem hiding this comment.
@bzbarsky-apple related to the slack message, I cannot find a way to decode a null DeviceLocatioType from the encoded bytes. This is the solution that I have came up with so far.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR Fixes #33568.
This PR depends on PR #34208 that adds the Device Location attribute to the Basic Information cluster XML.
This is still work in progress as we try to figure out the best way to store and retrieve the Device Location information.