itemGroup.addItems() Unhandled exception on Parse Method in AddressParserNodeS7 Class

[flacohenao]

addItems Method returns a void.

One of the internal function of addItems is to add an S7Item to the main list. When the S7Item is instantiated by constructor, the first thing it does is to call the .parse() method of the AddressParserNodeS7 class, wich just keep the nodeS7 format safe.

The first step you do for reading a batch of variables by calling readAllItems() is to addItems, but when you add an Item on the wrong format there is an unhandled exception that prevents you to read the rest of the items on the list.

So.. for example if I have this:

itemGroup.addItems(["DB2.X0.0", "DB2,X0.1"]); you can see that the user misstyped the first variable, coz instead of a (,) he typed a (.)....

Now, the connection is not being dropped wich I kinda like... but then, if just right after that I just do:

itemGroup.readAllItems()
                .then(result => {
                    console.log('result: ', result);
                })
                .catch(err => {
                    console.log('ERROR: ', err);
                });

then nothing happens but an ugly error stacktrace!

so... being addItems() a void function, you could arge that we can just try catch the thing, and do something like:

  try {
                            itemGroup.addItems(["DB2.X0.0", "DB2,X0.1"]);
                            itemGroup.readAllItems()
                                .then(result => {
                                    console.log('result: ', result);
                                })
                                .catch(err => {
                                    console.log('ERRORS: ', err);
                                });

                        } catch (error) {
                            // console.log(error);
                        }

And then would be up to the user hanle the error the best way.... (maybe reseting the connection and sending some message or something)

Righ now, I'm thinking in two possibles very nice way to work this out.

the first one: Maybe, can just be.. (apart of try catch the thing) convert the addItems to return Promise in order to the users can chain addItems() and readAllItems() and .catch() something in erros and just do whathever they like with that error... Maybe, in this solution, the PLC can be also Disconnected.

The second one: (The most fancy and amazing one) Maybe, the addItems() method could just handle that exception and... when he adds it to the list:

let item = new S7Item(tag, addr);
this._items.set(tag, item);

can add a null or an undefined value instead of the ITEM, so... when the readAllItems() function is executed, you can just ignore the items with null or undefined values and then... in the success of the promise we coud recive some response like:

result: { 'DB2.X0.0': null, 'DB2,X0.1': true }

That would be awesome, coz, instead of managing reconections and stuff we can advise the user that THAT particular variable couldn't be read.. we can even attach the reason like:

result: { 'DB2.X0.0': {value: null, reason: (the reason why)}, 'DB2,X0.1': {value: true} }

I know that was made like that thinking on the node-Red module but.. Let me know!

[gfcittolin]

The first step you do for reading a batch of variables by calling readAllItems() is to addItems, but when you add an Item on the wrong format there is an unhandled exception that prevents you to read the rest of the items on the list.

You need two separate error-handling sections there. If you addItems() with a badly formatted address, it will throw an exception because of that, but nothing else is touched. You can continue to call readAllItems() on the items added before that.

The errors thrown by readAllItems() are directly tied to the action of reading them (e.g connection loss, area doesn't exist, bad permission, etc), and may be transient (connection bay come back later, a block may be downloaded, etc.)

Maybe, the addItems() method could just handle that exception and... when he adds it to the list:

let item = new S7Item(tag, addr);
this._items.set(tag, item);

can add a null or an undefined value instead of the ITEM

I disagree on this solution. If addItem has returned an Error, the item must be syntactically incorrect (as opposed to just being not available on the PLC, for example). It will never be eventually correct and read, and therefore makes no sense to add it to the list (again, as opposed to a syntactically correct address but unavailable on the PLC, that may eventually become available).

---

Just let me clarify/confirm something. After creating an instance of S7ItemGroup, you should call addItems() once, and then call readAllItems() whenever you want to read their values. You don't need to keep calling addItems() everytime. These two functions perform separate actions, and that's why they'd need separate error handling. Is this the way you're using it?

[flacohenao]

Just let me clarify/confirm something. After creating an instance of S7ItemGroup, you should call addItems() once, and then call readAllItems() whenever you want to read their values. You don't need to keep calling addItems() everytime. These two functions perform separate actions, and that's why they'd need separate error handling. Is this the way you're using it?

No, I'm not calling addItems() everytime, just once. I'm aware of those separated actions on the addItems() and on the readAllItems() .

I see your point on not addng the items syntactically incorrect, and I agree..

But then, can be possible to at least improve the errors and the behivior? I say this, because if we need to try catch the addItems() would be bette if we get some details like:

so, for example... instead of the generic:

Error: Couldn't parse item address, invalid format
    at AddressParserNodeS7.parse (C:\Users\ACER\Documents\NODE PROJECTS\tot-bend\node_modules\nodes7\src\addressParser\nodes7.js:54:19)
    at new S7Item (C:\Users\ACER\Documents\NODE PROJECTS\tot-bend\node_modules\nodes7\src\s7item.js:50:43)
    at S7ItemGroup.addItems (C:\Users\ACER\Documents\NODE PROJECTS\tot-bend\node_modules\nodes7\src\s7itemGroup.js:387:24) 
    at _callee$ (C:\Users\ACER\Documents\NODE PROJECTS\tot-bend\src\components\plc-com-api\/SiemensComApi.js:70:27)        
    at tryCatch (C:\Users\ACER\Documents\NODE PROJECTS\tot-bend\node_modules\regenerator-runtime\runtime.js:45:40)
    at Generator.invoke [as _invoke] (C:\Users\ACER\Documents\NODE PROJECTS\tot-bend\node_modules\regenerator-runtime\runtime.js:274:22)
    at Generator.prototype.<computed> [as next] (C:\Users\ACER\Documents\NODE PROJECTS\tot-bend\node_modules\regenerator-runtime\runtime.js:97:21)
    at asyncGeneratorStep (C:\Users\ACER\Documents\NODE PROJECTS\tot-bend\src\components\plc-com-api\SiemensComApi.js:3:103)
    at _next (C:\Users\ACER\Documents\NODE PROJECTS\tot-bend\src\components\plc-com-api\SiemensComApi.js:5:194)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

So, if I have lets say:

itemGroup.addItems(["DB2.X0.0", "DB2,X0.1", "correct", "correct", "incorrect"]);

I suppose that the error can indicate the name (in case of being multiple variables syntactically incorrect) of the first wrong variable?

I guess I can throw the last idea here....

what if, the correct ones, can be succesfully added to the list this._items and the others are just ignored and notifyed on a reject or and error? or maybe an once() event?

Just for the record, I know that it sounds crazy... but its just more handy...

In any case, if maybe those approaches are not possible, could we just improve the error message to know in an Object well formed error, wich element was the failing one? because if at least one of the list I'm adding fails the others just go to hell... hahaha...

[gfcittolin]

Got your point. Note that there are some more detailed error messages (try adding the invalid address DB1,X0.9 and it will throw Bit address offset out of range 0-7 on "DB1,X0.9"), this one is thrown when we can't even match the basic syntax, and therefore can't be precise on what exactly is wrong. For example, what could be the error in your example when adding the variable with the address "correct"? :)

what if, the correct ones, can be succesfully added to the list this._items and the others are just ignored and notifyed on a reject or and error?

The issue here is how to inform the caller that more than one have an issue, since we can throw only once. Note that the array format here is just for convenience. If you need to handle case-by-case, you could do:

const vars = ["DB2.X0.0", "DB2,X0.1", "correct", "correct", "incorrect"];

for (v of vars) {
  try {
    itemGroup.addItems(v);
  } catch (e) {
    //inform the user about the error
  }
}

What do you think?

P.S.: I'm going to improve the error message when the regex is not met to include the offending item

[flacohenao]

const vars = ["DB2.X0.0", "DB2,X0.1", "correct", "correct", "incorrect"];

for (v of vars) { try { itemGroup.addItems(v); } catch (e) { //inform the user about the error } }

Yeah, that was my thinking..

Ok!! sounds good to me!! of course, improving the error will be good!

[gfcittolin]

As of fc14877 i've improved the message a bit and added the offending variable. It will now print

Could not parse item "${address}", invalid address format

I don't have any more ideas on what/how to improve the error message... any other ideas? :)

[flacohenao]

Oh! haven't seen that!! thank you vert much!!! nice!