-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Impl basic information home location attribute #34276
base: master
Are you sure you want to change the base?
Impl basic information home location attribute #34276
Conversation
…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)
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
DeviceLocatioType
on 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)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That "write to const memory area" thing absolutely needs to be fixed.
And this PR probably needs to be broken up into separate "update XML" and "add implementation" PRs.
…ow for a mutabel device location.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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.