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

Impl basic information home location attribute #34276

Draft
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

hicklin
Copy link
Contributor

@hicklin hicklin commented Jul 10, 2024

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.

Copy link

github-actions bot commented Jul 10, 2024

PR #34276: Size comparison from 47fee48 to 0a0f521

Increases above 0.2%:

platform target config section 47fee48 0a0f521 change % change
linux air-purifier-app debug FLASH 2529936 2539472 9536 0.4
all-clusters-app debug RAM 492176 495440 3264 0.7
bridge-app debug FLASH 4478256 4488192 9936 0.2
chip-tool debug RAM 545946 549610 3664 0.7
chip-tool-ipv6only arm64 RAM 594728 598376 3648 0.6
fabric-admin debug RAM 542754 546450 3696 0.7
fabric-bridge-app debug FLASH 4243776 4253744 9968 0.2
lock-app debug FLASH 4541600 4551568 9968 0.2
ota-provider-app debug FLASH 4197648 4207600 9952 0.2
ota-requestor-app debug FLASH 4323200 4333136 9936 0.2
shell debug FLASH 2804925 2814637 9712 0.3
Full report (16 builds for linux)
platform target config section 47fee48 0a0f521 change % change
linux air-purifier-app debug unknown 4592 4592 0 0.0
FLASH 2529936 2539472 9536 0.4
RAM 125072 125136 64 0.1
all-clusters-app debug unknown 5368 5368 0 0.0
FLASH 5590606 5600318 9712 0.2
RAM 492176 495440 3264 0.7
all-clusters-minimal-app debug unknown 5288 5288 0 0.0
FLASH 5061520 5071280 9760 0.2
RAM 235488 235552 64 0.0
bridge-app debug unknown 5256 5256 0 0.0
FLASH 4478256 4488192 9936 0.2
RAM 212744 212808 64 0.0
chip-tool debug unknown 5728 5728 0 0.0
FLASH 11769703 11784103 14400 0.1
RAM 545946 549610 3664 0.7
chip-tool-ipv6only arm64 unknown 20064 20096 32 0.2
FLASH 10882348 10896524 14176 0.1
RAM 594728 598376 3648 0.6
fabric-admin debug unknown 5616 5616 0 0.0
FLASH 10850375 10866407 16032 0.1
RAM 542754 546450 3696 0.7
fabric-bridge-app debug unknown 4528 4528 0 0.0
FLASH 4243776 4253744 9968 0.2
RAM 198808 198856 48 0.0
lighting-app debug+rpc+ui unknown 5936 5936 0 0.0
FLASH 5382594 5392386 9792 0.2
RAM 224096 224176 80 0.0
lock-app debug unknown 5192 5192 0 0.0
FLASH 4541600 4551568 9968 0.2
RAM 200240 200304 64 0.0
ota-provider-app debug unknown 4576 4576 0 0.0
FLASH 4197648 4207600 9952 0.2
RAM 194520 194584 64 0.0
ota-requestor-app debug unknown 4512 4512 0 0.0
FLASH 4323200 4333136 9936 0.2
RAM 199176 199240 64 0.0
shell debug unknown 4112 4112 0 0.0
FLASH 2804925 2814637 9712 0.3
RAM 153000 153080 80 0.1
thermostat-no-ble arm64 unknown 9144 9152 8 0.1
FLASH 4167996 4175844 7848 0.2
RAM 235640 235696 56 0.0
tv-app debug unknown 5472 5472 0 0.0
FLASH 5605920 5615808 9888 0.2
RAM 341760 341824 64 0.0
tv-casting-app debug unknown 5096 5096 0 0.0
FLASH 9939582 9956414 16832 0.2
RAM 402128 402736 608 0.2

return CHIP_NO_ERROR;
}

memcpy((void *) location.Value().locationName.data(), loc.Value().locationName.data(), loc.Value().locationName.size());
Copy link
Contributor Author

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?

Copy link
Contributor

@bzbarsky-apple bzbarsky-apple Jul 11, 2024

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:

  1. Define a struct type inheriting from DeviceLocatioType [sic] which has storage for the data.
  2. Define a copy assignment operator on that which takes DeviceLocatioType [sic] and copies things correctly.
  3. Pass in a reference to this new type.
  4. Decode into a DeviceLocatioType on the stack and then assign into the outparam.

@github-actions github-actions bot added esp32 zephyr tizen For Tizen platform labels Jul 11, 2024
Copy link

github-actions bot commented Jul 11, 2024

PR #34276: Size comparison from 47fee48 to b67437b

Increases above 0.2%:

platform target config section 47fee48 b67437b change % change
cc32xx air-purifier CC3235SF_LAUNCHXL FLASH 606074 608010 1936 0.3
lock CC3235SF_LAUNCHXL FLASH 651118 653078 1960 0.3
linux air-purifier-app debug FLASH 2529936 2540800 10864 0.4
all-clusters-app debug FLASH 5590606 5601998 11392 0.2
RAM 492176 496504 4328 0.9
all-clusters-minimal-app debug FLASH 5061520 5073104 11584 0.2
bridge-app debug FLASH 4478256 4489440 11184 0.2
chip-tool debug FLASH 11769703 11813015 43312 0.4
RAM 545946 551314 5368 1.0
chip-tool-ipv6only arm64 unknown 20064 20160 96 0.5
FLASH 10882348 10921916 39568 0.4
RAM 594728 600264 5536 0.9
fabric-admin debug FLASH 10850375 10893463 43088 0.4
RAM 542754 548186 5432 1.0
fabric-bridge-app debug FLASH 4243776 4254992 11216 0.3
lighting-app debug+rpc+ui FLASH 5382594 5393650 11056 0.2
lock-app debug FLASH 4541600 4553520 11920 0.3
ota-provider-app debug FLASH 4197648 4208848 11200 0.3
ota-requestor-app debug FLASH 4323200 4334208 11008 0.3
shell debug FLASH 2804925 2815773 10848 0.4
thermostat-no-ble arm64 unknown 9144 9192 48 0.5
tv-app debug FLASH 5605920 5637328 31408 0.6
tv-casting-app debug FLASH 9939582 9985870 46288 0.5
RAM 402128 403544 1416 0.4
telink shell tlsr9518adk80d FLASH 466316 470410 4094 0.9
window-covering tlsr9118bdk40d FLASH 519116 520424 1308 0.3
tizen all-clusters-app arm FLASH 1638592 1645236 6644 0.4
chip-tool-ubsan arm FLASH 16235438 16306838 71400 0.4
RAM 7138096 7167264 29168 0.4
Full report (59 builds for cc13x4_26x4, cc32xx, esp32, linux, mbed, nrfconnect, telink, tizen)
platform target config section 47fee48 b67437b change % change
cc13x4_26x4 lighting-app LP_EM_CC1354P10_6 FLASH 798348 799608 1260 0.2
RAM 109180 109180 0 0.0
lock-ftd LP_EM_CC1354P10_6 FLASH 816320 817548 1228 0.2
RAM 116956 116948 -8 -0.0
lock-mtd LP_EM_CC1354P10_6 FLASH 807820 809048 1228 0.2
RAM 111236 111236 0 0.0
pump-app LP_EM_CC1354P10_6 FLASH 760692 761936 1244 0.2
RAM 105336 105328 -8 -0.0
pump-controller-app LP_EM_CC1354P10_6 FLASH 746412 747664 1252 0.2
RAM 105576 105576 0 0.0
cc32xx air-purifier CC3235SF_LAUNCHXL FLASH 606074 608010 1936 0.3
RAM 204508 204508 0 0.0
lock CC3235SF_LAUNCHXL FLASH 651118 653078 1960 0.3
RAM 204780 204780 0 0.0
esp32 all-clusters-app c3devkit DRAM 90860 90932 72 0.1
FLASH 1469090 1470768 1678 0.1
IRAM 75570 75570 0 0.0
m5stack DRAM 117412 117420 8 0.0
FLASH 1538087 1539707 1620 0.1
IRAM 125403 125403 0 0.0
linux air-purifier-app debug unknown 4592 4592 0 0.0
FLASH 2529936 2540800 10864 0.4
RAM 125072 125176 104 0.1
all-clusters-app debug unknown 5368 5368 0 0.0
FLASH 5590606 5601998 11392 0.2
RAM 492176 496504 4328 0.9
all-clusters-minimal-app debug unknown 5288 5288 0 0.0
FLASH 5061520 5073104 11584 0.2
RAM 235488 235528 40 0.0
bridge-app debug unknown 5256 5256 0 0.0
FLASH 4478256 4489440 11184 0.2
RAM 212744 212896 152 0.1
chip-tool debug unknown 5728 5728 0 0.0
FLASH 11769703 11813015 43312 0.4
RAM 545946 551314 5368 1.0
chip-tool-ipv6only arm64 unknown 20064 20160 96 0.5
FLASH 10882348 10921916 39568 0.4
RAM 594728 600264 5536 0.9
fabric-admin debug unknown 5616 5616 0 0.0
FLASH 10850375 10893463 43088 0.4
RAM 542754 548186 5432 1.0
fabric-bridge-app debug unknown 4528 4528 0 0.0
FLASH 4243776 4254992 11216 0.3
RAM 198808 198880 72 0.0
lighting-app debug+rpc+ui unknown 5936 5936 0 0.0
FLASH 5382594 5393650 11056 0.2
RAM 224096 224216 120 0.1
lock-app debug unknown 5192 5192 0 0.0
FLASH 4541600 4553520 11920 0.3
RAM 200240 200280 40 0.0
ota-provider-app debug unknown 4576 4576 0 0.0
FLASH 4197648 4208848 11200 0.3
RAM 194520 194608 88 0.0
ota-requestor-app debug unknown 4512 4512 0 0.0
FLASH 4323200 4334208 11008 0.3
RAM 199176 199232 56 0.0
shell debug unknown 4112 4112 0 0.0
FLASH 2804925 2815773 10848 0.4
RAM 153000 153088 88 0.1
thermostat-no-ble arm64 unknown 9144 9192 48 0.5
FLASH 4167996 4176948 8952 0.2
RAM 235640 235896 256 0.1
tv-app debug unknown 5472 5472 0 0.0
FLASH 5605920 5637328 31408 0.6
RAM 341760 342184 424 0.1
tv-casting-app debug unknown 5096 5096 0 0.0
FLASH 9939582 9985870 46288 0.5
RAM 402128 403544 1416 0.4
mbed lock-app-release cy8cproto_062_4343w FLASH 1502500 1503804 1304 0.1
RAM 226640 226648 8 0.0
nrfconnect all-clusters-app nrf52840dk_nrf52840 FLASH 882448 883692 1244 0.1
RAM 142109 142229 120 0.1
nrf7002dk_nrf5340_cpuapp FLASH 952948 954112 1164 0.1
RAM 140537 140657 120 0.1
all-clusters-minimal-app nrf52840dk_nrf52840 FLASH 827984 829200 1216 0.1
RAM 141023 141123 100 0.1
light-switch-app nrf52840dk_nrf52840 FLASH 786864 788080 1216 0.2
RAM 132548 132652 104 0.1
nrf7002dk_nrf5340_cpuapp FLASH 932160 933384 1224 0.1
RAM 130241 130345 104 0.1
lighting-app nrf52840dk_nrf52840+rpc FLASH 871944 873172 1228 0.1
RAM 146831 146935 104 0.1
nrf52840dongle_nrf52840 FLASH 813764 814992 1228 0.2
RAM 154836 154940 104 0.1
nrf5340dk_nrf5340_cpuapp FLASH 769456 770688 1232 0.2
RAM 145637 145741 104 0.1
nrf7002dk_nrf5340_cpuapp FLASH 932160 933384 1224 0.1
RAM 130241 130345 104 0.1
lock-app nrf52840dk_nrf52840 FLASH 798460 799876 1416 0.2
RAM 133071 133171 100 0.1
nrf5340dk_nrf5340_cpuapp FLASH 723752 725168 1416 0.2
RAM 133137 133237 100 0.1
nrf7002dk_nrf5340_cpuapp FLASH 944696 946120 1424 0.2
RAM 130764 130864 100 0.1
pump-app nrf52840dk_nrf52840 FLASH 751240 752464 1224 0.2
RAM 131756 131860 104 0.1
pump-controller-app nrf52840dk_nrf52840 FLASH 737852 739076 1224 0.2
RAM 131555 131659 104 0.1
telink air-quality-sensor-app tlsr9528a_retention FLASH 632734 634032 1298 0.2
RAM 50424 50528 104 0.2
all-clusters-app tlsr9118bdk40d FLASH 658574 659894 1320 0.2
RAM 148296 148408 112 0.1
all-clusters-minimal-app tlsr9528a FLASH 778900 780196 1296 0.2
RAM 113116 113212 96 0.1
bridge-app tlsr9258a FLASH 675720 677028 1308 0.2
RAM 95200 95304 104 0.1
contact-sensor-app tlsr9528a_retention FLASH 634318 635616 1298 0.2
RAM 50468 50572 104 0.2
light-switch-app-ota-shell-factory-data tlsr9528a FLASH 720192 721500 1308 0.2
RAM 77044 77148 104 0.1
lighting-app-ota-factory-data tlsr9118bdk40d FLASH 613722 615030 1308 0.2
RAM 144532 144636 104 0.1
lighting-app-ota-rpc-factory-data-4mb tlsr9518adk80d FLASH 801474 802782 1308 0.2
RAM 102936 103040 104 0.1
lock-app-dfu tlsr9528a FLASH 665886 667182 1296 0.2
RAM 69756 69852 96 0.1
ota-requestor-app tlsr9258a FLASH 695050 696358 1308 0.2
RAM 94924 95028 104 0.1
pump-app tlsr9518adk80d FLASH 616582 617890 1308 0.2
RAM 56848 56952 104 0.2
pump-controller-app tlsr9518adk80d FLASH 606966 608274 1308 0.2
RAM 56648 56752 104 0.2
shell tlsr9518adk80d FLASH 466316 470410 4094 0.9
RAM 72460 72484 24 0.0
smoke_co_alarm-app tlsr9528a_retention FLASH 640936 642234 1298 0.2
RAM 52096 52200 104 0.2
temperature-measurement-app-mars-ota tlsr9518adk80d FLASH 650792 652100 1308 0.2
RAM 60284 60388 104 0.2
thermostat tlsr9518adk80d FLASH 625856 627164 1308 0.2
RAM 56980 57084 104 0.2
window-covering tlsr9118bdk40d FLASH 519116 520424 1308 0.3
RAM 97696 97800 104 0.1
tizen all-clusters-app arm unknown 1584 1584 0 0.0
FLASH 1638592 1645236 6644 0.4
RAM 48540 48548 8 0.0
chip-tool-ubsan arm unknown 2384 2384 0 0.0
FLASH 16235438 16306838 71400 0.4
RAM 7138096 7167264 29168 0.4

Copy link
Contributor

@bzbarsky-apple bzbarsky-apple left a 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.

ReturnErrorOnFailure(tlvReader.Next(TLV::AnonymousTag()));
ReturnErrorOnFailure(loc->Decode(tlvReader));

// This would not be needed if the Decode methed proprly decodes a null value.
Copy link
Contributor Author

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.

@mergify mergify bot added the conflict label Dec 17, 2024
@mergify mergify bot added conflict and removed conflict labels Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Add a location attribute to the Basic Information cluster
3 participants