Support for BBMD messages and COV pushing #127

adam-nielsen · 2019-01-25T03:19:54Z

Checklist

I've read and followed the Contribution Guide.
My commit messages reference affected issues and mention breaking changes if applicable.
I've updated / wrote inline documentation for the source code affected by my changes.
All mandatory CI checks have passed (see when Pull Request has been submitted).

What does this Pull Request do

This adds support for a couple of BACnet messages useful when implementing a client device. It allows the device to appear as a BBMD (sort of a BACnet proxy server), which is useful for some systems (such as Johnson Controls Metasys) which can only talk to BBMD proxies and cannot talk to remote BACnet devices directly.

It also adds the ability to send COV notifications. A remote BACnet system can already subscribe to a data element provided by a device based on this library, however without this addition there is no way to make use of this subscription to push updates to the remote system as they occur.

…rt numbers

adam-nielsen · 2019-02-12T08:07:31Z

Added a few improvements, squashed commits, sync with master branch

fh1ch

@adam-nielsen a big thanks for those awesome contributions and sorry, that it took a while to start the review 💯 🎉

I still haven't managed to review all the code and will continue during the next days and will also do some testing against real devices. I might also cherry-pick the commits, which are unrelated to the BBMD part but are already good to go, into master.

fh1ch · 2019-03-07T15:56:25Z

test/unit/bvlc.spec.js

@@ -13,7 +13,7 @@ describe('bacstack - BVLC layer', () => {
      len: 4,
      func: 10,
      msgLength: 1482,
-      forwardedFrom: null,
+      originatingIP: null,


What is the reason for the rename here? In the case of null, this seems to be semantically wrong (no forwardedFrom -> message not forwarded).

I found it confusing when you are generating a new packet and you have to specify forwardedFrom as your own (device) IP address, when you aren't the one forwarding it (the BBMD is).

If you are writing a BBMD, you find the sender's IP in the forwardedFrom when you haven't forwarded it yet, which is also confusing to me.

I still use forwardedFrom where it makes sense, in client.js when you have received a packet from a BBMD.

If you have any other suggestions for a name let me know, but anything with "forwarded" in it can be misleading in my opinion.

fh1ch · 2019-03-07T16:00:28Z

lib/client.js

@@ -704,19 +704,19 @@ class Client extends EventEmitter {
   *   }
   * );
   */
-  confirmedCOVNotification(address, monitoredObject, subscribeId, initiatingDeviceId, lifetime, values, options, next) {
+  confirmedCOVNotification(receiver, monitoredObject, subscribeId, initiatingDeviceId, lifetime, values, options, next) {


Why the rename here?

Using receiver seems a bit too generic to me (how to address a receiver?).

The parameter for addressing devices should be in-line trough-out all client functions and not only change for a single one.

The change should be reflected in the JSDoc comment above.

This was simply for consistency. I found address confusing when working with BBMD code (is it your address, the sender's address, or the recipient's address?) so I used 'receiver' as this word was already used by other functions (e.g. readPropertyResponse). You are welcome to use a different word but the important part is that they are all the same (better not to mix address and receiver as exists in the current master branch.)

I am not sure what you mean by the parameter being in-line, as this change makes receiver behave the same for all functions, and it can be a string as before.

fh1ch · 2019-03-07T16:16:20Z

lib/client.js

@@ -3,6 +3,7 @@
 // Util Modules
 const EventEmitter      = require('events').EventEmitter;
 const debug             = require('debug')('bacstack');
+debug.trace             = require('debug')('bacstack:trace');


This shouldn't be a global variable. Please stick to debug.

I wanted to have two different types of debugging messages, because the trace ones are very verbose. By doing it this way you can choose to hide the bacstack:trace messages, so you only see the more important bacstack debug messages. For example, like this:

DEBUG='*,-bacstack:trace' node index.js

If these are all changed to be only bacstack then it becomes difficult to see important messages as they are lost amongst the trace.

In my environment the trace messages are extremely useful to ensure firewall rules are working and when debugging why messages are being ignored, but once the code is working there are pages of them every few seconds so they need to be disabled in order to view the normal debug messages.

I am happy to do this another way, but I think it is very important to have the two different levels of messages so that one can be disabled while leaving the other active.

fh1ch · 2019-03-07T16:22:06Z

lib/enum.js

+ * );
+ * console.log(s); // "PRESENT_VALUE(85)"
+ */
+module.exports.getEnumName = function(group, value) {


The enum file should only contain type definitions and no utility functions.

Where would you suggest putting the utility function? I had it in client.js originally but since it's only related to enums I thought enum.js would be a better fit.

I am using enum.js in another module so it is nice that I can get the enum utility function without including any additional node-bacstack code.

Actually this is part of PR #134 instead, so if you want to resolve this issue over there then I can rebase this one to pick up the fix.

fh1ch · 2019-03-07T16:22:55Z

lib/npdu.js

@@ -2,6 +2,7 @@

 const baEnum      = require('./enum');

+const DEFAULT_HOP_COUNT = 255;


The BACNET Standard (135-2016 - 6.2.3) defines the default value as 0xFF, as it was before.

My mistake, will fix this.

This one is also part of PR #134 so I will apply the fix over there. If you are happy to merge that one then I will rebase this one to pick up the changes.

fh1ch · 2019-03-07T16:26:12Z

lib/bvlc.js

+      originatingIP = buffer.slice(4, 8).join('.');
+
+      // Only add the port if it's not the usual one.
+      if (port != DefaultBACnetPort) {


Should be !==

… device BREAKING CHANGE: whoIs and other functions now require a destination address, but it can be null to broadcast as before BREAKING CHANGE: addresses are no longer strings but are now objects, so that they can contain both the target IP and the remote node behind a BBMD

…ubscribers

…ctions

adam-nielsen · 2019-03-28T23:13:16Z

Many thanks for your review, and apologies for my own delayed reply, as I have been away for a few weeks.

I've pushed some changes and left comments explaining my reasoning for others, so I hope this helps.

Feel free to cherry-pick or edit as necessary. I tried to structure the PRs so that you can merge the other non-BBMD ones (e.g. #128 and #134) without too much hassle, so hopefully you won't need to cherry pick these ones. Both #128 and #134 are included here, as I based the BBMD patch on top of those two.

seriph · 2021-10-01T00:45:19Z

This is great! @adam-nielsen would you mind updating this on top of the latest master?

adam-nielsen · 2021-10-01T06:39:49Z

I believe it has already been merged into the fork at https://github.com/BiancoRoyal/node-bacstack along with a bunch of other fixes, as this one appears to be abandoned now. So I'd probably suggest trying that one first.

fh1ch self-assigned this Feb 10, 2019

fh1ch added the enhancement label Feb 10, 2019

fh1ch added this to the 0.0.1-beta.14 milestone Feb 10, 2019

fix(transport): Correctly handle messages from non-standard BACnet po…

7b6c618

…rt numbers

adam-nielsen force-pushed the bbmd branch from ed90716 to 577558f Compare February 12, 2019 08:07

adam-nielsen force-pushed the bbmd branch 4 times, most recently from 9de615d to fb082a0 Compare February 12, 2019 10:21

adam-nielsen force-pushed the bbmd branch from 9799b26 to 2bab316 Compare February 27, 2019 03:50

fh1ch requested changes Mar 7, 2019

View reviewed changes

adam-nielsen added 10 commits March 29, 2019 09:06

fix(client): Default to a hop count of 255 rather than 0

f1133b6

feat(client): Add helper function for constructing a bit string

3b60b36

feature(client): Add helper function to convert enum into string

aeaa690

feat(client): add confirmedCOVNotification() for pushing changes to s…

11259b4

…ubscribers

refactor(client): Remove 'broadcast' from naming of iAm and iHave fun…

e5a6163

…ctions

fix(client): Respond to message errors as unicast not broadcast

e439f54

fix(client): Make confirmedCOV function use sendBvlc

00557be

fix(tests): Update property name

b125eae

fix(client): Address PR feedback

9561259

adam-nielsen mentioned this pull request Mar 28, 2019

Misc fixes #134

Open

adam-nielsen force-pushed the bbmd branch from af76716 to 9561259 Compare March 28, 2019 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for BBMD messages and COV pushing #127

Support for BBMD messages and COV pushing #127

adam-nielsen commented Jan 25, 2019

adam-nielsen commented Feb 12, 2019

fh1ch left a comment

fh1ch Mar 7, 2019

adam-nielsen Mar 28, 2019

fh1ch Mar 7, 2019

adam-nielsen Mar 28, 2019

fh1ch Mar 7, 2019

adam-nielsen Mar 28, 2019

fh1ch Mar 7, 2019

adam-nielsen Mar 28, 2019

adam-nielsen Mar 28, 2019

fh1ch Mar 7, 2019

adam-nielsen Mar 28, 2019

adam-nielsen Mar 28, 2019

fh1ch Mar 7, 2019

adam-nielsen commented Mar 28, 2019

seriph commented Oct 1, 2021

adam-nielsen commented Oct 1, 2021

		@@ -2,6 +2,7 @@

		const baEnum = require('./enum');

		const DEFAULT_HOP_COUNT = 255;

Support for BBMD messages and COV pushing #127

Are you sure you want to change the base?

Support for BBMD messages and COV pushing #127

Conversation

adam-nielsen commented Jan 25, 2019

Checklist

What does this Pull Request do

adam-nielsen commented Feb 12, 2019

fh1ch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-nielsen commented Mar 28, 2019

seriph commented Oct 1, 2021

adam-nielsen commented Oct 1, 2021