-
Notifications
You must be signed in to change notification settings - Fork 179
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
TCP / RTU Client Manager #255
base: v4.0-dev
Are you sure you want to change the base?
Conversation
needs to be default for extending the rtu modbus client
- tests do not have polyfills for older versions of node.js, so reverted the spread operator to Object.assign
Hey @alexbuczynsky this is indeed a useful feature, thanks for your work. I will check your code in the next days and write some feedback if necessary. |
Hey @alexbuczynsky, common things I noticed that I could not annotate in your code. First the standard linter is missing in the package.json for the js files. There are a lot of complaints about assert.equals is deprecated, wrong indentation etc. Please change the following in the package.json
Some Tests are marked with ToDo, what is it about that? |
assert.equal(manager.socketCount, 1, 'Number of sockets should be one') | ||
}) | ||
|
||
it('should find a created client', () => { |
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.
Test Case for when a client is not found?
slaveId | ||
},defaultSerialPortOptions)) | ||
|
||
assert.throws(() => manager.createClient({ host, port, slaveId })) |
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.
host and port are undefined
Is it a common use case to add or remove clients during runtime? Have you checked the memory behaviour in that case? |
@stefanpoeter sorry I have been extremely busy with work. Will get back to this as soon as I can. A couple of comments for now... Linting Missing test cases Memory Behavior
I used the same client manager class for a work based project. The server has been running for the past 3 months without any noticeable memory issues. If we want data to back up that statement, I would have to run some stress test to check this. |
Hi @alexbuczynsky no need to be sorry. This is a open source project so you do what you do when you do it :-) Some data to check the memory behaviour would be great :-) |
Hi, I just tried to build the changes on Ubuntu, but it fails building some dependencies of the serialport package (see logs). |
Summary
Every time I use jsmodbus in one of my projects, I wind up creating a utility class to manage all the clients. I also like to make sure that only one socket is being used for a single
host:port
combination at a time like the image below show:This pull request adds two main new classes:
ModbusTCPClientManager
ModbusRTUClientManager
Description
The class manages the active socket connections and only allows one socket connection open to any modbus master at a time. Each client then uses the same socket reference if the client matches the host and port number of that already created socket.
For example, for the
ModbusTCPClientManager
it keeps a record of the host and port that each client being added to the system has and assigns the correct currently active socket connection to the clients as they are added. The image below shows a diagram of how this works.Below is an example use case for why this manager can be very useful:
Notes: