yaacov / node-modbus-serial

A pure JavaScript implemetation of MODBUS-RTU (and TCP) for NodeJS
ISC License
640 stars 241 forks source link

Why port doesn't have method to close connection? #23

Closed abc-z closed 8 years ago

abc-z commented 8 years ago

I want open tcp-connection, read some values and close connection. Probably all ports in ./apis/connection.js must return "this._port" to implementation it, like below

cl.connectTCP = function (ip, options, next) {
    ... 
    this.open(next);
    return this._port;
};

Or it's no good?

'use strict';
let ModbusRTU = require('modbus-serial');
let client = new ModbusRTU();
let port = client.connectTCP('127.0.0.1', {port: 502}, function(){
    client.setID(1);
    client.readCoils(11, 1, function(err, data) {
        console.log(err, data);
        port.close(); // THIS
    });
});
yaacov commented 8 years ago

Hi, thanks, can you make a pull request with the fix , ( and check that it works :-) )?

abc-z commented 8 years ago

What is reason to make port as private member for client? Maybe change this._port to this.port is better way?

yaacov commented 8 years ago

In the indes.js file we implemented an open function. I think a nice solution will be to add a close method, can you try and make one ?

abc-z commented 8 years ago

I think there are 3 variants of implement disconnect ability 1: Described in first message above

let port = client.connectTCP(...) // connect
port.close(); // disconnect

2: Make port property as public member of client

client.connectTCP(...) // connect
client.port.close(); // disconnect

3: Define new method for client

client.connectTCP(...) // connect
client.disconnect(); // disconnect

What way is better?

yaacov commented 8 years ago

you should try all of them, and implement the one you like best.

a. test that it work and not break other things. b. i do not like options that expose the internal port, their are many types of ports and we do not need to trust the user to know what to do with each one of them.

connium commented 8 years ago

Up to now there is no reason to use the internal variable port and I like the idea of having one clean and simple API. Therefore I vote for introducing a new method disconnect on the client which matches option 3 proposed by @abc-z.

yaacov commented 8 years ago

@connium @abc-z we have an open method: https://github.com/yaacov/node-modbus-serial/blob/master/index.js#L145 I will be more happy with a function named close then one named disconnect

@abc-z for start just do:

ModbusRTU.prototype.close = function (callback) {
   this_port.close(callback)
}
abc-z commented 8 years ago

The method "open" is a port method, so method "close" may be assign to port and not pretty as connection method. Little later I add method disconnect to connection prototype. I think It's more clear then export port property.

yaacov commented 8 years ago

Little later I add method disconnect to connection prototype.

Please make a pull request when you do :+1: Please check that the new close method solves your initial issue.

yaacov commented 8 years ago

fixed by: https://github.com/yaacov/node-modbus-serial/pull/24