Skip to content

Commit

Permalink
Reduce RAM usage from CHIPDeviceEvent (#17959)
Browse files Browse the repository at this point in the history
* Reduce RAM usage from CHIPDeviceEvent

- 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 #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

* Restyled by clang-format

* Fix CI on newer compilers

* Restyled by clang-format

Co-authored-by: Restyled.io <commits@restyled.io>
  • Loading branch information
2 people authored and pull[bot] committed Nov 8, 2023
1 parent 9d52492 commit 9e9247d
Show file tree
Hide file tree
Showing 24 changed files with 89 additions and 114 deletions.
2 changes: 1 addition & 1 deletion examples/all-clusters-app/ameba/main/DeviceCallbacks.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ void DeviceCallbacks::OnInternetConnectivityChange(const ChipDeviceEvent * event
{
if (event->InternetConnectivityChange.IPv4 == kConnectivity_Established)
{
ChipLogProgress(DeviceLayer, "Server ready at: %s:%d", event->InternetConnectivityChange.address, CHIP_PORT);
ChipLogProgress(DeviceLayer, "IPv4 Server ready...");
chip::app::DnssdServer::Instance().StartServer();
}
else if (event->InternetConnectivityChange.IPv4 == kConnectivity_Lost)
Expand Down
2 changes: 1 addition & 1 deletion examples/all-clusters-app/esp32/main/DeviceCallbacks.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ void DeviceCallbacks::OnInternetConnectivityChange(const ChipDeviceEvent * event
static bool isOTAInitialized = false;
if (event->InternetConnectivityChange.IPv4 == kConnectivity_Established)
{
ESP_LOGI(TAG, "Server ready at: %s:%d", event->InternetConnectivityChange.address, CHIP_PORT);
ESP_LOGI(TAG, "IPv4 Server ready...");
wifiLED.Set(true);
chip::app::DnssdServer::Instance().StartServer();

Expand Down
2 changes: 1 addition & 1 deletion examples/bridge-app/esp32/main/DeviceCallbacks.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ void DeviceCallbacks::OnInternetConnectivityChange(const ChipDeviceEvent * event
{
if (event->InternetConnectivityChange.IPv4 == kConnectivity_Established)
{
ESP_LOGI(TAG, "Server ready at: %s:%d", event->InternetConnectivityChange.address, CHIP_PORT);
ESP_LOGI(TAG, "IPv4 Server ready...");
chip::app::DnssdServer::Instance().StartServer();
}
else if (event->InternetConnectivityChange.IPv4 == kConnectivity_Lost)
Expand Down
2 changes: 1 addition & 1 deletion examples/chef/esp32/main/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ void DeviceEventCallback(const ChipDeviceEvent * event, intptr_t arg)
case DeviceEventType::kInternetConnectivityChange:
if (event->InternetConnectivityChange.IPv4 == kConnectivity_Established)
{
ChipLogProgress(Shell, "Server ready at: %s:%d", event->InternetConnectivityChange.address, CHIP_PORT);
ChipLogProgress(Shell, "IPv4 Server ready...");
chip::app::DnssdServer::Instance().StartServer();
}
else if (event->InternetConnectivityChange.IPv4 == kConnectivity_Lost)
Expand Down
2 changes: 1 addition & 1 deletion examples/lighting-app/ameba/main/DeviceCallbacks.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ void DeviceCallbacks::OnInternetConnectivityChange(const ChipDeviceEvent * event
{
if (event->InternetConnectivityChange.IPv4 == kConnectivity_Established)
{
printf("Server ready at: %s:%d", event->InternetConnectivityChange.address, CHIP_PORT);
printf("IPv4 Server ready...");
chip::app::DnssdServer::Instance().StartServer();
}
else if (event->InternetConnectivityChange.IPv4 == kConnectivity_Lost)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ void DeviceCallbacks::OnInternetConnectivityChange(const ChipDeviceEvent * event
{
if (event->InternetConnectivityChange.IPv4 == kConnectivity_Established)
{
log_info("Server ready at: %s:%d\r\n", event->InternetConnectivityChange.address, CHIP_PORT);
log_info("IPv4 Server ready...\r\n");
// TODO
// wifiLED.Set(true);
chip::app::DnssdServer::Instance().StartServer();
Expand Down
2 changes: 1 addition & 1 deletion examples/lighting-app/esp32/main/DeviceCallbacks.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ void DeviceCallbacks::OnInternetConnectivityChange(const ChipDeviceEvent * event
static bool isOTAInitialized = false;
if (event->InternetConnectivityChange.IPv4 == kConnectivity_Established)
{
ESP_LOGI(TAG, "Server ready at: %s:%d", event->InternetConnectivityChange.address, CHIP_PORT);
ESP_LOGI(TAG, "IPv4 Server ready...");
chip::app::DnssdServer::Instance().StartServer();

if (!isOTAInitialized)
Expand Down
2 changes: 1 addition & 1 deletion examples/lock-app/esp32/main/DeviceCallbacks.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ void DeviceCallbacks::OnInternetConnectivityChange(const ChipDeviceEvent * event
{
if (event->InternetConnectivityChange.IPv4 == kConnectivity_Established)
{
ESP_LOGI(TAG, "Server ready at: %s:%d", event->InternetConnectivityChange.address, CHIP_PORT);
ESP_LOGI(TAG, "IPv4 Server ready...");
chip::app::DnssdServer::Instance().StartServer();
}
else if (event->InternetConnectivityChange.IPv4 == kConnectivity_Lost)
Expand Down
2 changes: 1 addition & 1 deletion examples/ota-provider-app/esp32/main/DeviceCallbacks.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ void DeviceCallbacks::OnInternetConnectivityChange(const ChipDeviceEvent * event
{
if (event->InternetConnectivityChange.IPv4 == kConnectivity_Established)
{
ESP_LOGI(TAG, "Server ready at: %s:%d", event->InternetConnectivityChange.address, CHIP_PORT);
ESP_LOGI(TAG, "IPv4 Server ready...");
chip::app::DnssdServer::Instance().StartServer();
}
else if (event->InternetConnectivityChange.IPv4 == kConnectivity_Lost)
Expand Down
2 changes: 1 addition & 1 deletion examples/ota-requestor-app/ameba/main/DeviceCallbacks.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ void DeviceCallbacks::OnInternetConnectivityChange(const ChipDeviceEvent * event
{
if (event->InternetConnectivityChange.IPv4 == kConnectivity_Established)
{
ChipLogProgress(DeviceLayer, "Server ready at: %s:%d", event->InternetConnectivityChange.address, CHIP_PORT);
ChipLogProgress(DeviceLayer, "IPv4 Server ready...");
chip::app::DnssdServer::Instance().StartServer();
}
else if (event->InternetConnectivityChange.IPv4 == kConnectivity_Lost)
Expand Down
2 changes: 1 addition & 1 deletion examples/ota-requestor-app/esp32/main/DeviceCallbacks.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ void DeviceCallbacks::OnInternetConnectivityChange(const ChipDeviceEvent * event
static bool isOTAInitialized = false;
if (event->InternetConnectivityChange.IPv4 == kConnectivity_Established)
{
ESP_LOGI(TAG, "Server ready at: %s:%d", event->InternetConnectivityChange.address, CHIP_PORT);
ESP_LOGI(TAG, "IPv4 Server ready...");
chip::app::DnssdServer::Instance().StartServer();
if (!isOTAInitialized)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ void DeviceCallbacks::OnInternetConnectivityChange(const ChipDeviceEvent * event
{
if (event->InternetConnectivityChange.IPv4 == kConnectivity_Established)
{
ESP_LOGI(TAG, "Server ready at: %s:%d", event->InternetConnectivityChange.address, CHIP_PORT);
ESP_LOGI(TAG, "IPv4 Server ready...");
chip::app::DnssdServer::Instance().StartServer();
}
else if (event->InternetConnectivityChange.IPv4 == kConnectivity_Lost)
Expand Down
10 changes: 9 additions & 1 deletion src/include/platform/CHIPDeviceEvent.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#pragma once
#include <stdint.h>

#include <inet/IPAddress.h>
#include <lib/core/DataModelTypes.h>

namespace chip {
Expand Down Expand Up @@ -373,7 +374,14 @@ struct ChipDeviceEvent final
{
ConnectivityChange IPv4;
ConnectivityChange IPv6;
char address[INET6_ADDRSTRLEN];
// WARNING: There used to be `char address[INET6_ADDRSTRLEN]` here and it is
// deprecated/removed since it was too large and only used for logging.
// Consider not relying on ipAddress field either since the platform
// layer *does not actually validate* that the actual internet is reachable
// before issuing this event *and* there may be multiple addresses
// (especially IPv6) so it's recommended to use `ChipDevicePlatformEvent`
// instead and do something that is better for your platform.
chip::Inet::IPAddress ipAddress;
} InternetConnectivityChange;
struct
{
Expand Down
13 changes: 0 additions & 13 deletions src/inet/IPAddress.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,19 +57,6 @@ bool IPAddress::operator!=(const IPAddress & other) const
return Addr[0] != other.Addr[0] || Addr[1] != other.Addr[1] || Addr[2] != other.Addr[2] || Addr[3] != other.Addr[3];
}

IPAddress & IPAddress::operator=(const IPAddress & other)
{
if (this != &other)
{
Addr[0] = other.Addr[0];
Addr[1] = other.Addr[1];
Addr[2] = other.Addr[2];
Addr[3] = other.Addr[3];
}

return *this;
}

#if CHIP_SYSTEM_CONFIG_USE_LWIP && !CHIP_SYSTEM_CONFIG_USE_OPEN_THREAD_ENDPOINT

IPAddress::IPAddress(const ip6_addr_t & ipv6Addr)
Expand Down
15 changes: 4 additions & 11 deletions src/inet/IPAddress.h
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ union SockAddr
* @details
* The CHIP Inet Layer uses objects of this class to represent Internet
* protocol addresses (independent of protocol version).
*
*/
class DLL_EXPORT IPAddress
{
Expand All @@ -141,8 +142,7 @@ class DLL_EXPORT IPAddress
static constexpr uint16_t kMaxStringLength = OT_IP6_ADDRESS_STRING_SIZE;
#endif

IPAddress() = default;
IPAddress(const IPAddress & other) = default;
IPAddress() = default;

#if CHIP_SYSTEM_CONFIG_USE_LWIP && !CHIP_SYSTEM_CONFIG_USE_OPEN_THREAD_ENDPOINT
explicit IPAddress(const ip6_addr_t & ipv6Addr);
Expand Down Expand Up @@ -324,15 +324,6 @@ class DLL_EXPORT IPAddress
*/
bool operator!=(const IPAddress & other) const;

/**
* @brief Conventional assignment operator.
*
* @param[in] other The address to copy.
*
* @return A reference to this object.
*/
IPAddress & operator=(const IPAddress & other);

/**
* @brief Emit the IP address in conventional text presentation format.
*
Expand Down Expand Up @@ -667,5 +658,7 @@ class DLL_EXPORT IPAddress
static IPAddress Any;
};

static_assert(std::is_trivial<IPAddress>::value, "IPAddress is not trivial");

} // namespace Inet
} // namespace chip
11 changes: 0 additions & 11 deletions src/inet/IPPrefix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -49,17 +49,6 @@ bool IPPrefix::operator!=(const IPPrefix & other) const
return IPAddr != other.IPAddr || Length != other.Length;
}

IPPrefix & IPPrefix::operator=(const IPPrefix & other)
{
if (this != &other)
{
IPAddr = other.IPAddr;
Length = other.Length;
}

return *this;
}

bool IPPrefix::MatchAddress(const IPAddress & addr) const
{
uint8_t l = (Length <= 128) ? Length : 128;
Expand Down
15 changes: 0 additions & 15 deletions src/inet/IPPrefix.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,6 @@ class IPPrefix
IPPrefix() = default;
IPPrefix(const IPAddress & ipAddress, uint8_t length) : IPAddr(ipAddress), Length(length) {}

/**
* Copy constructor for the IPPrefix class.
*
*/
IPPrefix(const IPPrefix & other) = default;

/** An IPv6 or IPv4 address. */
IPAddress IPAddr;

Expand Down Expand Up @@ -107,15 +101,6 @@ class IPPrefix
*/
bool operator!=(const IPPrefix & other) const;

/**
* @brief Conventional assignment operator.
*
* @param[in] other the prefix to copy.
*
* @return a reference to this object.
*/
IPPrefix & operator=(const IPPrefix & other);

/**
* @brief Test if an address matches the prefix.
*
Expand Down
9 changes: 5 additions & 4 deletions src/platform/Ameba/ConnectivityManagerImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -699,10 +699,11 @@ void ConnectivityManagerImpl::UpdateInternetConnectivityState(void)

// Alert other components of the state change.
ChipDeviceEvent event;
event.Type = DeviceEventType::kInternetConnectivityChange;
event.InternetConnectivityChange.IPv4 = GetConnectivityChange(hadIPv4Conn, haveIPv4Conn);
event.InternetConnectivityChange.IPv6 = GetConnectivityChange(hadIPv6Conn, haveIPv6Conn);
addr.ToString(event.InternetConnectivityChange.address);
event.Type = DeviceEventType::kInternetConnectivityChange;
event.InternetConnectivityChange.IPv4 = GetConnectivityChange(hadIPv4Conn, haveIPv4Conn);
event.InternetConnectivityChange.IPv6 = GetConnectivityChange(hadIPv6Conn, haveIPv6Conn);
event.InternetConnectivityChange.ipAddress = addr;

PlatformMgr().PostEventOrDie(&event);

if (haveIPv4Conn != hadIPv4Conn)
Expand Down
9 changes: 5 additions & 4 deletions src/platform/EFR32/ConnectivityManagerImpl_WIFI.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -450,10 +450,11 @@ void ConnectivityManagerImpl::UpdateInternetConnectivityState(void)

// Alert other components of the state change.
ChipDeviceEvent event;
event.Type = DeviceEventType::kInternetConnectivityChange;
event.InternetConnectivityChange.IPv4 = GetConnectivityChange(hadIPv4Conn, haveIPv4Conn);
event.InternetConnectivityChange.IPv6 = GetConnectivityChange(hadIPv6Conn, haveIPv6Conn);
addr.ToString(event.InternetConnectivityChange.address, sizeof(event.InternetConnectivityChange.address));
event.Type = DeviceEventType::kInternetConnectivityChange;
event.InternetConnectivityChange.IPv4 = GetConnectivityChange(hadIPv4Conn, haveIPv4Conn);
event.InternetConnectivityChange.IPv6 = GetConnectivityChange(hadIPv6Conn, haveIPv6Conn);
event.InternetConnectivityChange.ipAddress = addr;

(void) PlatformMgr().PostEvent(&event);

if (haveIPv4Conn != hadIPv4Conn)
Expand Down
9 changes: 5 additions & 4 deletions src/platform/ESP32/ConnectivityManagerImpl_WiFi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1011,10 +1011,11 @@ void ConnectivityManagerImpl::UpdateInternetConnectivityState(void)

// Alert other components of the state change.
ChipDeviceEvent event;
event.Type = DeviceEventType::kInternetConnectivityChange;
event.InternetConnectivityChange.IPv4 = GetConnectivityChange(hadIPv4Conn, haveIPv4Conn);
event.InternetConnectivityChange.IPv6 = GetConnectivityChange(hadIPv6Conn, haveIPv6Conn);
addr.ToString(event.InternetConnectivityChange.address);
event.Type = DeviceEventType::kInternetConnectivityChange;
event.InternetConnectivityChange.IPv4 = GetConnectivityChange(hadIPv4Conn, haveIPv4Conn);
event.InternetConnectivityChange.IPv6 = GetConnectivityChange(hadIPv6Conn, haveIPv6Conn);
event.InternetConnectivityChange.ipAddress = addr;

PlatformMgr().PostEventOrDie(&event);

if (haveIPv4Conn != hadIPv4Conn)
Expand Down
14 changes: 8 additions & 6 deletions src/platform/Linux/ConnectivityManagerImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1181,13 +1181,15 @@ void ConnectivityManagerImpl::PostNetworkConnect()
if ((it.GetAddress(addr) == CHIP_NO_ERROR) && addr.IsIPv4())
{
ChipDeviceEvent event;
event.Type = DeviceEventType::kInternetConnectivityChange;
event.InternetConnectivityChange.IPv4 = kConnectivity_Established;
event.InternetConnectivityChange.IPv6 = kConnectivity_NoChange;
addr.ToString(event.InternetConnectivityChange.address);
event.Type = DeviceEventType::kInternetConnectivityChange;
event.InternetConnectivityChange.IPv4 = kConnectivity_Established;
event.InternetConnectivityChange.IPv6 = kConnectivity_NoChange;
event.InternetConnectivityChange.ipAddress = addr;

ChipLogDetail(DeviceLayer, "Got IP address on interface: %s IP: %s", ifName,
event.InternetConnectivityChange.address);
char ipStrBuf[chip::Inet::IPAddress::kMaxStringLength] = { 0 };
addr.ToString(ipStrBuf);

ChipLogDetail(DeviceLayer, "Got IP address on interface: %s IP: %s", ifName, ipStrBuf);

PlatformMgr().PostEventOrDie(&event);
}
Expand Down
10 changes: 5 additions & 5 deletions src/platform/Linux/PlatformManagerImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -149,15 +149,15 @@ void PlatformManagerImpl::WiFIIPChangeListener()
continue;
}

char ipStrBuf[chip::Inet::IPAddress::kMaxStringLength] = { 0 };
inet_ntop(AF_INET, RTA_DATA(routeInfo), ipStrBuf, sizeof(ipStrBuf));
ChipLogDetail(DeviceLayer, "Got IP address on interface: %s IP: %s", name, ipStrBuf);

ChipDeviceEvent event;
event.Type = DeviceEventType::kInternetConnectivityChange;
event.InternetConnectivityChange.IPv4 = kConnectivity_Established;
event.InternetConnectivityChange.IPv6 = kConnectivity_NoChange;
inet_ntop(AF_INET, RTA_DATA(routeInfo), event.InternetConnectivityChange.address,
sizeof(event.InternetConnectivityChange.address));

ChipLogDetail(DeviceLayer, "Got IP address on interface: %s IP: %s", name,
event.InternetConnectivityChange.address);
VerifyOrDie(chip::Inet::IPAddress::FromString(ipStrBuf, event.InternetConnectivityChange.ipAddress));

CHIP_ERROR status = PlatformMgr().PostEvent(&event);
if (status != CHIP_NO_ERROR)
Expand Down
8 changes: 4 additions & 4 deletions src/platform/P6/ConnectivityManagerImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -657,10 +657,10 @@ void ConnectivityManagerImpl::UpdateInternetConnectivityState(void)

// Alert other components of the state change.
ChipDeviceEvent event;
event.Type = DeviceEventType::kInternetConnectivityChange;
event.InternetConnectivityChange.IPv4 = GetConnectivityChange(hadIPv4Conn, haveIPv4Conn);
event.InternetConnectivityChange.IPv6 = GetConnectivityChange(hadIPv6Conn, haveIPv6Conn);
addr.ToString(event.InternetConnectivityChange.address);
event.Type = DeviceEventType::kInternetConnectivityChange;
event.InternetConnectivityChange.IPv4 = GetConnectivityChange(hadIPv4Conn, haveIPv4Conn);
event.InternetConnectivityChange.IPv6 = GetConnectivityChange(hadIPv6Conn, haveIPv6Conn);
event.InternetConnectivityChange.ipAddress = addr;
PlatformMgr().PostEventOrDie(&event);

if (haveIPv4Conn != hadIPv4Conn)
Expand Down
Loading

0 comments on commit 9e9247d

Please sign in to comment.