In co_handle_rx will different functions be called, depending on the received function code.

However, the function co_emcy_rx will never be called since the value CO_FUNCTION_EMCY is the same as the value CO_FUNCTION_SYNC, which already has been checked.

The correct condition to call co_pdo_sync should be:

else if ((function == CO_FUNCTION_SYNC) && (node == 0))

The public co_api header defines the co_access_fn function type. But when implementing such an access function, you probably need one or more of the CO_SDO_ABORT codes which are only defined in the private co_sdo header.

While implementing CiA 417 object dictionary I noticed that RPDO 261 is specified (CiA 417:3 §A.1) as using COB-ID 0x180 but the stack is not accepting this. And indeed, looking at CiA 301 §7.3.5 that CAN-ID is listed as restricted, not for use by PDOs etc.

Now I wonder, is that a mistake/typo in the CiA 301 document and, by extension, in the stack? Or is it just a temporary loss of brain functionality when they picked COB-IDs for CiA 417?

Regardless, can the check in co_validate_cob_id() be relaxed to support the CiA 417 requirements?

It's a bit inefficient to poll the stack for its timeout handling at 1 kHz.

Suggestion from #3:

It would be nice if the clients of the periodic job feature could say when they are expiring, so that the timer can be armed to the time required instead of periodically. Since all callees in co_handle_periodic() seems to end up using co_is_expired() to check now against a timestamp and fixed period, this should be easily implemented by returning the time left from co_is_expired() and propagating the minimum value up the call chain to co_main() which can re-arm the timer.

Then also the CO_JOB_PERIODIC message must be force sent whenever any event causes a timer to be set, to recalculate the timeout. Maybe co_handle_periodic() can be called every iteration of the co_main() loop to cover all cases where the event passes trough the main thread? Are there others?

The main loop could probably even use the timeout feature of os_mbox_fetch() to wake up at the right time instead of keeping a separate timer around.

hpz420/~/canopen/c-open/c-open 14 cmake --build build --target all check
[ 5%] Built target osal
[ 35%] Built target canopen
[ 41%] Built target slave
[ 47%] Built target slaveinfo
[ 50%] Built target gtest
[ 52%] Building CXX object CMakeFiles/co_test.dir/test/test_sdo_server.o
/home/john/canopen/c-open/c-open/test/test_sdo_server.cpp: In member function ‘virtual void SdoServerTest_BadSubIndex_Test::TestBody()’:
/home/john/canopen/c-open/c-open/test/test_sdo_server.cpp:207:30: error: storing the address of local variable ‘obj’ in ‘mock_co_obj_find_result’ [-Werror=dangling-pointer=]
207 | mock_co_obj_find_result = &obj;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~
/home/john/canopen/c-open/c-open/test/test_sdo_server.cpp:199:18: note: ‘obj’ declared here
199 | const co_obj_t obj = {0, OTYPE_NULL, 0, NULL, NULL};
| ^~~
In file included from /home/john/canopen/c-open/c-open/test/test_sdo_server.cpp:22:
/home/john/canopen/c-open/c-open/test/mocks.h:75:25: note: ‘mock_co_obj_find_result’ declared here
75 | extern const co_obj_t * mock_co_obj_find_result;
| ^~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
gmake[2]: *** [CMakeFiles/co_test.dir/build.make:76: CMakeFiles/co_test.dir/test/test_sdo_server.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:1087: CMakeFiles/co_test.dir/all] Error 2
gmake: *** [Makefile:166: all] Error 2

Fixed by adding "static" at line 199 in test_sdo_server.cpp.

The 1029h Error behavior object allows to configure whether the device should autonomously enter Pre-operational, Stopped or to remain in the current NMT state, after a "serious CANopen device failure". The default behavior, and the only behavior if 1029h is not implemented, is to transition to Pre-operational.

This is implemented correctly by the stack, however the transition (if not disabled) is taken regardless of which error has occurred, for any EMCY that is sent by the application. CiA301 lists the following conditions as mandatory for being "CANopen device failures":

• Bus-off
• Life guarding or Heartbeat time-out

"Severe CANopen device errors also may be caused by CANopen device internal failures". But there is no way to tell that a specific EMCY is not considered severe by the application.

I have a pre-defined PDO with a COB-ID that is defined to be based on the node ID. i.e. something like the following in the od_defaults array (much like the example slave):

	{0x1881, 1, 0x40000480 + DEFAULT_NODE_ID }, /* Enable TPDO 130 */

However, when the node ID is changed via LSS, the COB-ID of course still matches the default node ID, not the new one. I see no possible way to fix this.

I have considered patching the defaults array with the new node ID, however the new ID is not available until after the defaults have been applied.

I have also considered using my PR #46 to update the COB-ID later, in the NMT callback when going Pre-operational and the active node ID has been established. However, since this is (at least during startup) still inside the co_init() call, no client has been possible to create to be able to submit jobs to the co_main thread.

On linux we check for incoming can frames by creating a thread that blocks in select(). When select() unblocks, the thread calls a callback to inform the main thread that there are messages to read, by posting in a mailbox. The thread will then attempt the select() again, which will succeed until the main thread has read the can message. If the main thread is not allowed to run this will cause high cpu-usage and fill the mailbox.

Find a better method to create a callback on incoming messages on linux, or rework the abstraction layer so that incoming messages can be posted to the main thread.

Unfortunately it seems there is no way to use objects of datatype UNSIGNED48, or any other INTEGER or UNSIGNED of size between 32 and 64.

Even for 64 bit integers, there's no way to programmatically handle the access via a co_access_fn.

UNSIGNED48 is widely used for all virtual I/O in e.g. CiA 417, where programmatic access is also needed to take care of the somewhat complicated behaviour of those objects.

If the application triggers PDO transmission using co_pdo_event() or co_pdo_obj_event(), the stack transmits the PDOs regardless of NMT state. It should not send PDOs unless the NMT state is Operational.

In some setups, a TPDO on another node can have slightly different setups depending on the manufacturer/type of the particular device. For example, a device profile may mandate that PDO X contains object A, while some devices are mapping both object A and B in that PDO. Even if we only want to use object A (which is guaranteed to be available) and only have it mapped in our RPDO, the stack throws an EMCY with code 0x8210 just because it receives a PDO with "additional junk" on the end.

This is covered by §7.5.2.36 in CiA 301 which states (emphasis mine):

If a CANopen device receives a PDO that is having less data bytes than the number of mapped data bytes (length), then the CANopen device shall initiate the EMCY write service, if supported, with the error code 8210 h.

And, a little earlier in the paragraph:

If the CANopen device receives a PDO that is having more data bytes than the number of mapped data bytes is (length), then the CANopen device shall use the first data bytes up to the length and may be initiate the EMCY write service, if supported.

So the case with extra data in the PDO is not necessary to write an EMCY for (it does not even state the code to use) and in cases like this also not very helpful. Can the check be disabled for this case, or if it makes sense to have it in other cases, at least be able to disable it by configuration?

It seems standard RPDO padding between mappings as per CiA 301 §7.4.7.1 is not implemented.

For static mappings I guess you could work around it by defining objects like

	{0x0005, OTYPE_DEFTYPE,0,               OD0005, NULL},

with

static uint8_t dummy;

/* Entry descriptor for UNSIGNED8 (0005h) */
static const co_entry_t OD0005[] = {
	{0x00, OD_WO | OD_TRANSIENT | OD_RPDO, DTYPE_UNSIGNED8, 8, 0, &dummy},
};

However to support dynamic mappings you'd basically have to do this for all integer types for all slaves, because you don't know what mapping the master may set up. Seems better if that was built into the stack.

It would be nice if the stack supported MPDO, as required by some CANopen profiles (such as CiA 417).

An updated CAN-ID for a PDO that is setup in the defaults array does not stick even if the communication parameters are stored. This can be demonstrated using the example slave, with the addition of a long delay before exiting main, and the following test script:

# Start the node to see the TPDO, then reset again
cansend vcan0 000#0101
sleep 1
cansend vcan0 000#8101
sleep 1

# Reconfigure the TPDO with CAN-ID 0x456 and duplicate the mapping
sdowrite vcan0 1 0x1800 1 4 0x80000456
sdowrite vcan0 1 0x1A00 0 1 0
sdowrite vcan0 1 0x1A00 1 4 0x20010010
sdowrite vcan0 1 0x1A00 2 4 0x20010010
sdowrite vcan0 1 0x1A00 0 1 2
sdowrite vcan0 1 0x1800 1 4 0x00000456

# Store the communication parameters
sdowrite vcan0 1 0x1010 2 4 0x65766173

# Start the node to see the TPDO with updated CAN-ID and mapping
cansend vcan0 000#0101
sleep 1

# Reset the node, then start again and expect the PDO to be identical
cansend vcan0 000#8101
sleep 1
cansend vcan0 000#0101

candump shows the following output:

  vcan0  701   [1]  00
  vcan0  081   [8]  00 10 81 00 00 00 00 00
  vcan0  081   [8]  00 00 00 00 00 00 00 00
  vcan0  000   [2]  01 01
  vcan0  181   [2]  AA 55
  vcan0  000   [2]  81 01
  vcan0  701   [1]  00
  vcan0  601   [8]  23 00 18 01 56 04 00 80
  vcan0  581   [8]  60 00 18 01 00 00 00 00
  vcan0  601   [8]  2F 00 1A 00 00 00 00 00
  vcan0  581   [8]  60 00 1A 00 00 00 00 00
  vcan0  601   [8]  23 00 1A 01 10 00 01 20
  vcan0  581   [8]  60 00 1A 01 00 00 00 00
  vcan0  601   [8]  23 00 1A 02 10 00 01 20
  vcan0  581   [8]  60 00 1A 02 00 00 00 00
  vcan0  601   [8]  2F 00 1A 00 02 00 00 00
  vcan0  581   [8]  60 00 1A 00 00 00 00 00
  vcan0  601   [8]  23 00 18 01 56 04 00 00
  vcan0  581   [8]  60 00 18 01 00 00 00 00
  vcan0  601   [8]  23 10 10 02 73 61 76 65
  vcan0  581   [8]  60 10 10 02 00 00 00 00
  vcan0  000   [2]  01 01
  vcan0  456   [4]  AA 55 AA 55
  vcan0  000   [2]  81 01
  vcan0  701   [1]  00
  vcan0  000   [2]  01 01
  vcan0  181   [4]  AA 55 AA 55

The last line shows that the TPDO COB-ID reverted to the default after an NMT reset. However, the updated mapping was correctly remembered.

Currently, communication errors are polled for in the CO_JOB_PERIODIC context by calling co_emcy_handle_can_state(). To avoid the need for periodic polling (see #6), the drivers could send events on errors, just like RX is handled.

For embedded controllers they most likely provide the ability to interrupt on error flags.

SocketCAN on linux can be told to send error events through the regular RX path, see https://www.kernel.org/doc/html/latest/networking/can.html#raw-socket-option-can-raw-err-filter for details.

If some platform doesn't support it, there's always the option to fall back to a separate polling thread.

Current rt-kernel coal drivers allow DLC up to 15 on received can frames. However only 8 bytes of data is available in the data buffers. This could lead to out of bounds read.

I can't find a way to use high PDO numbers without setting MAX_TX_PDO and MAX_RX_PDO to those high numbers, which is wasting huge amounts of RAM due to the large arrays. I want to implement RPDO 259, 261 and 263 and TPDO 260, 262 and 385, as mandated by the 417 application profile.

Finding a matching PDO is also inefficient because it's doing a linear search through the whole, almost entirely empty, array.

Is there a way around it or would the PDO code need to use a data structure that scales better?