search

jisotalo / node-red-contrib-ads-client

Beckhoff TwinCAT ADS client library for Node-RED (unofficial). Connects to Beckhoff TwinCAT automation systems using ADS protocol.
https://flows.nodered.org/node/node-red-contrib-ads-client
MIT License
15 stars 4 forks source link

Improved error handling #8

Closed Hopperpop closed 3 years ago

Hopperpop commented 3 years ago

Suggestions for error handling:

1) Reading the following topic: nodered.org. It seems that done(err) and node.error() shouldn't be called together. As they serve the same purpose but for different node-red versions.

2) I saw the formatError() function, but couldn't figure out why a new object is created. I modified it in a way it still appends the ads error to the message. And also the possibility to append the adsErrorInfo object to the msg. If the catch node is used, this object will be fully available for debugging. What do you think about this?

3) In the subscription node, only the first error is logged to prevent continually spamming of errors in the log. (Still need to do this for subscription errors?)

4) There are some other cleanups to prevent double errors. Ex. in ads-client-connection.js: connect() only throws an error. The caller needs to catch the error and decide if it wants to log it or not.

These are all suggestions, everything can be debated.

jisotalo commented 3 years ago

Thanks! I will check this in few days, been quite busy, sorry.

jisotalo commented 3 years ago

Thanks Hopperpop! Sorry for delay.

All your changes look great! I'm happy for your help as I haven't used the Node-RED much, basically only for some small tests.

  1. Reading the following topic: nodered.org. It seems that done(err) and node.error() shouldn't be called together. As they serve the same purpose but for different node-red versions.

That seems to be true!

  1. I saw the formatError() function, but couldn't figure out why a new object is created. I modified it in a way it still appends the ads error to the message. And also the possibility to append the adsErrorInfo object to the msg. If the catch node is used, this object will be fully available for debugging. What do you think about this?

Yep now looking at the diff, I can't find any good reason why to create a new object. The new way is better. That's also a good feature to include the original ADS error information as it's usually quite informative when thinking what's going wrong.

  1. In the subscription node, only the first error is logged to prevent continually spamming of errors in the log. (Still need to do this for subscription errors?)

Seems good, makes no sense to fill the whole log with same error again and again.

  1. There are some other cleanups to prevent double errors. Ex. in ads-client-connection.js: connect() only throws an error. The caller needs to catch the error and decide if it wants to log it or not.

Thanks!