Reduce RAM usage from CHIPDeviceEvent #17959
Merged
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
- CHIPDeviceEvent has had a historical `InternetConnectivityChange` event that embeds a 46 byte string (instead of a 16 byte IPAddress) and is *only used for logging* in examples.= - This event is in a union and is the largest of all the sub-structs, therefore forcing max-sized events to that event. Issue project-chip#9261 This PR: - Changes the `address` from a string to an IPAddress - Renames to `ipAddress` to break external clients using this struct so that they notice on the next roll to master (usage change is trivial). - Updates all examples to no longer log the address, since it was only logged for IPv4 and not IPv6, and IPv6 is the standard, so if it was good enough for IPv6 not to log, it's good enough for IPv4. - Make IPAddress trivially copyable by removing a non-trivial `operator=` that was actually implementing the trivial copy. Upstream OpenWeave code that was the basis for the IPAddress.h file removed it already in late 2021, > 1.5 years after import (see openweave/openweave-core@fbbb01c#diff-33121ed998c085b8dc0026f30f24aaadb8b8b36042e38b449cec15c32c8ba86e) - Update all platforms that populated the address to use the actual address, not the string Testing done: - All cert tests pass - All unit tests pass
pullapprove
bot
requested review from
andy31415,
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic and
dhrishi
pullapprove
bot
requested review from
msandstedt,
mspang,
robszewczyk,
sagar-apple,
saurabhst,
selissia,
tecimovic,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yufengwangca
bzbarsky-apple
approved these changes
May 2, 2022
andy31415
approved these changes
May 2, 2022
|
PR #17959: Size comparison from 0824e95 to 90dd900 Increases (15 builds for cc13x2_26x2, efr32, esp32, mbed, p6)
Decreases (16 builds for cc13x2_26x2, cyw30739, efr32, k32w, nrfconnect, telink)
Full report (26 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, mbed, nrfconnect, p6, telink)
|
|
PR #17959: Size comparison from 0824e95 to 5517f1d Increases (13 builds for cc13x2_26x2, efr32, esp32, linux, mbed)
Decreases (18 builds for cc13x2_26x2, cyw30739, efr32, k32w, linux, nrfconnect, p6, telink)
Full report (28 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
msandstedt
approved these changes
May 2, 2022
tcarmelveilleux
added a commit
to tcarmelveilleux/connectedhomeip
that referenced
this pull request
May 2, 2022
Issue project-chip#9261 - K32W platform ran out of BSS last week - Current usage is default max 100 events in queue. PR project-chip#17959 already reduced the size of each event and saved approx 600 bytes. This PR should reduce it further. - This PR moves max queue size from 100 to 75, which is 25% reduction and much larger than some platforms which use 25 (which seems low to me) - Other Thread platforms like QPG and EFR32 run on 25, so not worried here Testing done: - Sanity check on K32W via @doru91
andy31415
pushed a commit
that referenced
this pull request
May 3, 2022
Issue #9261 - K32W platform ran out of BSS last week - Current usage is default max 100 events in queue. PR #17959 already reduced the size of each event and saved approx 600 bytes. This PR should reduce it further. - This PR moves max queue size from 100 to 75, which is 25% reduction and much larger than some platforms which use 25 (which seems low to me) - Other Thread platforms like QPG and EFR32 run on 25, so not worried here Testing done: - Sanity check on K32W via @doru91
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.
Problem
InternetConnectivityChangeevent that embeds a 46 byte string (instead of a 16 byte IPAddress)
and is only used for logging in examples.=
therefore forcing max-sized events to that event.
Issue #9261
Change overview
addressfrom a string to an IPAddressipAddressto break external clients using this structso that they notice on the next roll to master (usage change is trivial).
logged for IPv4 and not IPv6, and IPv6 is the standard, so if it was
good enough for IPv6 not to log, it's good enough for IPv4.
operator=that was actually implementing thetrivial copy. Upstream OpenWeave code that was the basis for the IPAddress.h file removed it already in late
2021, > 1.5 years after import (see
openweave/openweave-core@fbbb01c#diff-33121ed998c085b8dc0026f30f24aaadb8b8b36042e38b449cec15c32c8ba86e)
not the string
Testing