roccomuso / node-ads

NodeJS Twincat ADS protocol implementation
58 stars 21 forks source link

multiRead - Error: parameter size not correct #7

Closed ovi1337 closed 6 years ago

ovi1337 commented 6 years ago

Hi, i have a problem by using the multiRead method, i tried different things and also only one value, but it's returning every time only the error message Error: parameter size not correct. What i'm doing wrong? The communication itself is working well so far. Single values are working as expected.

I'm using a Beckhoff TwinCAT v2.11.

var ads = require('node-ads')

var options = {
    host: "192.168.1.174",
    amsNetIdTarget: "10.0.0.105.1.1",
    amsNetIdSource: "192.168.137.50.1.1",
}

var client = ads.connect(options, function() {
    this.notify({
        symname: '.AI_Light_Sensor',       
        bytelength: ads.INT
    });

    this.notify({
        symname: '.PT_Temp_Sensor',       
        bytelength: ads.INT
    });

    this.read({
        symname: '.AI_Light_Sensor',       
        bytelength: ads.INT,  
    }, function(err, handle) {
        if (err) console.log(err)
            console.log(handle.value)
    });

    this.read({
        symname: '.PT_Temp_Sensor',       
        bytelength: ads.INT,  
    }, function(err, handle) {
        if (err) console.log(err)
            console.log(handle.value)
    });

    this.multiRead([
        {
            symname: '.DO_Lamp_1',       
            bytelength: ads.BOOL,  
        }, {
            symname: 'MAIN.test',       
            bytelength: ads.BOOL,  
        }, {
            symname: '.DI_Button',       
            bytelength: ads.BOOL,  
        }, {
            symname: '.AI_Light_Sensor',       
            bytelength: ads.INT,  
        }, {
            symname: '.PT_Temp_Sensor',       
            bytelength: ads.INT,  
        }, {
            symname: '.DIM_Lamp_2',       
            bytelength: ads.INT,  
        }, {
            symname: '.GLOBAL_Dimmer_1',       
            bytelength: ads.INT,  
        }
    ], 
    function(err, handle) {
        if (err) console.log(err)
            console.log(handle)
    });
});

client.on('notification', function(handle){
    console.log(handle)
})

process.on('exit', function () {
    console.log("exit")
})

process.on('SIGINT', function() {
    client.end(function() {
        process.exit()
    })
})

client.on('error', function(error) {
    console.log(error)
})

This is the error message on the console:

{ options:
   { host: '192.168.1.174',
     amsNetIdTarget: '10.0.0.105.1.1',
     amsNetIdSource: '192.168.137.50.1.1',
     port: 48898,
     amsPortSource: 32905,
     amsPortTarget: 801,
     verbose: 0 },
  invokeId: 9,
  pending:
   { '6': { cb: [Function], timeout: [Object] },
     '7': { cb: [Function], timeout: [Object] },
     '8': { cb: [Function], timeout: [Object] },
     '9': { cb: [Function], timeout: [Object] } },
  symHandlesToRelease:
   [ <Buffer 44 00 00 f7>,
     <Buffer 44 00 00 f7>,
     <Buffer 63 02 00 f7>,
     <Buffer a3 00 00 f7> ],
  notificationsToRelease: [],
  notifications: {},
  dataStream: null,
  tcpHeaderSize: 6,
  amsHeaderSize: 32,
  adsClient:
   EventEmitter {
     connect: [Function],
     end: [Function],
     readDeviceInfo: [Function],
     read: [Function],
     write: [Function],
     notify: [Function],
     writeRead: [Function],
     getSymbols: [Function],
     multiRead: [Function],
     _eventsCount: 2 },
  tcpClient:
   Socket {
     connecting: false,
     _hadError: false,
     _handle:
      TCP {
        reading: true,
        owner: [Circular],
        onread: [Function: onread],
        onconnection: null,
        writeQueueSize: 0 },
     _parent: null,
     _host: null,
     _readableState:
      ReadableState {
        objectMode: false,
        highWaterMark: 16384,
        buffer: [Object],
        length: 0,
        pipes: null,
        pipesCount: 0,
        flowing: true,
        ended: false,
        endEmitted: false,
        reading: false,
        sync: false,
        needReadable: true,
        emittedReadable: false,
        readableListening: false,
        resumeScheduled: false,
        destroyed: false,
        defaultEncoding: 'utf8',
        awaitDrain: 0,
        readingMore: false,
        decoder: null,
        encoding: null },
     readable: true,
     domain: null,
     _events:
      { end: [Object],
        finish: [Function: onSocketFinish],
        _socketEnd: [Function: onSocketEnd],
        data: [Function],
        timeout: [Function],
        error: [Function] },
     _eventsCount: 6,
     _maxListeners: undefined,
     _writableState:
      WritableState {
        objectMode: false,
        highWaterMark: 16384,
        finalCalled: false,
        needDrain: false,
        ending: false,
        ended: false,
        finished: false,
        destroyed: false,
        decodeStrings: false,
        defaultEncoding: 'utf8',
        length: 0,
        writing: false,
        corked: 0,
        sync: false,
        bufferProcessing: false,
        onwrite: [Function: bound onwrite],
        writecb: null,
        writelen: 0,
        bufferedRequest: null,
        lastBufferedRequest: null,
        pendingcb: 3,
        prefinished: false,
        errorEmitted: false,
        bufferedRequestCount: 0,
        corkedRequestsFree: [Object] },
     writable: true,
     allowHalfOpen: false,
     _bytesDispatched: 595,
     _sockname: null,
     _pendingData: null,
     _pendingEncoding: '',
     server: null,
     _server: null,
     read: [Function],
     _consuming: true,
     [Symbol(asyncId)]: 6,
     [Symbol(bytesRead)]: 0 },
  dataCallback: [Function] }
Error: parameter size not correct
    at getError (/Users/oVi/dev/src-one/homecontrol/node_modules/node-ads/lib/ads.js:1245:13)
    at Object.getWriteReadResult (/Users/oVi/dev/src-one/homecontrol/node_modules/node-ads/lib/ads.js:760:13)
    at Object.analyseResponse (/Users/oVi/dev/src-one/homecontrol/node_modules/node-ads/lib/ads.js:235:28)
    at Object.checkResponseStream (/Users/oVi/dev/src-one/homecontrol/node_modules/node-ads/lib/ads.js:173:25)
    at Object.analyseResponse (/Users/oVi/dev/src-one/homecontrol/node_modules/node-ads/lib/ads.js:241:23)
    at Object.checkResponseStream (/Users/oVi/dev/src-one/homecontrol/node_modules/node-ads/lib/ads.js:173:25)
    at Object.analyseResponse (/Users/oVi/dev/src-one/homecontrol/node_modules/node-ads/lib/ads.js:241:23)
    at Object.checkResponseStream (/Users/oVi/dev/src-one/homecontrol/node_modules/node-ads/lib/ads.js:173:25)
    at Object.analyseResponse (/Users/oVi/dev/src-one/homecontrol/node_modules/node-ads/lib/ads.js:241:23)
    at Object.checkResponseStream (/Users/oVi/dev/src-one/homecontrol/node_modules/node-ads/lib/ads.js:173:25)
roccomuso commented 6 years ago

Ok, the error code given by the server is 1797: parameter size not correct. I don't know if the server has more detailed logs, that would definetly help debugging the issue. I would also try to change the types.

ovi1337 commented 6 years ago

Hey, thank you for your quick response!

Currently i'm not at home to dig deeper, but maybe i found something:

Actually the object for getting single values looks like this:

{
    symname: '.AI_Light_Sensor',
    bytelength: ads.INT,
}

Can it be that it's wanted like this?

{
    name: '.AI_Light_Sensor',
    readLength: ads.INT,
}

The value assignments are looking quite different and not consistent, can it be?

Please take a look here:
multiRead https://github.com/roccomuso/node-ads/blob/master/lib/ads.js#L262-L268

read https://github.com/roccomuso/node-ads/blob/master/lib/ads.js#L337-L342

I think it really can be possible, because the length don't will be defined and also the name is missing, so the defined length of the value the name is left in undefined|null state. There are 7 values in my example example, but all values are not defined in case of different namings of the necessary parameters, because there is no error check for missing parameters at this place.

roccomuso commented 6 years ago

@ovi1337 I didn't noticed it. But you seem right. The multiread oneOption.name should be renamed in oneOption.symname in the lib. Could you try it when you get back home?

Then we should also write a multiread sample usage on the README.md doc. This is related to #3

ovi1337 commented 6 years ago

Yes I will try it out when I'm back home. I also can open a pull request with a compatibility patch for consistency if you want.

roccomuso commented 6 years ago

Yes sure. That would be really appreciated

ovi1337 commented 6 years ago

mmh... something is absolutely strange in this method. The whole buffer calculation is wrong and is wrong and all seems to be completely broken here. Could it be that this function has never worked before and is unfinished?

roccomuso commented 6 years ago

@ovi1337 It's highly possible and It's also not documented. We could try to fix it.

ovi1337 commented 6 years ago

I checked out the documentation - there's no useful sample for communicating via TCP in javascript, but this can maybe help us to fix it. I think the transmission container is basically the same and only the way of communication is a bit different. (simple web request vs TCP sockets)

https://infosys.beckhoff.com/content/1033/tcsample_tcadswebservicejs/html/sample01.html?id=3617063643338924848

https://infosys.beckhoff.com/content/1033/tcsample_tcadswebservicejs/html/sample02.html?id=6127851926177519742

This also can be helpful (It's for Java and is also utilizing the SDK, but there are some more descriptions about the data packets): https://infosys.beckhoff.com/content/1033/tcsample_java/html/tcjavatoads_sample12.html?id=6497303773881420725

https://infosys.beckhoff.com/content/1033/tcsample_java/html/tcjavatoads_sample13.html?id=7916112603633813148

roccomuso commented 6 years ago

The communication via tcp should be the same used by the single read. Here It's just a matter of dealing with the data packets parsing, but i wasn't able to find a clear pdf.

ovi1337 commented 6 years ago

I've found a solution for this problem yesterday evening and i'm working on a fix. With the current solution it's only possible to make a multiRead by using the indexGroup together with the indexOffset instead of just using the symName. I think it's necessary to make a larger refactoring for more comfort. There are also many things in the library which needs to have adjustments and improvements.

But at first it's working so far and we can plan the next improvements.

roccomuso commented 6 years ago

@ovi1337 great. I could give you access to the repo as mantainer

ovi1337 commented 6 years ago

hahaha, okay 😅

By the way, i think that i've handled the way to use just the SymbolName - We have to resolve the ID's with AdsReadSymbolDesc before, after that i have the whole configuration of the Symbol, including indexGroup and indexOffset. https://infosys.beckhoff.com/english.php?content=../content/1033/tcadsocx/html/tcadsocx_methadsreadsymboldesc.htm&id=3582448685738059069

The funny side-effect is, that we normally also don't need to define the byteLength anymore. To save unnecessary CPU time is it also possible to implement a Symbol-Cache. But for this "ultra comfortable" way should it be a complete refactoring i think :)

Let's just fix this part first ^^

roccomuso commented 6 years ago

OK, we're first gonna merge the existing PR if you agree

ovi1337 commented 6 years ago

@roccomuso yes, absolutely! I'm still on the multiRead. I hadn't much time at the weekend, but i think that have some time to finish it today :)

roccomuso commented 6 years ago

Great, In the meanwhile I'll merge any pending PR. (I'm not gonna publish a new release yet, first let's try to do a test and fix the multiRead). Try working with the new merged lib on github to make sure we don't introduce bugs or any breaking change with the new additions :)

ovi1337 commented 6 years ago

Fixed by myself