-
Notifications
You must be signed in to change notification settings - Fork 8.1k
net: conn_mgr_connectivity: idle/inactivity timeout #91733
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
net: conn_mgr_connectivity: idle/inactivity timeout #91733
Conversation
997159c
to
476b8f7
Compare
|
Update zephyr fork with Connectivity Manager idle timeout support. Upstream PR: zephyrproject-rtos/zephyr#91733 Signed-off-by: Jordan Yates <[email protected]>
Update zephyr fork with Connectivity Manager idle timeout support. Upstream PR: zephyrproject-rtos/zephyr#91733 Signed-off-by: Jordan Yates <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything is super neat, although I have one concern about the inactivity timer placement.
drivers/net/nsos_sockets.c
Outdated
} | ||
} | ||
|
||
static void nsos_idle_fn(struct k_work *work) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, so from the issue description and the new documentation I had an impression that this idle timeout handling would be a generic implementation in the conn_mgr, and not handled individually by each binding implementation?
So that when the timer for particular binding expires, the conn_mgr would call conn_mgr_if_disconnect()
on the interface. This way the feature would practically become avaialble for each binding implementation for free? That would also put the need of having used()
API in struct conn_mgr_conn_api
at question.
Or do you forsee there may be some differences between implementations and it is actually beneficial to have the incactivity timer to be implemented by each binding individually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that was the original plan, but it ran up against some pretty immediate roadblocks due to the design of the Connectivity Manager. Because it delegates all management of connections to the bindings, it has no knowledge of when a connection actually succeeds or disconnects. Since those are the two events needed to implement the idle timeout (to start and terminate it), it poses a problem.
So it's either add new callbacks from bindings back to the connectivity manager so that it can handle the idle timeouts, or just handle the timeout in the binding directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it delegates all management of connections to the bindings, it has no knowledge of when a connection actually succeeds or disconnects. Since those are the two events needed to implement the idle timeout (to start and terminate it), it poses a problem.
What about generic NET_EVENT_IF_UP
/NET_EVENT_IF_DOWN
events? The interface should only go operational UP after it's connected, and when it loses connectivity it goes down. Couldn't we use those for a common implementation of the idle timeout feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
Add a helper macro to declare a network interface, enabling the use of `NET_IF_GET` in the driver before the call to `NET_DEVICE_OFFLOAD_INIT` or `NET_DEVICE_INIT`. Signed-off-by: Jordan Yates <[email protected]>
476b8f7
to
f5b72d9
Compare
f5b72d9
to
6b5afbc
Compare
Updated implementation from @rlubos suggestions, moving the core functionality inside the connection manager. Connectivity bindings no longer need any updates, interfaces just need to call |
6f18ea4
to
3820224
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, couple of minor nits about pointer checks.
Replace all `!binding` checks with `binding == NULL` for MISRA compliance. Signed-off-by: Jordan Yates <[email protected]>
Add an interface idle timeout parameter to the connectivity binding structure. This will be used to track idle timeouts for interfaces. Signed-off-by: Jordan Yates <[email protected]>
3820224
to
8290dd3
Compare
Added an initial commit to switch all the existing usage of |
8290dd3
to
b69cc51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might need some release note bits. Looks good!
Add tests for the idle timeout parameter. Signed-off-by: Jordan Yates <[email protected]>
Implement idle timeouts, primarily in the common connectivity library, with individual interfaces notifying the library when the interface has been used. Signed-off-by: Jordan Yates <[email protected]>
Add interface usage notifications to the `NET_NATIVE` code paths. Signed-off-by: Jordan Yates <[email protected]>
Add interface usage notifications to the native socket offload code paths. Signed-off-by: Jordan Yates <[email protected]>
Test the behaviour of the idle timeout on native sockets. Only tests a subset of the "active" paths to not require any external services. Signed-off-by: Jordan Yates <[email protected]>
Add documentation for the new idle timeout feature of the connection manager. Signed-off-by: Jordan Yates <[email protected]>
b69cc51
to
d413b6e
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
Add the option for users to specify idle timeouts on an interface, which results in a disconnect event if N seconds pass without any activity on the interface.
Proper operation of this feature requires two changes to existing bindings:
used
callback to receive usage notifications (to refresh the idle timeout)conn_mgr_if_used
calls to the underlying networking driverExisting bindings do not break with this addition, but the idle timeout won't work without updates.
Adding the new
conn_mgr_if_used
function call appeared to be the best way to notify the connection manager of interface activity, rather than trying to patch into the net_stats module.The second requirement has been done for the native socket abstraction and
NET_NATIVE
(which include nRF7x).The implementation has been tested on an OOT WiFi connectivity manager implementation.
The following are example logs with a 10 second idle timeout, with no activity occuring and persistence enabled.
Implements #90198