Cloud-Automation / node-modbus

Modbus TCP Client/Server implementation for Node.JS
466 stars 174 forks source link

unitId should be a parameter of modbus functions #149

Closed laurbadescu closed 6 years ago

laurbadescu commented 6 years ago

Hi. I've seen that unitId is stored as a parameter in the tcp client object on modbus-tcp-client.js. Each call to a modbus function (I am currently using readHoldingRegisters from ReadHoldingRegisters.js) ultimately loads the buffer with the pdu and the unitId from the connection and then sends the buffer. So, regardless of the function used, a single tcp connection will always use the same unitId. It would be very nice if the modbus functions could receive a different unitId as parameter so each of them will load its buffer with a different unitId thus addressing different devices on the same connection. I've made some raw changes in your sources to implement this and is working perfectly.

The only alternative that I see to that is calling modbus functions one by one in sequence and changing the tcp connection's unitId in between. It works as well, of course, but that means that I have to manage the hassle of calling the functions on my own and tracking their status, defeating the whole purpose of reqFifo io modbus-client-core.js

stefanpoeter commented 6 years ago

If you use two modbus server on the same machine, don't the run on different ports?

laurbadescu commented 6 years ago

No, the server is a robustel 3g gateway that can communicate with several modbus devices behind it. I connect to the robustel on a single ip/port and address the devices behind by different unitIds

stefanpoeter commented 6 years ago

So you want to fire requests to a modbus server and send different unitIds per request. I am not sure if that is part of the specification since the unid Ids are bound to the clients and not to the server. I'll check the specs.

laurbadescu commented 6 years ago

Specs say that Read Holding Registers encodes slave address on the body of the call.

The solution I am using right now is: ReadHoldingRegisters.js, line39: adding unitId param to function this.readHoldingRegisters = function (start, quantity, unitId) {

ReadHoldingRegisters.js, line50: overloading pdu with unitId if (unitId) pdu=Buffer.concat([pdu, Buffer.from([unitId])]);

modbus-tcp-client.js, line 134: handling pdu overload

if (pdu.length==6 && pdu[0]>=1 && pdu[0]<=4){ head.writeUInt16BE(pdu.length, 4) head.writeUInt8(pdu[5], 6) pdu=pdu.slice(0,5) } else { head.writeUInt16BE(pdu.length + 1, 4) head.writeUInt8(this.unitId, 6) }

stefanpoeter commented 6 years ago

Unit Identifier – This field is used for intra-system routing purpose. It is typically used to communicate to a MODBUS+ or a MODBUS serial line slave through a gateway between an Ethernet TCP-IP network and a MODBUS serial line. This field is set by the MODBUS Client in the request and must be returned with the same value in the response by the server.

Ok, that makes sense. If you prepare a pull request (including tests) then I will merge it into the main branch. Make sure the new API won't brake anything.

stefanpoeter commented 6 years ago

A better solution would be to make an interface in the constructor for an external socket.

// create a modbus client
var client = modbus.client.tcp.complete({ 
        'socket'           : socket // thats new
        'host'              : host, // we don't need
        'port'              : port, // these four anymore 
        'autoReconnect'     : true, // since we take care of that
        'reconnectTimeout'  : 1000, // somewhere else
        'timeout'           : 5000,
        'unitId'            : 0

    });

This way the API don't need to be changed. You connect the socket outwards an create as many clients as you want to. you just need to assure that the clients won't handle packages that aren't ment for them.

laurbadescu commented 6 years ago

The problem with that approach is that 2 (or more) clients with same ip/port and different unitIds would use the same (external) socket. That means data queueing-in must be handled outside as well in order not to mix data from several requests on the same socket. That defeats the internal queue (that works fine) as well... My solution above was just a quick and dirty change as little as possible solution. For the pull request I would implement an optional parameter for all functions that has the connection's unitId by default and can be set on demand.

stefanpoeter commented 6 years ago

Nope, writing to sockets from different instances will be fine. The internal queue is for handling the packages related to the client (in this case the unitId). All you need to take care of are incoming packages with a unitId other than the one from the sending instance.

stefanpoeter commented 6 years ago

Any progress here?

tremendus commented 6 years ago

@stefanpoeter I need this functionality too - if @laurbadescu didn't make any progress already?

According to the spec here, http://modbus.org/docs/PI_MBUS_300.pdf, halfway down page 9, the address should be one byte / 8 bits, after the start block.

Am I right in thinking that you're arbitrarily setting device Id of 1 onSend():76?

So changing this for the unitId passed in opts should work? What do you recommend for writing a test for this?

I see you have a 3.0alpha branch - is this already handled ... and is this code at all stable or close to it?

stefanpoeter commented 6 years ago

Hi @tremendus,

in my eyes this feature can be very easy to implement. If you pass a socket object on instantiation of the client and set a unitId on the client, then you can pass that socket to other clients as well. On the 'data' event of that socket, you only need to handle messages that contain the same unitId as the client.

Testing is very easy, you only need to mock the socket then.

The v3.0-alpha branch is in development and is currently not stable. But there is work happening.

tremendus commented 6 years ago

It does make sense, but it wouldn't be compatible with 2.x release. It seems that passing the socket may be the better way to do it and it looks like that's where you're heading with 3.x .. but the point @laurbadescu made was to create a simple update to 2.x that is backwards compatible. Passing the unitId as an instance option to the serial client and using this as I mentioned before to set the address in the buffer is quick, easy and 2.x compatible (assuming you main the default address of 0x1 which I have).

Nevertheless, I'll have time later today to dig into your code and see if this an easy retrofit into 2.x or not.

But I do agree with this: your serial 'client', isn't really a modbus client in 2.x - its a serial port connection directing communications bound to a specific addressed device and moving forward, passing a socket makes much better sense.

Out of curiosity, in 3.x, will that socket support the management of the request queue? Most Modbus devices I've worked with are half-duplex, so only one request/response transaction can be on the line at a time. How far out are you with that release?

stefanpoeter commented 6 years ago

I cannot say when there will be a release. @AnthonyMujic did some work lately and I didn't do anything for a while now. You are welcome to help if you like.

The 2.x branch is sending exactly one request and waits for a response. The next request will be send when the previous request terminates.

tremendus commented 6 years ago

Ok, I have looked through your source for 3.0, and I agree its a much better way to go passing a single instance of the serial port around to different serial requests.

I see some errors in the examples (baudrate vs baudRate), I will address.

Do you have a collaboration board anywhere - Slack, or Trello or something? What else is left to do to make it production ready? Your coding style is very different to mine (I always use scopes and raw functions over classes, I prefer composition over inheritance!) but I'll try to get on board and learn from your existing code!

BTW, I'm having trouble completing a readHoldingRegisters() request over serial with 3.0 - I can connect,but all I see is the same three bytes of data returned, no errors. I haven't figured this out yet, is this method perhaps not finished?

stefanpoeter commented 6 years ago

Happy to hear that you want to help :-)

There is no collaboration board anywhere, I'll see if I can set something up on slack. We set the coding standard a while back, it follows the javascript standard coding style. There must be something in the README about that. The Class oriented approach is due to the new es6 features that helped me get rid of many dependencies like stamps, q and so on. Composition over inheritance is the prefered way. The basic code structure is done I guess, at least for the client. I think the server needs some attention.

What operating system are you using? I wasn't able to use the serialport module on linux. Could be exactly the same error.

stefanpoeter commented 6 years ago

I'll refer to issue #178