stream icon indicating copy to clipboard operation
stream copied to clipboard

Can we split modbus-stream into modbus-stream-core and modbus-stream

Open lygstate opened this issue 3 years ago • 13 comments

So that we can use modbus-stream-core in bare-metal environment.

lygstate avatar May 07 '21 14:05 lygstate

If you want the common modbus part, there's modbus-pdu which this module uses.

dresende avatar May 07 '21 15:05 dresende

If you want the common modbus part, there's modbus-pdu which this module uses.

I wanna use stream.js only and provide own driver

lygstate avatar May 07 '21 16:05 lygstate

This module has very little maintenance nowadays. I would advise you to fork and make your changes. You can make a pull request in the end and we can integrate.

dresende avatar May 07 '21 16:05 dresende

This module has very little maintenance nowadays. I would advise you to fork and make your changes. You can make a pull request in the end and we can integrate.

If you have no objection, I'd like create an fork that only contains the core part. can you create a stream-core repository for me and give me admin permission in node-modbus?

lygstate avatar May 07 '21 16:05 lygstate

This module has very little maintenance nowadays. I would advise you to fork and make your changes. You can make a pull request in the end and we can integrate.

Another option is move stream and transport from node-modbus/stream to node-modbus/pdu, which prefer?

lygstate avatar May 07 '21 17:05 lygstate

I think it would be better to have modbus-pdu with just the modbus package. We could think of having an hierarchy like:

  • modbus-stream-<some transport>
    • modbus-stream-core
    • modbus-pdu

Or something similar..

dresende avatar May 07 '21 17:05 dresende

I sent you an invite as admin to https://github.com/node-modbus/stream-core

dresende avatar May 07 '21 17:05 dresende

I think it would be better to have modbus-pdu with just the modbus package. We could think of having an hierarchy like:

  • modbus-stream-<some transport>

    • modbus-stream-core
    • modbus-pdu

Or something similar..

Like this idea, can you create a repository stream-core, I wanna do the initial work

lygstate avatar May 07 '21 17:05 lygstate

I think it would be better to have modbus-pdu with just the modbus package. We could think of having an hierarchy like:

  • modbus-stream-<some transport>

    • modbus-stream-core
    • modbus-pdu

Or something similar..

I'd like do it in two stage, first split modbus-stream-core out, then the drivers.

lygstate avatar May 07 '21 17:05 lygstate

I sent you an invite as admin to https://github.com/node-modbus/stream-core

Do you have objection to use github actions? I wanna convert the CI from travis to github actions

lygstate avatar May 07 '21 17:05 lygstate

I sent you an invite as admin to https://github.com/node-modbus/stream-core

Do you have objection to use github actions? I wanna convert the CI from travis to github actions

Feel free. I also use them in new projects 😃

dresende avatar May 07 '21 17:05 dresende

@lygstate as you are refactoring, could you please consider also adding support for passing in an external transport? e.g. for a ModbusTCP client, instead of having to call modbus.tcp.connect(port, host, options, callback) it'd be nice to be able to pass an existing net.Socket object in, maybe something like this:

import { createConnection } from 'net';
import { ModbusClient } from 'modbus-stream';

function getConnection() {
  // treat as "black box" -- could be transporting over TLS/SSL, could be a test mock, etc.
  return createConnection(...);
}

async function main() {
  const someExistingConnection = getConnection();
  const mbClient = new ModbusClient(someExistingConnection);
  try {
    const result = await mbClient.readCoils(...);
    console.log(result);
    // ...
  } catch(e) {
    throw Error(`FIXME: handle Modbus errors`);
  }
}

jacobq avatar Jun 07 '21 20:06 jacobq

@lygstate as you are refactoring, could you please consider also adding support for passing in an external transport? e.g. for a ModbusTCP client, instead of having to call modbus.tcp.connect(port, host, options, callback) it'd be nice to be able to pass an existing net.Socket object in, maybe something like this:

import { createConnection } from 'net';
import { ModbusClient } from 'modbus-stream';

function getConnection() {
  // treat as "black box" -- could be transporting over TLS/SSL, could be a test mock, etc.
  return createConnection(...);
}

async function main() {
  const someExistingConnection = getConnection();
  const mbClient = new ModbusClient(someExistingConnection);
  try {
    const result = await mbClient.readCoils(...);
    console.log(result);
    // ...
  } catch(e) {
    throw Error(`FIXME: handle Modbus errors`);
  }
}

Hi, PR at https://github.com/node-modbus/stream-core are welcome. I've merged pdu into https://github.com/node-modbus/stream-core for easier to improve stream. Now we can use webpack to pack it, (I assume), there is no depedencies to nodejs 'fs' anymore.

lygstate avatar Jun 08 '21 06:06 lygstate