Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Revisit MRP implementation (#11988)
Browse files Browse the repository at this point in the history
kghost authored and pull[bot] committed Dec 11, 2023
1 parent c342a02 commit 4936579
Showing 45 changed files with 256 additions and 656 deletions.
11 changes: 0 additions & 11 deletions config/esp32/components/chip/Kconfig
Original file line number Diff line number Diff line change
@@ -67,17 +67,6 @@ menu "CHIP Core"
message handlers with the CHIP message layer to direct incoming messages to
their code.

config RMP_TIMER_DEFAULT_PERIOD_SHIFT
int "Default WRMP Timer Tick Interval Shift (ms)"
range 0 16
default 6
help
The default interval shift, in milliseconds (e.g. 6 bits shift
= 64ms), at which items in the WRMP pending message list are
processed for the purpose of retransmission or timeout.

This value can be overridden by the application at runtime.

config ENABLE_PW_RPC
bool "Enable Pigweed RPC library"
default n
4 changes: 2 additions & 2 deletions examples/chip-tool/commands/discover/Commands.h
Original file line number Diff line number Diff line change
@@ -55,12 +55,12 @@ class Resolve : public DiscoverCommand, public chip::Dnssd::ResolverDelegate
auto retryInterval = nodeData.GetMrpRetryIntervalIdle();

if (retryInterval.HasValue())
ChipLogProgress(chipTool, " MRP retry interval (idle): %" PRIu32 "ms", retryInterval.Value());
ChipLogProgress(chipTool, " MRP retry interval (idle): %" PRIu32 "ms", retryInterval.Value().count());

retryInterval = nodeData.GetMrpRetryIntervalActive();

if (retryInterval.HasValue())
ChipLogProgress(chipTool, " MRP retry interval (active): %" PRIu32 "ms", retryInterval.Value());
ChipLogProgress(chipTool, " MRP retry interval (active): %" PRIu32 "ms", retryInterval.Value().count());

ChipLogProgress(chipTool, " Supports TCP: %s", nodeData.mSupportsTcp ? "yes" : "no");
SetCommandExitStatus(CHIP_NO_ERROR);
2 changes: 1 addition & 1 deletion examples/lighting-app/efr32/include/CHIPProjectConfig.h
Original file line number Diff line number Diff line change
@@ -128,4 +128,4 @@
* needs (e.g. sleeping period) using Service Discovery TXT record CRA key.
*
*/
#define CHIP_CONFIG_MRP_DEFAULT_ACTIVE_RETRY_INTERVAL (2000)
#define CHIP_CONFIG_MRP_DEFAULT_ACTIVE_RETRY_INTERVAL (2000_ms32)
2 changes: 1 addition & 1 deletion examples/lock-app/efr32/include/CHIPProjectConfig.h
Original file line number Diff line number Diff line change
@@ -125,4 +125,4 @@
* needs (e.g. sleeping period) using Service Discovery TXT record CRA key.
*
*/
#define CHIP_CONFIG_MRP_DEFAULT_ACTIVE_RETRY_INTERVAL (2000)
#define CHIP_CONFIG_MRP_DEFAULT_ACTIVE_RETRY_INTERVAL (2000_ms32)
Original file line number Diff line number Diff line change
@@ -38,4 +38,4 @@
#define CHIP_DEVICE_CONFIG_USE_TEST_SETUP_PIN_CODE 20202021
#define CHIP_DEVICE_CONFIG_USE_TEST_SETUP_DISCRIMINATOR 0xF00

#define CHIP_DEVICE_CONFIG_SED_SLOW_POLLING_INTERVAL 2000
#define CHIP_DEVICE_CONFIG_SED_SLOW_POLLING_INTERVAL 2000_ms32
4 changes: 2 additions & 2 deletions examples/lock-app/nxp/k32w/k32w0/include/CHIPProjectConfig.h
Original file line number Diff line number Diff line change
@@ -185,8 +185,8 @@
#define CHIP_CONFIG_MAX_DEVICE_ADMINS 4 // 3 fabrics + 1 for rotation slack

#define CHIP_DEVICE_CONFIG_ENABLE_SED 1
#define CHIP_DEVICE_CONFIG_SED_SLOW_POLLING_INTERVAL 1000
#define CHIP_DEVICE_CONFIG_SED_FAST_POLLING_INTERVAL 100
#define CHIP_DEVICE_CONFIG_SED_SLOW_POLLING_INTERVAL 1000_ms32
#define CHIP_DEVICE_CONFIG_SED_FAST_POLLING_INTERVAL 100_ms32

/**
* CHIP_CONFIG_EVENT_LOGGING_DEFAULT_IMPORTANCE
Original file line number Diff line number Diff line change
@@ -124,8 +124,8 @@
#define CHIP_DEVICE_CONFIG_ENABLE_THREAD_SRP_CLIENT 1

#define CHIP_DEVICE_CONFIG_ENABLE_SED 1
#define CHIP_DEVICE_CONFIG_SED_SLOW_POLLING_INTERVAL 5000
#define CHIP_DEVICE_CONFIG_SED_FAST_POLLING_INTERVAL 5000
#define CHIP_DEVICE_CONFIG_SED_SLOW_POLLING_INTERVAL 5000_ms32
#define CHIP_DEVICE_CONFIG_SED_FAST_POLLING_INTERVAL 5000_ms32

/**
* CHIP_CONFIG_EVENT_LOGGING_DEFAULT_IMPORTANCE
Original file line number Diff line number Diff line change
@@ -124,8 +124,8 @@
#define CHIP_DEVICE_CONFIG_ENABLE_THREAD_SRP_CLIENT 1

#define CHIP_DEVICE_CONFIG_ENABLE_SED 1
#define CHIP_DEVICE_CONFIG_SED_SLOW_POLLING_INTERVAL 5000
#define CHIP_DEVICE_CONFIG_SED_FAST_POLLING_INTERVAL 5000
#define CHIP_DEVICE_CONFIG_SED_SLOW_POLLING_INTERVAL 5000_ms32
#define CHIP_DEVICE_CONFIG_SED_FAST_POLLING_INTERVAL 5000_ms32

/**
* CHIP_CONFIG_EVENT_LOGGING_DEFAULT_IMPORTANCE
2 changes: 1 addition & 1 deletion examples/window-app/efr32/include/CHIPProjectConfig.h
Original file line number Diff line number Diff line change
@@ -165,4 +165,4 @@
* needs (e.g. sleeping period) using Service Discovery TXT record CRA key.
*
*/
#define CHIP_CONFIG_MRP_DEFAULT_ACTIVE_RETRY_INTERVAL (2000)
#define CHIP_CONFIG_MRP_DEFAULT_ACTIVE_RETRY_INTERVAL (2000_ms32)
1 change: 0 additions & 1 deletion src/app/OperationalDeviceProxy.cpp
Original file line number Diff line number Diff line change
@@ -120,7 +120,6 @@ CHIP_ERROR OperationalDeviceProxy::UpdateDeviceData(const Transport::PeerAddress
if (secureSession != nullptr)
{
secureSession->SetPeerAddress(addr);
secureSession->SetMRPConfig(mMRPConfig);
}
}

1 change: 0 additions & 1 deletion src/controller/CommissioneeDeviceProxy.cpp
Original file line number Diff line number Diff line change
@@ -134,7 +134,6 @@ CHIP_ERROR CommissioneeDeviceProxy::UpdateDeviceData(const Transport::PeerAddres

Transport::SecureSession * secureSession = mSessionManager->GetSecureSession(mSecureSession.Value());
secureSession->SetPeerAddress(addr);
secureSession->SetMRPConfig(config);

return CHIP_NO_ERROR;
}
Original file line number Diff line number Diff line change
@@ -100,15 +100,15 @@ void pychip_CommissionableNodeController_PrintDiscoveredCommissioners(
ChipLogProgress(Discovery, "\tPairing Hint\t\t%u", dnsSdInfo->pairingHint);
if (dnsSdInfo->GetMrpRetryIntervalIdle().HasValue())
{
ChipLogProgress(Discovery, "\tMrp Interval idle\t%u", dnsSdInfo->GetMrpRetryIntervalIdle().Value());
ChipLogProgress(Discovery, "\tMrp Interval idle\t%u", dnsSdInfo->GetMrpRetryIntervalIdle().Value().count());
}
else
{
ChipLogProgress(Discovery, "\tMrp Interval idle\tNot present");
}
if (dnsSdInfo->GetMrpRetryIntervalActive().HasValue())
{
ChipLogProgress(Discovery, "\tMrp Interval active\t%u", dnsSdInfo->GetMrpRetryIntervalActive().Value());
ChipLogProgress(Discovery, "\tMrp Interval active\t%u", dnsSdInfo->GetMrpRetryIntervalActive().Value().count());
}
else
{
4 changes: 2 additions & 2 deletions src/controller/python/ChipDeviceController-ScriptBinding.cpp
Original file line number Diff line number Diff line change
@@ -425,15 +425,15 @@ void pychip_DeviceController_PrintDiscoveredDevices(chip::Controller::DeviceComm
ChipLogProgress(Discovery, "\tPairing Hint\t\t%u", dnsSdInfo->pairingHint);
if (dnsSdInfo->GetMrpRetryIntervalIdle().HasValue())
{
ChipLogProgress(Discovery, "\tMrp Interval idle\t%u", dnsSdInfo->GetMrpRetryIntervalIdle().Value());
ChipLogProgress(Discovery, "\tMrp Interval idle\t%u", dnsSdInfo->GetMrpRetryIntervalIdle().Value().count());
}
else
{
ChipLogProgress(Discovery, "\tMrp Interval idle\tNot present");
}
if (dnsSdInfo->GetMrpRetryIntervalActive().HasValue())
{
ChipLogProgress(Discovery, "\tMrp Interval active\t%u", dnsSdInfo->GetMrpRetryIntervalActive().Value());
ChipLogProgress(Discovery, "\tMrp Interval active\t%u", dnsSdInfo->GetMrpRetryIntervalActive().Value().count());
}
else
{
4 changes: 2 additions & 2 deletions src/include/platform/CHIPDeviceConfig.h
Original file line number Diff line number Diff line change
@@ -132,7 +132,7 @@
* This interval is used by the device to periodically wake up and poll the data in the idle mode.
*/
#ifndef CHIP_DEVICE_CONFIG_SED_SLOW_POLLING_INTERVAL
#define CHIP_DEVICE_CONFIG_SED_SLOW_POLLING_INTERVAL 5000
#define CHIP_DEVICE_CONFIG_SED_SLOW_POLLING_INTERVAL 5000_ms32
#endif

/**
@@ -142,7 +142,7 @@
* This interval is used by the device to periodically wake up and poll the data in the active mode.
*/
#ifndef CHIP_DEVICE_CONFIG_SED_FAST_POLLING_INTERVAL
#define CHIP_DEVICE_CONFIG_SED_FAST_POLLING_INTERVAL 200
#define CHIP_DEVICE_CONFIG_SED_FAST_POLLING_INTERVAL 200_ms32
#endif

// -------------------- Device Identification Configuration --------------------
14 changes: 6 additions & 8 deletions src/include/platform/ConnectivityManager.h
Original file line number Diff line number Diff line change
@@ -275,15 +275,13 @@ class ConnectivityManager
*/
struct ConnectivityManager::SEDPollingConfig
{
uint32_t FastPollingIntervalMS; /**< Interval at which the device polls its parent
when there are active chip exchanges in progress. Only meaningful
when the device is acting as a sleepy end node. */
/** Interval at which the device polls its parent when there are active chip exchanges in progress. Only meaningful when the
* device is acting as a sleepy end node. */
System::Clock::Milliseconds32 FastPollingIntervalMS;

uint32_t SlowPollingIntervalMS; /**< Interval at which the device polls its parent
when there are NO active chip exchanges in progress. Only meaningful
when the device is acting as a sleepy end node. */

void Clear() { memset(this, 0, sizeof(*this)); }
/** Interval at which the device polls its parent when there are NO active chip exchanges in progress. Only meaningful when the
* device is acting as a sleepy end node. */
System::Clock::Milliseconds32 SlowPollingIntervalMS;
};

/**
23 changes: 9 additions & 14 deletions src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp
Original file line number Diff line number Diff line change
@@ -161,33 +161,28 @@ class AdvertiserMinMdns : public ServiceAdvertiser,
// TODO: Issue #5833 - MRP retry intervals should be updated on the poll period value change or device type change.
#if CHIP_DEVICE_CONFIG_ENABLE_SED
chip::DeviceLayer::ConnectivityManager::SEDPollingConfig sedPollingConfig;
sedPollingConfig.Clear();
ReturnErrorOnFailure(chip::DeviceLayer::ConnectivityMgr().GetSEDPollingConfig(sedPollingConfig));
// Increment default MRP retry intervals by SED poll period to be on the safe side
// and avoid unnecessary retransmissions.
if (optionalMrp.HasValue())
{
auto mrp = optionalMrp.Value();
optionalMrp.SetValue(ReliableMessageProtocolConfig(
mrp.mIdleRetransTimeoutTick +
(sedPollingConfig.SlowPollingIntervalMS >> CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT),
mrp.mActiveRetransTimeoutTick +
(sedPollingConfig.FastPollingIntervalMS >> CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT)));
optionalMrp.SetValue(ReliableMessageProtocolConfig(mrp.mIdleRetransTimeout + sedPollingConfig.SlowPollingIntervalMS,
mrp.mActiveRetransTimeout + sedPollingConfig.FastPollingIntervalMS));
}
#endif
if (optionalMrp.HasValue())
{
auto mrp = optionalMrp.Value();
{
if ((mrp.mIdleRetransTimeoutTick << CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT) > kMaxRetryInterval)
if (mrp.mIdleRetransTimeout > kMaxRetryInterval)
{
ChipLogProgress(Discovery,
"MRP retry interval idle value exceeds allowed range of 1 hour, using maximum available");
mrp.mIdleRetransTimeoutTick = kMaxRetryInterval >> CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT;
mrp.mIdleRetransTimeout = kMaxRetryInterval;
}
size_t writtenCharactersNumber =
snprintf(storage.mrpRetryIntervalIdleBuf, sizeof(storage.mrpRetryIntervalIdleBuf), "CRI=%" PRIu32,
mrp.mIdleRetransTimeoutTick << CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT);
size_t writtenCharactersNumber = snprintf(storage.mrpRetryIntervalIdleBuf, sizeof(storage.mrpRetryIntervalIdleBuf),
"CRI=%" PRIu32, mrp.mIdleRetransTimeout.count());
VerifyOrReturnError((writtenCharactersNumber > 0) &&
(writtenCharactersNumber < sizeof(storage.mrpRetryIntervalIdleBuf)),
CHIP_ERROR_INVALID_STRING_LENGTH);
@@ -196,15 +191,15 @@ class AdvertiserMinMdns : public ServiceAdvertiser,
}

{
if ((mrp.mActiveRetransTimeoutTick << CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT) > kMaxRetryInterval)
if (mrp.mActiveRetransTimeout > kMaxRetryInterval)
{
ChipLogProgress(Discovery,
"MRP retry interval active value exceeds allowed range of 1 hour, using maximum available");
mrp.mActiveRetransTimeoutTick = kMaxRetryInterval >> CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT;
mrp.mActiveRetransTimeout = kMaxRetryInterval;
}
size_t writtenCharactersNumber =
snprintf(storage.mrpRetryIntervalActiveBuf, sizeof(storage.mrpRetryIntervalActiveBuf), "CRA=%" PRIu32,
mrp.mActiveRetransTimeoutTick << CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT);
mrp.mActiveRetransTimeout.count());
VerifyOrReturnError((writtenCharactersNumber > 0) &&
(writtenCharactersNumber < sizeof(storage.mrpRetryIntervalActiveBuf)),
CHIP_ERROR_INVALID_STRING_LENGTH);
23 changes: 10 additions & 13 deletions src/lib/dnssd/Discovery_ImplPlatform.cpp
Original file line number Diff line number Diff line change
@@ -136,45 +136,42 @@ CHIP_ERROR AddCommonTxtElements(const BaseAdvertisingParams<Derived> & params, c
// in with the correct values and set one level up from here.
#if CHIP_DEVICE_CONFIG_ENABLE_SED
chip::DeviceLayer::ConnectivityManager::SEDPollingConfig sedPollingConfig;
sedPollingConfig.Clear();
ReturnErrorOnFailure(chip::DeviceLayer::ConnectivityMgr().GetSEDPollingConfig(sedPollingConfig));
// Increment default MRP retry intervals by SED poll period to be on the safe side
// and avoid unnecessary retransmissions.
if (optionalMrp.HasValue())
{
auto mrp = optionalMrp.Value();
optionalMrp.SetValue(ReliableMessageProtocolConfig(
mrp.mIdleRetransTimeoutTick + (sedPollingConfig.SlowPollingIntervalMS >> CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT),
mrp.mActiveRetransTimeoutTick +
(sedPollingConfig.FastPollingIntervalMS >> CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT)));
optionalMrp.SetValue(ReliableMessageProtocolConfig(mrp.mIdleRetransTimeout + sedPollingConfig.SlowPollingIntervalMS,
mrp.mActiveRetransTimeout + sedPollingConfig.FastPollingIntervalMS));
}
#endif
if (optionalMrp.HasValue())
{
auto mrp = optionalMrp.Value();
{
if ((mrp.mIdleRetransTimeoutTick << CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT) > kMaxRetryInterval)
if (mrp.mIdleRetransTimeout > kMaxRetryInterval)
{
ChipLogProgress(Discovery,
"MRP retry interval idle value exceeds allowed range of 1 hour, using maximum available");
mrp.mIdleRetransTimeoutTick = kMaxRetryInterval >> CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT;
mrp.mIdleRetransTimeout = kMaxRetryInterval;
}
size_t writtenCharactersNumber = snprintf(mrpRetryIdleStorage, sizeof(mrpRetryIdleStorage), "%" PRIu32,
mrp.mIdleRetransTimeoutTick << CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT);
size_t writtenCharactersNumber =
snprintf(mrpRetryIdleStorage, sizeof(mrpRetryIdleStorage), "%" PRIu32, mrp.mIdleRetransTimeout.count());
VerifyOrReturnError((writtenCharactersNumber > 0) && (writtenCharactersNumber <= kTxtRetryIntervalIdleMaxLength),
CHIP_ERROR_INVALID_STRING_LENGTH);
txtEntryStorage[txtEntryIdx++] = { "CRI", Uint8::from_const_char(mrpRetryIdleStorage), strlen(mrpRetryIdleStorage) };
}

{
if ((mrp.mActiveRetransTimeoutTick << CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT) > kMaxRetryInterval)
if (mrp.mActiveRetransTimeout > kMaxRetryInterval)
{
ChipLogProgress(Discovery,
"MRP retry interval active value exceeds allowed range of 1 hour, using maximum available");
mrp.mActiveRetransTimeoutTick = kMaxRetryInterval >> CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT;
mrp.mActiveRetransTimeout = kMaxRetryInterval;
}
size_t writtenCharactersNumber = snprintf(mrpRetryActiveStorage, sizeof(mrpRetryActiveStorage), "%" PRIu32,
mrp.mActiveRetransTimeoutTick << CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT);
size_t writtenCharactersNumber =
snprintf(mrpRetryActiveStorage, sizeof(mrpRetryActiveStorage), "%" PRIu32, mrp.mActiveRetransTimeout.count());
VerifyOrReturnError((writtenCharactersNumber > 0) && (writtenCharactersNumber <= kTxtRetryIntervalActiveMaxLength),
CHIP_ERROR_INVALID_STRING_LENGTH);
txtEntryStorage[txtEntryIdx++] = { "CRA", Uint8::from_const_char(mrpRetryActiveStorage),
24 changes: 10 additions & 14 deletions src/lib/dnssd/Resolver.h
Original file line number Diff line number Diff line change
@@ -56,15 +56,11 @@ struct ResolvedNodeData

ReliableMessageProtocolConfig GetMRPConfig() const
{
return ReliableMessageProtocolConfig(GetMrpRetryIntervalIdle().ValueOr(gDefaultMRPConfig.mIdleRetransTimeoutTick
<< CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT) >>
CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT,
GetMrpRetryIntervalActive().ValueOr(gDefaultMRPConfig.mActiveRetransTimeoutTick
<< CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT) >>
CHIP_CONFIG_RMP_TIMER_DEFAULT_PERIOD_SHIFT);
return ReliableMessageProtocolConfig(GetMrpRetryIntervalIdle().ValueOr(gDefaultMRPConfig.mIdleRetransTimeout),
GetMrpRetryIntervalActive().ValueOr(gDefaultMRPConfig.mActiveRetransTimeout));
}
Optional<uint32_t> GetMrpRetryIntervalIdle() const { return mMrpRetryIntervalIdle; }
Optional<uint32_t> GetMrpRetryIntervalActive() const { return mMrpRetryIntervalActive; }
Optional<System::Clock::Milliseconds32> GetMrpRetryIntervalIdle() const { return mMrpRetryIntervalIdle; }
Optional<System::Clock::Milliseconds32> GetMrpRetryIntervalActive() const { return mMrpRetryIntervalActive; }

PeerId mPeerId;
size_t mNumIPs = 0;
@@ -73,8 +69,8 @@ struct ResolvedNodeData
uint16_t mPort = 0;
char mHostName[kHostNameMaxLength + 1] = {};
bool mSupportsTcp = false;
Optional<uint32_t> mMrpRetryIntervalIdle;
Optional<uint32_t> mMrpRetryIntervalActive;
Optional<System::Clock::Milliseconds32> mMrpRetryIntervalIdle;
Optional<System::Clock::Milliseconds32> mMrpRetryIntervalActive;
System::Clock::Timestamp mExpiryTime;
};

@@ -100,8 +96,8 @@ struct DiscoveredNodeData
uint16_t pairingHint;
char pairingInstruction[kMaxPairingInstructionLen + 1];
bool supportsTcp;
Optional<uint32_t> mrpRetryIntervalIdle;
Optional<uint32_t> mrpRetryIntervalActive;
Optional<System::Clock::Milliseconds32> mrpRetryIntervalIdle;
Optional<System::Clock::Milliseconds32> mrpRetryIntervalActive;
uint16_t port;
int numIPs;
Inet::InterfaceId interfaceId[kMaxIPAddresses];
@@ -135,8 +131,8 @@ struct DiscoveredNodeData
bool IsHost(const char * host) const { return strcmp(host, hostName) == 0; }
bool IsInstanceName(const char * instance) const { return strcmp(instance, instanceName) == 0; }
bool IsValid() const { return !IsHost("") && ipAddress[0] != chip::Inet::IPAddress::Any; }
Optional<uint32_t> GetMrpRetryIntervalIdle() const { return mrpRetryIntervalIdle; }
Optional<uint32_t> GetMrpRetryIntervalActive() const { return mrpRetryIntervalActive; }
Optional<System::Clock::Milliseconds32> GetMrpRetryIntervalIdle() const { return mrpRetryIntervalIdle; }
Optional<System::Clock::Milliseconds32> GetMrpRetryIntervalActive() const { return mrpRetryIntervalActive; }

void LogDetail() const
{
Loading

0 comments on commit 4936579

Please sign in to comment.