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

New Feature: Allow bacstack client to communicate with bacnet server on same device #79

Open
tim-view opened this issue Dec 24, 2017 · 6 comments

Comments

@tim-view
Copy link

I don't know if this is useful for everyone or is a special case for me, but I have a bacnet client I'm writing that needs to communicate with an existing bacnet server. In most cases, this new client and the existing server will be on the same device/at the same IP.

I need a way to bind the client to on UDP port and communicate with the server on the UDP port it is already bound to. If I don't do this, the client can't bind to the UDP port in use by the server and they cannont communicate with each other.

I got this working by doing two things:

  1. Adding a new option to the client called "server_port". It defaults to the value of "port" so it is purely optional and doesn't change behavior if not defined.
  2. Changing bacnet-transport.send to use the "settings.server_port" instead of "settings.port".

I'm not a bacnet expert and am not sure if there is more that needs to be done to make complete this feature, but this change completely solves my issues.

Does anyone else need this feature?

If so, should I submit a pull request?

Is there a better name for the option I added? It makes sense for my use case, but I don't think "server_port" is the right name.

@fh1ch
Copy link
Owner

fh1ch commented Dec 27, 2017

Hi @tim-view

I think this is indeed a rather special use-case you have and I've never seen something like this in other BACNET projects. There are two points I would like to make:

  1. Most BACNET stacks also only allow one single listener/sender port, since many BANCET services rely broadcast. Using a different listener/sender port would therefore mean that those services won't work. I would therefore assume your BACNET server is also quiet special and that some services (e.g. whoIs) won't work at all.
  2. Node BACstack actually allows to re-use the IP/Port to support multiple instances listening on it (see here). I've used this to develop a BACNET-Browser (see here) while simulating a BACNET-Device using one of my other projects (see here) on the same IP/Port.

However, I don't completely oppose the idea of introducing a new and optional parameter to have a different listener/sender port, as long as we keep the impact as small as possible. I always love PRs 😄

I would like to see something like this (code located here):

  var settings = {
    port: options.port || 47808,
    senderPort: options.senderPort || options.port || 47808,
    interface: options.interface,
    transport: options.transport,
    broadcastAddress: options.broadcastAddress || '255.255.255.255',
    adpuTimeout: options.adpuTimeout || 3000
  };

What's your thought on this?

Cheers

@tim-view
Copy link
Author

tim-view commented Dec 27, 2017 via email

@tim-view
Copy link
Author

tim-view commented Dec 27, 2017

Hello @fh1ch

Thanks for getting back to me and thanks for the great work with the library!

I definitely have a special case. I also know very little about BACNET – just enough to get my app working and to be dangerous ☺

I did find that my issue is a little common. It is related to question 15 in this FAQ: http://svn.code.sf.net/p/bacnet/code/trunk/bacnet-stack/doc/README.faq.

My app/service is more like a remote or browser. The server is more common and responds to broadcast. My app is more similar to your browser and connects to one or more BACNET servers to read and write properties.

I will go ahead and submit a PR.

My code was basically the same as the example you sent except for I was calling it serverPort since that was what my use case was. I will change it to senderPort.

I also updated the transport code to use this port when sending:

(bacnet-client)

var settings = {
port: options.port || 47808,
senderPort: options.senderPort || options.port || 47808,
interface: options.interface,
transport: options.transport,
broadcastAddress: options.broadcastAddress || '255.255.255.255',
adpuTimeout: options.adpuTimeout || 3000
};

var transport = settings.transport || new baTransport({
port: settings.port,
senderPort: settings.senderPort,
interface: settings.interface,
broadcastAddress: settings.broadcastAddress
});

and

(bacnet-transport)

self.send = function(buffer, offset, receiver) {
server.send(buffer, 0, offset, settings.senderPort, receiver);
};

If that seems right, I will submit a PR.

One last question…

Should I update the readme to mention this optional parameter? I don’t want to confuse people with it, but would want them to know about it.

Thanks again!!

@fh1ch
Copy link
Owner

fh1ch commented Dec 31, 2017

@tim-view thanks for your reply.

Regarding the FAQ entry: yes, that's indeed a common issue, which I'm also experience when using my browser / device simulator. If not started in the right order (device first), who-is won't work. I'm currently working around this for some upcoming compliance tests using Docker and it's internal networking (each container has it's own IP, see https://github.com/fh1ch/node-bacstack/blob/test/compliance-tests/docker-compose.yml).

Yes, the code snippets provided by you look all-right and should pass all linter checks with ease (I would still suggest you to read the CONTRIBUTING.md, as the commit message has to be specially formatted, so it will work for the CHANGELOG.md).

Regarding documentation: The README.md doesn't really contain any documentation (on purpose) and shall only give a broad overview. I therefore also think that you should not add anything to the readme. However, the project has a lot of inline documentation (see https://github.com/fh1ch/node-bacstack/blob/master/lib/client.js#L23), which should be updated accordingly.

@username-II
Copy link

I wanted to do some quick bacnet developement and had a similar issue.

According to the readme from https://sourceforge.net/projects/yetanotherbacnetexplorer/: The BACnet socket at 0xBAC0 should be used for receiving broadcasts. Not to transmit anything or receive any unicasts. If you want to transmit a broadcast you use the other socket (a random port given to you by the PC) and send the broadcast to the 0xBAC0 port.

After a quick look at the source this mod to bacstack\lib\transport.js seemed reasonable to allow addresses to be string ip/id or object with {address:'',port:0} with the following caveats:

client.js : control methods
They compare address with broadcast address to set broadcast flag in message blocks.
> ok as long as to send a broadcast you provide the broadcast ip string instead of address & port object as target address.

poss issues I didn't follow up:
baNpdu.encode : 3rd arg is msg source ip compared to set binary structure flags. But compared as an object not a string?

Seemed to work with some very basic testing.

var dgram = require('dgram');

module.exports = function (settings) {
    var self = this;

    var messageHandler, forwardMessage = function (msg, rinfo) {
        if (messageHandler) messageHandler(msg, rinfo);
    };
    var errorHandler, forwardError = function (err) {
        if (errorHandler) errorHandler(err);
    };

    // capture broadcast messages
    var broadcast = dgram.createSocket({ type: 'udp4', reuseAddr: true }, forwardMessage);
    broadcast.on('error', forwardError);

    // capture personal replies and send requests/broadcasts
    var personal = dgram.createSocket({ type: 'udp4' }, forwardMessage);
    personal.on('error', forwardError);

    // Public functions
    self.setMessageHandler = function (handler) {
        messageHandler = handler;
    };

    self.setErrorHandler = function (handler) {
        errorHandler = handler;
    };

    self.getBroadcastAddress = function () {
        return settings.broadcastAddress;
    };

    self.getMaxPayload = function () {
        return 1482;
    };

    self.send = function (buffer, offset, receiver) {
        // modified to accept a string or object with address & port properties
        if ('object' === typeof receiver && receiver.port && receiver.address)
            personal.send(buffer, 0, offset, receiver.port, receiver.address);
        else if ('string' === typeof receiver)
            personal.send(buffer, 0, offset, settings.port, receiver);
        else // just broadcast
            personal.send(buffer, 0, offset, settings.port, settings.broadcastAddress);
    };

    self.open = function () {
        broadcast.bind({ port: settings.port, address: settings.interface, exclusive: false });
        personal.bind({ address: settings.interface }, function () {
            personal.setBroadcast(true);
        });
    };

    self.close = function () {
        broadcast.close();
        personal.close();
    };

    return self;
};

@Apollon77
Copy link

@tim-view @username-II Is this feature still interesting or do you already developed it? Maybe you could share your code as PR to https://github.com/BiancoRoyal/node-bacstack . I also had probles running tests on same device as the server ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants