roccomuso / node-ads

NodeJS Twincat ADS protocol implementation
58 stars 21 forks source link

fix multiRead and add getHandles functionality #12

Closed ovi1337 closed 6 years ago

ovi1337 commented 6 years ago

This PR will implement the multiRead successfully by using the exactly the same like read or write commands. Maybe we can simplify the API later, that you can also use an array in the read/write command and the lib is internally using the multiRead. (multiWrite should also be implemented then)

ovi1337 commented 6 years ago

@ChrisHanuta have you maybe an idea about this issue?

I've also created a topic about this issue in a german SPS community, but there's no response currently: https://www.sps-forum.de/codesys-und-iec61131/93118-mit-der-nutzung-von-handles-im-adsigrp_sumup_readwrite-request.html#post699030

ovi1337 commented 6 years ago

Currently there's no other way to do a multiRead without IndexGroup and IndexOffset, but in general this fix is working well and as expected. I will remove the fallback to use this method with symbolNames only, add an error message, for prevent the wrong usage and would like to merge it, that we can close the open tickets and have a clear way to use the multiRead.

What are you guys thinking about it?

ovi1337 commented 6 years ago

Mmh, it seems to be this what i've already implemented here: https://github.com/roccomuso/node-ads/pull/12/files#diff-94814b9df475840da8a076fabd72c51dR275

The problem is, that i don't know how i can use the handles in the multiRead command. It must be something similar to https://github.com/roccomuso/node-ads/blob/master/lib/ads.js#L354 But currently i don't have figured out the way to use it in the multiRead.

I'm pretty sure that i also must use

indexGroup: 0xF005
indexOffset: <symHandle>

But then i just get state: 1794 as callback. Maybe you have a good idea?

ovi1337 commented 6 years ago

Thank you for your support - maybe i miss something, but this is exactly what i already did here: https://github.com/roccomuso/node-ads/pull/12/files#diff-94814b9df475840da8a076fabd72c51dR275

I want to fetch Values with the multiRead method with the symbolName instead of the indexGroup and indexOffset. It's necessary to use the handles for fetching by name (0x0000F005 READ_/WRITE_SYMVAL_BYHANDLE). Where i'm currently stuck is the use of the handles in the multiRead: https://github.com/roccomuso/node-ads/pull/12/files#diff-94814b9df475840da8a076fabd72c51dR381

There must be something wrong or is missing. Currently i'm using the single handle assignment for simplicity at this point. The handles are available at this point, but the usage seems to be wrong or whatever.

TL;DR:

ovi1337 commented 6 years ago

Uh cool, let me take a look :)

I think the initial idea behind handle.arrayid was the index position. I think it's something unfinished/extracted from somethere. Generally the whole lib needs some more care and cleanup, but we're in a good way.

ovi1337 commented 6 years ago

Yes, the auto-formatting broke it, it's currently work in progress and will be fine again before we will merge it.

ovi1337 commented 6 years ago

Yes, i also think we can complete get rid of lodash. I just did the same things like the initial stage :) I'm also not very happy with all the names. But one step to the other. I will do an update on my PR in some minutes.

ovi1337 commented 6 years ago

Ah good to know about the handle.arrayid:)

ovi1337 commented 6 years ago

BTW: We also should introduce data models and reorganize the whole lib.

ovi1337 commented 6 years ago

bump for approvals :)

ovi1337 commented 6 years ago

ping

ovi1337 commented 6 years ago

@ChrisHanuta about the formatting and comparing the code i'd like to recommend you to simply switch to "split mode" if you still use it: image

ovi1337 commented 6 years ago

@ChrisHanuta fixed

ovi1337 commented 6 years ago

I will also do my own thing now, because i'm not happy at all to spend many hours to improve this lib without any care of the owner of this fork...

roccomuso commented 6 years ago

Sorry, I was a little bit busy.

Let me give you access to the whole repo to be able to merge on your own