embedded-office / canopen-stack

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

Discussion on possible code size reductions #108

Open jernejsk opened 2 years ago

jernejsk commented 2 years ago

As far as I can see, there are only 2 functions, which set offsets and in all cases, it's set to 0. What is rationale behind this control? Is it remnant of the past or for some future functionality? Removing it would lower code size and simplify stack a bit. While this probably doesn't affect big MCUs much, it can be noticeable on smaller MCUs, like the one I'm working on (proprietary design).

michael-hillmann commented 2 years ago

This function code was used in some special cases a few years ago. Removing it is possible, but will break all existing user-written control functions.

As we are working on an update for 4.2.x, we could announce this change for the next major version 4.3 (with the SDO client segmented transfer). I think we can assume it is a relict and can remove it to gain some resources.

The breaking change will be:

/*!< Control type function prototype (OLD) */
typedef CO_ERR   (*CO_OBJ_CTRL_FUNC) (struct CO_OBJ_T *, struct CO_NODE_T *, uint16_t, uint32_t);

/* will be replaced by */

/*!< Reset type function prototype (NEW) */
typedef CO_ERR   (*CO_OBJ_RESET_FUNC) (struct CO_OBJ_T *, struct CO_NODE_T *);
jernejsk commented 2 years ago

Ctrl handler is also used by COTAsync but it's scope is limited to PDOs, so ctrl handler might still be needed.

Please correct me if I'm wrong, but there are actually two ways that stack signals to reset offset: 1) via ctrl handler, where function argument is set to CO_CTRL_SET_OFF and value to 0 2) via read and write handler, where length parameter is set to 0

Second option is nice and doesn't need any additional handler. What do you think?

michael-hillmann commented 2 years ago

You are right, it is used in asynchronous PDOs, too.

Summary:

On one hand: We may remove the two arguments (function-code and parameter) to eliminate the code required to pass these values into the function during calls. This removes some flexibility for new types we may write as part of the stack in the future. In my opinion, this is acceptable when you can confirm that the code size on your target is reduced after this change.

On the other hand: Removing this function completely removes the ability to add type-specific callback functions before each read/write operation and after a changing write operation. This will eliminate the TPDO feature "autonomous PDO transmission after changed object entry". This is not acceptable until we find an alternative approach to define object-entry-specific callbacks before/after read/write and after object-entry value changes.

Will removing the arguments in the control function solve your code size issue?

jernejsk commented 2 years ago

Removing just parameter saves 136 B of code in my firmware, so not much, but it something. I think I'll get better code size savings by reworking some other parts of the firmware. Thanks for looking into this!

michael-hillmann commented 2 years ago

Can you share your current size and the size you want to achieve? Just to get a feeling on the challenge ;-)

jernejsk commented 2 years ago

Current firmware size without modifications is 65474 B and 65336 B without parameter in ctrl handler. Code space is 16 B less than 64 kB. CANopen stack with dictionary and custom types takes more than half of that. I need a few kBs of space to implement remaining features. There are some reserves by using size optimization level in compiler and some minor things across my firmware, which should be enough. I was just looking for low hanging fruits, like offset management here.

michael-hillmann commented 2 years ago

One idea may be to add a conditional compilation flag USE_SDO_BLOCK (with defaults to 1) in co_cfg.h. When setting this to 0 e.g. via compile switch -D USE_SDO_BLOCK=0, the SDO block transfer is removed from the stack. Is this a possible enhancement for the stack in your project?

Note: this is not implemented right now and needs to be done. But I guess it is fairly easy to do.

jernejsk commented 2 years ago

Thanks for reminding me. While block transfers are useful for speedy firmware updates, they are not essential. I can disable them as a last resort, if there won't be enough space.