-
Notifications
You must be signed in to change notification settings - Fork 103
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 #25
Add Large Forward Open service #25
Conversation
Hi all, This is my first contribution to EIPScanner, I'm not sure my PR takes care of everything (examples, doc, etc)? Also, I only tried LFO feature with OpENer stack and wireshark (I don't own any CIP adapter equipment), so I'm not 100% confident about implementation. First commit allows output assembly to change data without vector copy, maybe a better solution would be to provide something like this in IOConnection? Current solution is not threadsafe.
Second commit allows creation of ConnectionManager object as another class member without causing a timeout on first forward open. Last commit adds large forward open service and provides a new NetworkConnectionParametersBuilder class that takes care of serializing/deserializing network connection parameters. It can be used this way
I'm open to any remarks & suggestions Best regards, |
Hey @nefethael , thank you for the contribution. We appreciate your help and we are open to work here together. Unfortunately, I'm pretty busy at the moment and cannot make the code review in details. I'll give you my feedback at the weekend. Best regards, |
src/IOConnection.cpp
Outdated
auto now = std::chrono::steady_clock::now(); | ||
auto sinceLastHandle = | ||
std::chrono::duration_cast<std::chrono::milliseconds>(now - _lastHandleTime); | ||
Logger(LogLevel::TRACE) << "Last call was " << sinceLastHandle.count() << "ms ago"; |
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.
Unfortunately, there is an issue in our logger... This statement are calculated even if a user use a higher logging level. It could cause performance problems if you have a lot of IO connections.
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.
Ok, I can remove Logger call then?
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.
Yes, please.
If ConnectionManager is created way before forwardOpen call, session is terminated due to timeout. By moving timeout handling in IOConnection, timeout is calculated only if a session exists. Signed-off-by: Vincent Prince <vincent.prince.fr@gmail.com>
Signed-off-by: Vincent Prince <vincent.prince.fr@gmail.com>
@FLIPBack I rebased PR branch with corrections. I just reverted |
Hi @nefethael, thanks I see. What about other two topics?:
|
@FLIPBack I removed LOG_TRACE in notify function and fixed upper/lower-case issues, normally it should be ok |
@nefethael I didn't notice it because I was waiting for an additional commit =) Okay, I'll take your PR. Thank you very much for your contribution again! |
Signed-off-by: Vincent Prince vincent.prince.fr@gmail.com