embedded-office / canopen-stack

Free CANopen Stack for Embedded Systems
https://canopen-stack.org
Apache License 2.0
387 stars 117 forks source link

Documentation Bug on StringObject #70

Closed nebulorum closed 2 years ago

nebulorum commented 2 years ago

Describe the bug

In the example in docs/docs/usecase/dictionary.md the type is incorrect:

  const CO_STRING {
      0,                  /* variable for read position     */
      &DemoString[0]      /* start address of string memory */
  } DemoStringObj;

It should be CO_OBJ_STR also on Mac using const will result in bus error when reading since the Offset has to be updated on a a read operation. Updating an object like this on a read seems odd (more below).

I also think the Domain example is incorrect.

Additional context

This made me look at the code and question why this object is needed and why the offset is this object. It is not thread-safe, and if two SDO server interleave the read the offset cannot be relied upon. So my reading is that :

  1. Some protection exist to avoid concurrent and inter-leafed reading of large objects.
  2. OR users of the large object read functions have to reset the offset before each read, which again creates are temporal coupling and potential risk.

I'm not fully sure how these object should be used, but my intuition is that a CODictRdBuffer call should should include some sort of descriptor by reference (like the FD in files). The "FD" would allows you to control that reader offset, indicate if end of buffer was reached, etc. It also make each reader responsible for keeping its own offset. The API is kind of strange, if the buffer I use is too small I don't know how many bytes were copied? Also from string the \0 is not added.

Again this last comment is based on just writing tests and getting unexpected behaviour. I could be missing the large picture.

michael-hillmann commented 2 years ago

First of all, the documentation needs to be corrected. Thanks for identifiying them in this issue!

Regarding the concurrent access of single object entries you are right: The concurrent access to object entries is not protected in the library itself. The reason is to keep the library independent from RTOS, hardware and possible existing mechanisms in multi-core environments.

I'm more than happy if we could identify a unique mechanism, which allows us to provide the protection in tiny microcontrolers, on multi-core processores with and without an RTOS.

In practise, there are only rare situation where this protection will help to avoid errors. In my projects I tend to protect the potential candidates at the calling location by critical sections (which disables interrupts), locking scheduler (which disable OS scheduling) or avoiding the concurrency at all (e.g. by setting up an object access queue in my RTOS).