Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discussion on possible code size reductions #108

Open
jernejsk opened this issue Jul 7, 2022 · 8 comments
Open

Discussion on possible code size reductions #108

jernejsk opened this issue Jul 7, 2022 · 8 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@jernejsk
Copy link

jernejsk commented Jul 7, 2022

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
Copy link
Contributor

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 *);

@michael-hillmann michael-hillmann added the enhancement New feature or request label Jul 12, 2022
@michael-hillmann michael-hillmann added this to the 4.3.0 milestone Jul 12, 2022
@jernejsk
Copy link
Author

jernejsk commented Jul 12, 2022

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
Copy link
Contributor

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

Summary:

  • TPDO: The obj->Type->Ctrl() is used to trigger the transmission when the PDO mappable object entry is changed during writing; function-code := always CO_TPDO_ASYNC, para := unused
  • OBJS: The obj->Type->Ctrl() is used to reset the internal working offset when reading/writing large object entries (domains, strings, parameters, etc.); function-code: always CO_CTRL_SET_OFF, para := new offset (allways set to 0)

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
Copy link
Author

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
Copy link
Contributor

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

@michael-hillmann michael-hillmann removed this from the 4.3.0 milestone Jul 12, 2022
@michael-hillmann michael-hillmann added the help wanted Extra attention is needed label Jul 12, 2022
@michael-hillmann michael-hillmann changed the title Usefulness of CO_CTRL_SET_OFF handling Discussion on possible code size reductions Jul 12, 2022
@jernejsk
Copy link
Author

jernejsk commented Jul 12, 2022

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
Copy link
Contributor

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
Copy link
Author

jernejsk commented Jul 12, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants