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

Modbus TCP Master is creating multiple connections to single TCP Gateway #195

Open
Wawszczak opened this issue Feb 9, 2023 · 9 comments

Comments

@Wawszczak
Copy link
Contributor

When multiple TCP modbus slaves are configured with the same IP and TCP port and different Slave ID for every of them, then engine tries to create multiple TCP connection for each of slaves separatelly.
Many of gateways (all but one I have tested) have limitation to 2-4 connections. This makes issues in querying network consisting more devices over the TCP connection limit.
It results in dropping connection, reconnecting and generally poor performance.
I can help solving this, but need guidance where it is implemented.

Current behavior:
separate TCP connection for each slave regardless the IP/PORT TCP destination.
Expected behavior:
common TCP connection for all slaves sharing common IP/PORT pair.

@Wawszczak
Copy link
Contributor Author

modbus_master.cpp contains implementation of port sharing in modbus RTU. Similar approach should be used for TCP connection. Functions initializeMB() and querySlaveDevices() should be adjusted.
I have created fork for this and will report back with pull request.

@thiagoralves
Copy link
Owner

I was about to write exactly what you wrote! :) A similar strategy was already developed for RTU, since they must share the same COM/TTY port. Thank you for contributing fixing this. Let me know if you need any help.

@Wawszczak
Copy link
Contributor Author

I was about to write exactly what you wrote! :) A similar strategy was already developed for RTU, since they must share the same COM/TTY port. Thank you for contributing fixing this. Let me know if you need any help.

Thank you for confirmation. I have analyzed the code and have performed refactor.
I am testing now inside docker image, but this should be architecture independed.

I have only one doubt. I cannot figure out what is purpose of special_functions[2] within modbus_master.cpp. Is this something I should take care of? My code is available in my forked repo, if you want to review it.
After testing I will create pull request.

@Wawszczak
Copy link
Contributor Author

@thiagoralves - I have another question.
In every read/write block, there are two sleeps - one is sleepms and another is nanosleep.
I don't understand the timing for nanosleep. There is ts.tv_nsec = (1000*1000*1000*28)/mb_device[i].rtu_baudrate. I can understand idea, but why there is multiplication by 28 ?
I am discussing delays, because I think there is a need to sleep for at least one baud between each operation. So we have two different waiting periods, and we should sleep for the longer period, not both. And I think that we should introduce baudrate for TCP as optional supplemental parameter - because all gateways has underlying Serial connection.

@thiagoralves
Copy link
Owner

special_functions[2] only counts the number of failed connection attempts, mainly to be used by the PLC program.

The ts.tv_nsec delay only exists for RTU connections, as part of the "end of message" delay expected for Modbus RTU. It looks like the following delayms call is a duplicate. I don't know who put it there, maybe it should be removed indeed. I'll check

@thiagoralves
Copy link
Owner

This is the commit that added it: 34f2a2d

I don't remember the justification for why this guy added it there if there was already a calculated pause according to the baud rate. Maybe checking his commit you will have some hints, but my guess is that it is redundant and can be removed

@thiagoralves
Copy link
Owner

He is the one that made the changes so that RTU devices can share the same port: #156

As far as I can tell, the default value for the transmission pause is zero, so it shouldn't affect anything. It is more an optional field on the UI that can delay the transmission of new packets. Perhaps the only thing to do in there would be to add a check
if (mb_devices[i].rtu_tx_pause != 0) to make sure the syscall is never made if transmission pause is zero.

@embeddman
Copy link

Are you planning to add this feature?

@thiagoralves
Copy link
Owner

@Wawszczak is your PR ready?

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

No branches or pull requests

3 participants