Daxbot / node-canopen

CANopen implementation for NodeJS
MIT License
32 stars 11 forks source link

Allow loading EDS from string/Buffer, and adding data entries at runtime #8

Closed dawn-minion closed 4 years ago

dawn-minion commented 4 years ago

In our tests we're currently generating an EDS at runtime based on input data, and then verifying our SDO operations work against a mock server based on the EDS generated at runtime. In Device.js, it would be nice if we could just pass the already loaded string or Buffer to this here instead of having to save it to a file.

As well, it would be nice to be able to dynamically add entries to the Device at runtime, as some cases we do not use an EDS. Whilst we can just do this by manually loading it via device.dataObjects, it'd be nice if it was an official API in case the Device.js object changes later.

wilkinsw commented 4 years ago

I completely agree, I have already added EDS manipulation as a feature on the v2-beta branch, but I have yet to fully test it or write the documentation.

The biggest change for v2 is that I have moved away from creating 'virtual' nodes with the same address as your physical node for object dictionary manipulation. Instead you would create a new uniquely addressed node and make it aware of your physical nodes through the EDS file. I hope to create some examples with the updated API this week, but all the tests work if you want to play around with it.

dawn-minion commented 4 years ago

The new API looks great! The loopback mode will be really handy too, as we were essentially doing just that to run tests. I'll have to give it a try :)

wilkinsw commented 4 years ago

How does this look? https://github.com/DaxBot/node-canopen/blob/v2-beta/examples/eds_creation.js

I'm not clear on what you mean by loading from a string/Buffer. Could you describe that use case?

dawn-minion commented 4 years ago

That looks great, honestly - exactly what we'd need :) For the string/buffer, we have some self tests where we define a list of object dictionary entries (So index+sub, data type, default value and such) that we need in the test, and then generate an EDS that matches that for our EDS parser. We don't actually save this EDS to disk, but just pass the string value of it to our parser. Was hoping we could do the same here to avoid writing it to disk.

In real usage though we always load it from a file, so it's just purely a nicety for self tests.

wilkinsw commented 4 years ago

Ok, I'm using an ini parser to do all the heavy lifting for the file import, so there doesn't seem to be a great way to add string parsing without writing it from scratch. The Device class takes the EDS as an input, so the simplest solution might be for you to extend the EDS class with a string parser and include it with the tests that need it. Then you can load the EDS from a string and use it to create the Device object as normal.

dawn-minion commented 4 years ago

@wilkinsw If I may bug you once more - I'm trying to give the new beta version a go in our project, and I'm not quite sure how data types now work in the SDO client. Am I right in thinking that we now just get the raw buffer back, and we need to transform it to the type we expected, or is there something I'm missing?

wilkinsw commented 4 years ago

Under version 1 Device was an abstraction of the remote node. So SDO upload/download would read/write directly to the local copy of the object dictionary. This was done to ensure that the local copy always had the most recent value.

With version 2 you are creating a full CANopen node that has its own independent address and accesses your remote nodes as an SDO client. This means that the EDS file for your local node could be very different than the remote nodes (and can be constructed programmatically).

Because the SDO module can no longer assume the datatype it is accessing it now deals exclusively in Buffer objects. Here is an excerpt from the SDO client example writing a string to node 0xA address 0x2000 and then reading that same object: https://github.com/DaxBot/node-canopen/blob/e8611caeee4f3776141b29980f3d9ad844912af2/examples/sdo_client.js#L45-L54

So to answer your question, yes, you are correct. I recommend using the functions rawToType and typeToRaw to convert the raw Buffers to their cooked values and vice versa. You'll find those exposed in the EDS module.

I'm excited that you're trying out the beta branch! If you have any more suggestions or recommendations please share them.

dawn-minion commented 4 years ago

@wilkinsw Could we not have an optional data type field, though? So if it's provided the Buffer is run through rawToType, otherwise you just get the same thing back. In our case we always know the datatype in advance, since we do not always use an EDS. Same with the download method if possible, too.

Only other suggestion at the moment would be being able to specify the send and receive methods for SDO. We're using this module to do CANopen via socketcan directly, as well as forwarding it to a CANOpen device via WiFi, a remote server, and bluetooth. When we switch to something not socketcan, we just implement a send and a fake addListener method that passes received data back to this module. This works great, but, we're sort of masquerading ourselves as the socketcan module. Thought being able to specify the methods might be prettier :)

Apart from that, I really like the refactor - not having to create entries for everything is quite nice. I'm hoping to convert the project over to v2 in the next week or so and can provide more feedback as we go along.

wilkinsw commented 4 years ago

That sounds reasonable, I have some other things to take care of and then I'll be back to this

wilkinsw commented 4 years ago

v2.2.0 beta changes the SDO upload/download prototype to take an arguments object; you can now provide dataType and it will attempt to convert the Buffer for you.

I also replaced the socketcan RawChannel object with callbacks as you suggested. https://github.com/DaxBot/node-canopen/blob/a28c4b2375b14442efcb3c0d23d663bae3b79e5c/examples/sdo_client.js#L41-L47

dawn-minion commented 4 years ago

Closing this one, since the original requests were implemented, thanks!