search

roccomuso / node-ads

NodeJS Twincat ADS protocol implementation
58 stars 21 forks source link

Fix notifications #5

Closed sanderd17 closed 6 years ago

sanderd17 commented 6 years ago

Fixes #4

roccomuso commented 6 years ago

is this well tested? are we sure this fix doesn't introduces other issues?

sanderd17 commented 6 years ago

Depends on your definition of "Well tested". AFAICS, there are no unit tests, so I can only speak from what I noticed.

It didn't break reading and writing (as I use that too), and it fixed notifications. That's all I tested and IMO the most important functionality.

roccomuso commented 6 years ago

Ok

Indeed, It would be good to have also unit test. I can give you access as mantainer if willing to. Unfortunately I don't have any device neither enough knowledge of the bechoff products and ads to write complete unit tests.

sanderd17 commented 6 years ago

Well, we normally use OPC-UA to communicate with Beckhoff PLCs, but for this project, I needed ADS.

I don't know if I'll be using it a lot after this project, so I don't think I'll be a good maintainer as well.

To test it, you'll probably have to mock the incoming TCP stream, and use it to answer to requests and send notifications.