-
Notifications
You must be signed in to change notification settings - Fork 165
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
open discussion - m2kplugin: Close all libm2k contexts when disconnecting the M2K from Scopy. #1553
Conversation
…Scopy. This change is needed for proper compatibility with libm2k v0.8.0 which introduced the concept of reference count for all open contexts together with handling context ownership (iio_context created outside libm2k or by libm2k). For libm2k to cleanup all the context refs, every m2kOpen call should be matched by a closeContext call. Signed-off-by: AlexandraTrifan <[email protected]>
Call m2kOpen once in the M2kPlugin and pass the M2k* (libm2k context object) to all the objects needing it. This seems a good option does it have any way of causing a negative impact ? |
Another idea that we could explore would be to make the M2K object use the RAII concept in c++. If we do that, the client would not be responsible for the number of calls to m2kOpen they do. We could introduce the ContextPtr/M2kPtr which references the Context/M2k obj (similar to a sharedptr) - and the Context/M2k would have a ref count. Each ContextPtr object would have a lifecycle, it would let the libm2k ContextBuilder(parent) know that it was created when constructed (by m2kOpen or a copy constructor) and when the destructor is called. The ContextBuilder would keep track of all Contexts and all CtxPtr(one-to-many). When all the CtxPtrs destructors are called, the ContextBuilder is notified to destroy the Context. Rust has implemented this (mandatory for all objects) to ensure that there are no dangling pointers left and the user does not have to keep track of all of them. Furthermore, if libm2k keeps track of all Context objects created, it can also apply a force close on all of them (if needed). CtxBuilder: {vector<Context*> m_ctxs; vector m_ctxs_ptrs;}; m2kOpen(){ctx=getCtxOrConstruct(); new CtxPtr(ctx);} |
I like Andrei's idea. I consider it essential to minimize the client's responsibilities whenever feasible, and this solution accomplishes just that. It seems to be the cleanest and most efficient method to solve this problem. |
…Scopy Fix the following scenario in the m2kplugin using libm2kv0.8.0: connect, disconnect, connect again - fails due to improper libm2k context deletion. See PR #1553 for more details on this issue. Signed-off-by: AlexandraTrifan <[email protected]>
Fix the following scenario in Scopy using libm2kv0.8.0: connect, disconnect, connect again - fails due to improper libm2k context deletion. See PR #1553 for more details on this issue. Signed-off-by: AlexandraTrifan <[email protected]>
Fix the following scenario in Scopy using libm2kv0.8.0: connect, disconnect, connect again - fails due to improper libm2k context deletion. See PR #1553 for more details on this issue. Signed-off-by: AlexandraTrifan <[email protected]>
…Scopy Fix the following scenario in the m2kplugin using libm2kv0.8.0: connect, disconnect, connect again - fails due to improper libm2k context deletion. See PR #1553 for more details on this issue. Signed-off-by: AlexandraTrifan <[email protected]>
This is related to the libm2k 0.9.0 context ownership changes. This was better described in the following pull request: #1553 All m2kOpen calls are removed and the only one opening and closing libm2k contexts is the M2kPlugin object. Signed-off-by: AlexandraTrifan <[email protected]>
This is related to the libm2k 0.9.0 context ownership changes. This was better described in the following pull request: #1553 All m2kOpen calls are removed and the only one opening and closing libm2k contexts is the M2kPlugin object. Signed-off-by: AlexandraTrifan <[email protected]>
This is related to the libm2k 0.9.0 context ownership changes. This was better described in the following pull request: #1553 All m2kOpen calls are removed and the only one opening and closing libm2k contexts is the M2kPlugin object. Signed-off-by: AlexandraTrifan <[email protected]>
…Scopy Fix the following scenario in the m2kplugin using libm2kv0.8.0: connect, disconnect, connect again - fails due to improper libm2k context deletion. See PR #1553 for more details on this issue. Signed-off-by: AlexandraTrifan <[email protected]>
This is related to the libm2k 0.9.0 context ownership changes. This was better described in the following pull request: #1553 All m2kOpen calls are removed and the only one opening and closing libm2k contexts is the M2kPlugin object. Signed-off-by: AlexandraTrifan <[email protected]>
…Scopy Fix the following scenario in the m2kplugin using libm2kv0.8.0: connect, disconnect, connect again - fails due to improper libm2k context deletion. See PR #1553 for more details on this issue. Signed-off-by: AlexandraTrifan <[email protected]>
This is related to the libm2k 0.9.0 context ownership changes. This was better described in the following pull request: #1553 All m2kOpen calls are removed and the only one opening and closing libm2k contexts is the M2kPlugin object. Signed-off-by: AlexandraTrifan <[email protected]>
…Scopy Fix the following scenario in the m2kplugin using libm2kv0.8.0: connect, disconnect, connect again - fails due to improper libm2k context deletion. See PR #1553 for more details on this issue. Signed-off-by: AlexandraTrifan <[email protected]>
This is related to the libm2k 0.9.0 context ownership changes. This was better described in the following pull request: #1553 All m2kOpen calls are removed and the only one opening and closing libm2k contexts is the M2kPlugin object. Signed-off-by: AlexandraTrifan <[email protected]>
PR needs more discussions on the subject:
This change is needed for proper compatibility with libm2k v0.8.0 which introduced the concept of reference count for all open contexts together with handling context ownership (iio_context created outside libm2k or by libm2k). For libm2k to cleanup all the context refs, every m2kOpen call should be matched by a closeContext call.
The libm2k change ( check PR analogdevicesinc/libm2k#344 ) was motivated by the bug that prevented clients who had ownership over the iio_context to properly destroy it because libm2k was assuming ownership.
Now for every m2kOpen we should have a contextClose call to cleanup the ref count. The issue here is:
Up to libm2k v0.7.0 there wasn't an issue with NOT providing the uri for m2kOpen with a previously created iio_context because it didn't actually depend on it. Now the reference count and open/close context actions depend on it.
The contextCloseAll() s not necessarily a good choice here since it would close all device contexts existing at that time.
Possible ways to fix this: