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.

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

Reset data inputs pointers in Adapter before interface is destroyed #301

Merged
merged 1 commit into from
Jan 18, 2025

Conversation

cochicde
Copy link
Contributor

@cochicde cochicde commented Jan 11, 2025

Starting the PR mostly to see if the solution is the best.

The context is the following:

  • 2 FB with an compatible adapter each
  • When the Adapter is created, CGenFunctionBlock<T>::setupFBInterface does its Voldermort-level-memory-magic, storing the data inputs and data outupts into member variables mDIs and mDOs
  • Whent the connecction between the adapters is created, mDIs = paPeer->mDOs; is done in each FB, meaning each FB has the pointer from the other, which is reseted to mLocalDIs when disconnected.

Two problems:

  1. mLocalDIs is set with mDIs in the constructor, but mDIs is set only afterwards in initialize, therefore this PE contrins the change in this initialize to store the updated value of mDIs
  2. disconnect (which resets mDIs with mLocalDIs) is called after the destructor of the Adapter, specifically from CAdapterConnection::performDisconnect in its destructor. That means whent he destructor of the Adapter is called, mDIs still has the mDOs value of the connected FB, and when the interface is destroyed in CGenFunctionBlock<T>::freeFBInterfaceData really bad things happen.
    I tried calling disconnect in the destructor of the Adapter, but the call later to performDisconnect is not happy (probably because the FB was destroyed alreayd, but not sure)

With the changes in the first commit of the PR, I don't get a crash. I'm surprised this was not detected before, so I'm not sure I'm doing something wrong. I used the ReferenceExamples from the Examples repository. There Ex2a_05_Adapter and Ex3a_05_Adapter have this issue. The particular case I see is that the Adapter has both Data Inputs and Data Ouputs, which is not the case for example of the ATimeout, but I didn't test too deep to see if this affects the context in which the crash occurs.

In my opinion, the adapter should perform the disconnection in its destructor, and the CAdapterConnection::performDisconnect should not exist

@cochicde cochicde requested a review from azoitl January 11, 2025 15:08
@cochicde cochicde changed the title reset data inputs before cleaning Reset data inputs pointers in Adapter before interface is destroyed Jan 11, 2025
@azoitl
Copy link
Contributor

azoitl commented Jan 12, 2025

As you bring up this point I'm not so shure if we should mirror the input and output arrays between plugs and sockets. Should we instead copy them over to have a stronger decoupling of both sides and a more correct with behavior, because now if an FB writes to an adapter it is immediately visible on the other side, and this would be wrong.

@mx990
Copy link
Member

mx990 commented Jan 13, 2025

Yes, this is something I wanted to fix for a long time but never got around to actually doing. I would also split adapters into separate classes for plugs and sockets. We could then add the required interface (and connection) variables directly in each class (as we do for regular FBs) and also perform proper sampling for events.

@cochicde
Copy link
Contributor Author

cochicde commented Jan 17, 2025

I don't know much about adaptaers or how they should behave. From the code perspective, it seems that the connection is little bit hacky. Separating into plug and sockets classes seems to be a nice solution.

Now, what do we do until then? As it's now at least for me forte crashes, so technically it doesn't support adapters.

I'd recommend merge this as a quick fix to have it working and open an issue about this issue to proper work on it.

@azoitl
Copy link
Contributor

azoitl commented Jan 18, 2025

You have a point here. Unless you would like to do it I don't see that happen soon. Will merge it therefore.

@azoitl azoitl merged commit 000d5c2 into eclipse-4diac:develop Jan 18, 2025
6 checks passed
@cochicde cochicde deleted the fix-adapter-crash branch January 19, 2025 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants