search

Daxbot / node-canopen

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

wrong cobId Tx and Rx in SDO upload #44

Closed jaghatei closed 6 months ago

jaghatei commented 6 months ago

I recently upgrade an old project from canopen 2.8.2 to 6.0.0 and found all SDO uploads failing with timeout. The reason is that the cobIdTx and cobIdRx are in version 6.0.0 no longer automatically enriched with the server node id. CAN traffix while SDO timeout in canopen 6.0.0:

(2024-03-31 21:41:42.263225) can0 600 [8] 40 E6 55 02 00 00 00 00 (2024-03-31 21:41:42.294210) can0 600 [8] 80 E6 55 02 00 00 04 05

CAN traffic without SDO timeout in canopen 2.8.2:

(2024-03-31 21:47:04.374661) can0 620 [8] 40 E6 55 02 00 00 00 00 (2024-03-31 21:47:04.375400) can0 5A0 [8] 41 E6 55 02 2A 00 00 00 ...

possible workaround in 6.0.0 is to call device.eds.addSdoClientParameter(0x20, 0x600|0x20 ,0x580|0x20); to manually set correct effective cobId(Tx/Rx), instead of device.eds.addSdoClientParameter(0x20, 0x600 ,0x580);

if looks odd to me that you are checking like this if serverId already has been applied to cobId(Tx/Rx):

            if ((cobIdRx & 0xF) == 0x0)
                cobIdRx |= deviceId;

I assume this more should be 

            if ((cobIdRx & 0x7F) == 0x0)
                cobIdRx |= deviceId;

as node ids are valid from 1 to 127 and not just from 1 to 15. Please review if this is a bug or a desired behaviour, but if it is desired you at least need to update the SDO examples to reflect the changed call of addSdoClientParameter.

wilkinsw commented 6 months ago

You are correct. I am no longer adding the server node id. As far as I can tell that was a convenience feature added to other libraries (notably CANopenNode which I was testing with). I removed it with the goal of making Eds entries more intentional and not assuming what the user wanted.

I am updating the example as you suggested, and adding another entry to the porting guide on the README to highlight the change.

I fixed the (cobIdRx & 0xF) == 0x0) bug for the deprecated addServer/addClient API. Good catch there. I also removed some vestigial code from upload/download which used to support that sort of "wildcard" SDO server entry.

Change should be reflected in 6.0.1 shortly.