Connected support

[bmhgit]

Added support for connected messages by creating forwardOpen/forwardClose handlers as well as updating the sendUnitData EthernetIP packet.

Description, Motivation, and Context

The requests can be built by the new connection manager. If a connection ID is assigned, it gets saved in the ENIP class instance and used for connected packets. When instancing a controller, you can specify wether you want this session to be connected or unconnect, which keeps compatibility.

Since this changes the way the library may send packets, it's important that you look over the changes and tell me if it's okay the way I implemented it, or maybe we could merge unconnected send into the connection manager.

How Has This Been Tested?

Since the library did not implement sendUnitData before, there should be no effect on sending ordinary CIP data. I tested it on a 1769-L32E PLC and implemented a couple of jests.

Screenshots (if appropriate):

The screenshot illustrates that both connected (Get Attribute Single from Identity Object) as well as unconnected (ReadTag) work side by side.

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [X] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• [X] My code follows the code style of this project.
• [ ] My change requires a change to the documentation.
• [ ] I have updated the documentation accordingly.
• [ ] I have read the CONTRIBUTING document.
• [X] I have added tests to cover my changes.
• [X] All new and existing tests passed. -> I needed a new sendUnitData snapshot in order to cover the correct format.
• [X] This is a work in progress, and I want some feedback (If yes, please mark it in the title -> e.g. [WIP] Some awesome PR title)

Related Issue

[coveralls]

Pull Request Test Coverage Report for Build 133

• 138 of 230 (60.0%) changed or added relevant lines in 5 files are covered.
• 3 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+4.2%) to 55.164%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/enip/cip/connection-manager/index.js	80	81	98.77%
src/enip/index.js	16	20	80.0%
src/controller/index.js	37	124	29.84%
<!--	Total:	138	230	60.0%	-->

Files with Coverage Reduction	New Missed Lines	%
src/enip/index.js	1	27.59%
src/controller/index.js	2	24.28%
<!--	Total:	3		-->

Totals
Change from base Build 120:	4.2%
Covered Lines:	633
Relevant Lines:	1109

---

💛 - Coveralls

[bmhgit]

Hi, I added a change to how the connected messages are declared. I moved it from each individual write_cip function to one parameter when instantiating the Controller Instance. My reason for this is that when a user wants to send connected messages, they probably want to specify that for the whole session, at least that's how I interpret the use-case. If there are no more issues, is it possible to get this (and other QoL changes) merged? I've been using connected messages for a while now.

[cmseaton42]

Quick question:

Does the controller fallback to using unconnected messaging if the forward open request fails?

[Penlane]

Quick question:

Does the controller fallback to using unconnected messaging if the forward open request fails?

Currently, the user would have to catch the timeout, then try to establish a new unconnected-connection (oh CIP, where unconnected messages need to be connected...). I'm very much in favor of just adding a try - catch around the forwardOpen()-function, where we set connectedMessaging to false in the catch-block and return a INFO-Message (or something like that) to the user.

[cmseaton42]

Quick question: Does the controller fallback to using unconnected messaging if the forward open request fails?

Currently, the user would have to catch the timeout, then try to establish a new unconnected-connection (oh CIP, where unconnected messages need to be connected...). I'm very much in favor of just adding a try - catch around the forwardOpen()-function, where we set connectedMessaging to false in the catch-block and return a INFO-Message (or something like that) to the user.

Yes, I definitely think this is the best way to handle it as well. Perhaps this should be its own PR as well?

[cmseaton42]

Also, I am assuming that @Penlane is the same user as @bmhgit lol?

[Penlane]

Perhaps this should be its own PR as well?

Looks like I was a little too quick about the implementation. I can revert the Commit and open a new PR after we merge the current implementation without fallback, if your prefer.

Also, yes, same person, but from now on, only Penlane will be communicating. :smile:

[cmseaton42]

Perhaps this should be its own PR as well?

Looks like I was a little too quick about the implementation. I can revert the Commit and open a new PR after we merge the current implementation without fallback, if your prefer.

:rocket:

Lol, that was quick indeed. Yea, I think it is best to keep the PRs centered around one piece of functionality at a time. This makes going back and reviewing changes later a little easier :)

[cmseaton42]

I am good with this PR. Assuming that you guys (@Penlane and @jhenson29 ) have resolved all conversations you were having concerning the PR/ future PRs, I will go ahead and pull this 🚀

[cmseaton42]

Merged, Ill release the v2.0-Beta.0 release to npm soon once I merge a couple of @bmhgit's changes.