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.

Sign up for GitHub

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

[ICD] Initial Impl Active Threshold #24394

Merged
merged 2 commits into from
Jan 13, 2023
Merged

[ICD] Initial Impl Active Threshold #24394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c039b2
Initial Impl Active Threshold
jepenven-silabs Jan 12, 2023
dc8ccce
Apply comments
jepenven-silabs Jan 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/include/platform/CHIPDeviceConfig.h
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,16 @@
#define CHIP_DEVICE_CONFIG_SED_ACTIVE_INTERVAL 200_ms32
#endif

/**
* CHIP_DEVICE_CONFIG_SED_ACTIVE_THRESHOLD
*
* Minimum amount the node SHOULD stay awake after network activity.
* Spec section 2.12.5
*/
#ifndef CHIP_DEVICE_CONFIG_SED_ACTIVE_THRESHOLD
#define CHIP_DEVICE_CONFIG_SED_ACTIVE_THRESHOLD System::Clock::Milliseconds32(4000)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this wasn't 4000_ms32 like other things in this file?

#endif

// -------------------- Device Identification Configuration --------------------

/**
Expand Down
6 changes: 3 additions & 3 deletions src/include/platform/ConnectivityManager.h
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ class ConnectivityManager
*
* @param[in] onOff true if active mode should be enabled and false otherwise.
*/
CHIP_ERROR RequestSEDActiveMode(bool onOff);
CHIP_ERROR RequestSEDActiveMode(bool onOff, bool delayIdle = false);
#endif

// CHIPoBLE service methods
Expand Down Expand Up @@ -480,9 +480,9 @@ inline CHIP_ERROR ConnectivityManager::SetSEDIntervalsConfig(const SEDIntervalsC
return static_cast<ImplClass *>(this)->_SetSEDIntervalsConfig(intervalsConfig);
}

inline CHIP_ERROR ConnectivityManager::RequestSEDActiveMode(bool onOff)
inline CHIP_ERROR ConnectivityManager::RequestSEDActiveMode(bool onOff, bool delayIdle)
{
return static_cast<ImplClass *>(this)->_RequestSEDActiveMode(onOff);
return static_cast<ImplClass *>(this)->_RequestSEDActiveMode(onOff, delayIdle);
}
#endif

Expand Down
6 changes: 3 additions & 3 deletions src/include/platform/ThreadStackManager.h
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ class ThreadStackManager
*
* @param[in] onOff true if active mode should be enabled and false otherwise.
*/
CHIP_ERROR RequestSEDActiveMode(bool onOff);
CHIP_ERROR RequestSEDActiveMode(bool onOff, bool delayIdle = false);
#endif

bool HaveMeshConnectivity();
Expand Down Expand Up @@ -402,9 +402,9 @@ inline CHIP_ERROR ThreadStackManager::SetSEDIntervalsConfig(const ConnectivityMa
return static_cast<ImplClass *>(this)->_SetSEDIntervalsConfig(intervalsConfig);
}

inline CHIP_ERROR ThreadStackManager::RequestSEDActiveMode(bool onOff)
inline CHIP_ERROR ThreadStackManager::RequestSEDActiveMode(bool onOff, bool delayIdle)
{
return static_cast<ImplClass *>(this)->_RequestSEDActiveMode(onOff);
return static_cast<ImplClass *>(this)->_RequestSEDActiveMode(onOff, delayIdle);
}
#endif

Expand Down
4 changes: 2 additions & 2 deletions src/include/platform/internal/GenericConnectivityManagerImpl_NoThread.h
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class GenericConnectivityManagerImpl_NoThread
CHIP_ERROR _SetThreadDeviceType(ConnectivityManager::ThreadDeviceType deviceType);
CHIP_ERROR _GetSEDIntervalsConfig(ConnectivityManager::SEDIntervalsConfig & intervalsConfig);
CHIP_ERROR _SetSEDIntervalsConfig(const ConnectivityManager::SEDIntervalsConfig & intervalsConfig);
CHIP_ERROR _RequestSEDActiveMode(bool onOff);
CHIP_ERROR _RequestSEDActiveMode(bool onOff, bool delayIdle = false);
bool _IsThreadAttached(void);
bool _IsThreadProvisioned(void);
void _ErasePersistentInfo(void);
Expand Down Expand Up @@ -130,7 +130,7 @@ inline CHIP_ERROR GenericConnectivityManagerImpl_NoThread<ImplClass>::_SetSEDInt
}

template <class ImplClass>
inline CHIP_ERROR GenericConnectivityManagerImpl_NoThread<ImplClass>::_RequestSEDActiveMode(bool onOff)
inline CHIP_ERROR GenericConnectivityManagerImpl_NoThread<ImplClass>::_RequestSEDActiveMode(bool onOff, bool delayIdle)
{
return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE;
}
Expand Down
6 changes: 3 additions & 3 deletions src/include/platform/internal/GenericConnectivityManagerImpl_Thread.h
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ class GenericConnectivityManagerImpl_Thread
#if CHIP_DEVICE_CONFIG_ENABLE_SED
CHIP_ERROR _GetSEDIntervalsConfig(ConnectivityManager::SEDIntervalsConfig & intervalsConfig);
CHIP_ERROR _SetSEDIntervalsConfig(const ConnectivityManager::SEDIntervalsConfig & intervalsConfig);
CHIP_ERROR _RequestSEDActiveMode(bool onOff);
CHIP_ERROR _RequestSEDActiveMode(bool onOff, bool delayIdle = false);
#endif
bool _IsThreadAttached();
bool _IsThreadProvisioned();
Expand Down Expand Up @@ -157,9 +157,9 @@ inline CHIP_ERROR GenericConnectivityManagerImpl_Thread<ImplClass>::_SetSEDInter
}

template <class ImplClass>
inline CHIP_ERROR GenericConnectivityManagerImpl_Thread<ImplClass>::_RequestSEDActiveMode(bool onOff)
inline CHIP_ERROR GenericConnectivityManagerImpl_Thread<ImplClass>::_RequestSEDActiveMode(bool onOff, bool delayIdle)
{
return ThreadStackMgrImpl().RequestSEDActiveMode(onOff);
return ThreadStackMgrImpl().RequestSEDActiveMode(onOff, delayIdle);
}
#endif

Expand Down
2 changes: 1 addition & 1 deletion src/messaging/ExchangeContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ void ExchangeContext::UpdateSEDIntervalMode(bool activeMode)
if (activeMode != IsRequestingActiveMode())
{
SetRequestingActiveMode(activeMode);
DeviceLayer::ConnectivityMgr().RequestSEDActiveMode(activeMode);
DeviceLayer::ConnectivityMgr().RequestSEDActiveMode(activeMode, true);
}
}
#endif
Expand Down
3 changes: 2 additions & 1 deletion src/platform/Linux/ThreadStackManagerImpl.cpp
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,9 +496,10 @@ CHIP_ERROR ThreadStackManagerImpl::_SetSEDIntervalsConfig(const ConnectivityMana
return CHIP_ERROR_NOT_IMPLEMENTED;
}

CHIP_ERROR ThreadStackManagerImpl::_RequestSEDActiveMode(bool onOff)
CHIP_ERROR ThreadStackManagerImpl::_RequestSEDActiveMode(bool onOff, bool delayIdle)
{
(void) onOff;
(void) delayIdle;

ChipLogError(DeviceLayer, "SED intervals config is not supported on linux");
return CHIP_ERROR_NOT_IMPLEMENTED;
Expand Down
2 changes: 1 addition & 1 deletion src/platform/Linux/ThreadStackManagerImpl.h
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ class ThreadStackManagerImpl : public ThreadStackManager
#if CHIP_DEVICE_CONFIG_ENABLE_SED
CHIP_ERROR _GetSEDIntervalsConfig(ConnectivityManager::SEDIntervalsConfig & intervalsConfig);
CHIP_ERROR _SetSEDIntervalsConfig(const ConnectivityManager::SEDIntervalsConfig & intervalsConfig);
CHIP_ERROR _RequestSEDActiveMode(bool onOff);
CHIP_ERROR _RequestSEDActiveMode(bool onOff, bool delayIdle = false);
#endif

bool _HaveMeshConnectivity();
Expand Down
36 changes: 33 additions & 3 deletions src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1836,10 +1836,11 @@ GenericThreadStackManagerImpl_OpenThread<ImplClass>::SetSEDIntervalMode(Connecti
{
return CHIP_ERROR_INVALID_ARGUMENT;
}
mIntervalsMode = intervalType;

Impl()->LockThreadStack();

mIntervalsMode = intervalType;

// For Thread devices, the intervals are defined as:
// * poll period for SED devices that poll the parent for data
// * CSL period for SSED devices that listen for messages in scheduled time slots.
Expand Down Expand Up @@ -1879,10 +1880,9 @@ GenericThreadStackManagerImpl_OpenThread<ImplClass>::SetSEDIntervalMode(Connecti
}

template <class ImplClass>
CHIP_ERROR GenericThreadStackManagerImpl_OpenThread<ImplClass>::_RequestSEDActiveMode(bool onOff)
CHIP_ERROR GenericThreadStackManagerImpl_OpenThread<ImplClass>::_RequestSEDActiveMode(bool onOff, bool delayIdle)
jepenven-silabs marked this conversation as resolved.
Show resolved Hide resolved
{
CHIP_ERROR err = CHIP_NO_ERROR;
ConnectivityManager::SEDIntervalMode mode;

if (onOff)
{
Expand All @@ -1894,13 +1894,43 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread<ImplClass>::_RequestSEDActiv
mActiveModeConsumers--;
}

if (!onOff && delayIdle && CHIP_DEVICE_CONFIG_SED_ACTIVE_THRESHOLD.count() != 0)
{
err = DeviceLayer::SystemLayer().StartTimer(CHIP_DEVICE_CONFIG_SED_ACTIVE_THRESHOLD, RequestSEDModeChange, this);
if (CHIP_NO_ERROR == err)
{
return err;
}

ChipLogError(DeviceLayer, "Failed to postponed Idle Mode with error %" CHIP_ERROR_FORMAT, err.Format());
jepenven-silabs marked this conversation as resolved.
Show resolved Hide resolved
}

return SEDChangeMode();
}

template <class ImplClass>
CHIP_ERROR GenericThreadStackManagerImpl_OpenThread<ImplClass>::SEDChangeMode()
jepenven-silabs marked this conversation as resolved.
Show resolved Hide resolved
{
CHIP_ERROR err = CHIP_NO_ERROR;
ConnectivityManager::SEDIntervalMode mode;

mode = mActiveModeConsumers > 0 ? ConnectivityManager::SEDIntervalMode::Active : ConnectivityManager::SEDIntervalMode::Idle;

if (mIntervalsMode != mode)
err = SetSEDIntervalMode(mode);

return err;
}

template <class ImplClass>
void GenericThreadStackManagerImpl_OpenThread<ImplClass>::RequestSEDModeChange(chip::System::Layer * apSystemLayer,
jepenven-silabs marked this conversation as resolved.
Show resolved Hide resolved
void * apAppState)
{
if (apAppState != nullptr)
{
static_cast<GenericThreadStackManagerImpl_OpenThread *>(apAppState)->SEDChangeMode();
Copy link
Contributor

@bzbarsky-apple bzbarsky-apple Jan 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong to me.

Let's say we go into active mode for an exchange, and then the exchange completes. We will start a timer to call this function.

If, before the timer fires, we enter active mode for some reason other than an exchange and then exit active mode, we will exit immediately, without waiting for this timer to fire.

What I think we need to do is that the timer starting successfully, if there was no timer so far, should count as an active mode consumer (and increase mActiveModeConsumers by 1). When the timer fires it should then decrement mActiveModeConsumers by 1.

Or something else to deal with this problem.

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 It's not a problem as per the spec. section 2.12.5 specify SHOULD stay active, the case you've described is a possible corner case that doesn't go against this implementation. However this would have to be address when the Client Monitoring cluster will fully be implemented.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior is allowed by the spec, but it's still not the intended behavior of this implementation as far as I can tell...

}
}
#endif

template <class ImplClass>
Expand Down
4 changes: 3 additions & 1 deletion src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,9 @@ class GenericThreadStackManagerImpl_OpenThread
#if CHIP_DEVICE_CONFIG_ENABLE_SED
CHIP_ERROR _GetSEDIntervalsConfig(ConnectivityManager::SEDIntervalsConfig & intervalsConfig);
CHIP_ERROR _SetSEDIntervalsConfig(const ConnectivityManager::SEDIntervalsConfig & intervalsConfig);
CHIP_ERROR _RequestSEDActiveMode(bool onOff);
CHIP_ERROR _RequestSEDActiveMode(bool onOff, bool delayIdle);
CHIP_ERROR SEDChangeMode(void);
static void RequestSEDModeChange(chip::System::Layer * apSystemLayer, void * apAppState);
#endif

bool _HaveMeshConnectivity(void);
Expand Down