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

Add Large Forward Open service #303

Merged
merged 4 commits into from
May 27, 2020
Merged

Add Large Forward Open service #303

merged 4 commits into from
May 27, 2020

Conversation

nefethael
Copy link

@nefethael nefethael commented May 25, 2020

Signed-off-by: Vincent Prince vincent.prince.fr@gmail.com

Signed-off-by: Vincent Prince <vincent.prince.fr@gmail.com>
@nefethael
Copy link
Author

Hi All,

This is my first contribution to OpENer, I'm not sure my PR takes care of everything (examples, doc, etc)?

Also, I only tried LFO feature with EIPScanner stack and wireshark (I don't own any CIP scanner equipment), so I'm not 100% confident about implementation.

Finally, I had to increase PC_OPENER_ETHERNET_BUFFER_SIZE value to fix buffer overflow here , nevertheless I'm not sure I did the right fix here.

Kind regards,
Vincent

@MartinMelikMerkumians
Copy link
Member

Hi @nefethael ,
thanks for providing the Large Forward Open. I didn't have the time to properly check it, and the need for the large buffer is a little strange to me, but your contribution will be added as soon as I have everything checked.

Cheers,
Martin

@MartinMelikMerkumians
Copy link
Member

Hi @nefethael,
first tests show, that this patch breaks the current ForwardOpen. Reply codes and reply lengths are wrong.

Best regards,
Martin

@MartinMelikMerkumians
Copy link
Member

@nefethael already found the reason. You are only setting is_large_forward_open when calling LFO service. It is never reseted, and as a global variable is used, the flag stays true after the first LFO call.

@MartinMelikMerkumians
Copy link
Member

Okay, I tried to push it back on this branch, but I failed miserably and created a new one

@MartinMelikMerkumians
Copy link
Member

@nefethael I will again try to add this to your PR, so that you get proper credits for your contribution

Signed-off-by: Martin Melik Merkumians <melik-merkumians@acin.tuwien.ac.at>
The flag is_large_forward_open is set permanently, as it is never
reseted and saved in the global variable struct g_dummy_connection

Signed-off-by: Martin Melik Merkumians <melik-merkumians@acin.tuwien.ac.at>
@MartinMelikMerkumians MartinMelikMerkumians merged commit 4b1b5f3 into EIPStackGroup:master May 27, 2020
@nefethael
Copy link
Author

@CapXilinx Thanks for this quick integration.

For PC_OPENER_ETHERNET_BUFFER_SIZE manner, my sampleapp assemblies have g_assembly_data064 & g_assembly_data096 with more than 512 bytes, so I had to increase buffer size accordingly.

Maybe we could set assemblies size in opener_user_conf.h and calculate PC_OPENER_ETHERNET_BUFFER_SIZE from it?

@MartinMelikMerkumians
Copy link
Member

@nefethael although the idea sounds nice, the problem is, that this calculation would be inherently bound to the specific sample application. A probably better solution would be to statically check if the created assembly fits into the PC_OPENER_ETHERNET_BUFFER_SIZE.
I checked the Internet on this, and there is a C11 feature which would accomplish that.
The problem is, that Microsoft is not updating their C Compiler to the new standard since years and I don't know if this would work with MSVC compilers or not

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants