Do you welcome code improvement?

[ladislas]

Adding Fusion to our project, it is now being analyzed by Sonarcloud and around 30 issues have been discovered.

Such as using reference to const to pass large objects.

Do you welcome PRs to fix those?

[xioTechnologies]

I welcome suggestions and discussions for improvements. Unfortunately, the one example you have suggested is not a fix or improvement. The reason it is preferable to pass large objects by reference is because for a normal function call, the whole object would need to be copied to the stack. If the object were instead passed by reference then then only a pointer would need to be copied to the stack. This would consume less of the stack and achieve faster execution.

The reason this is not a fix or improvement here is because the function is declared static inline within the header file. The function will therefore be implemented inline for each call so that no data is copied to the stack. Furthermore, the inline implementation used here and for all functions in FusionMath.h enables significant efficiencies to be achieved by the optimiser.

For example, consider the following function call. The optimiser will reduce this to 3 floating-point operations. If the function was not inline and instead received the values by reference then it would compile to 18 floating-point operations, most of them being multiplications by zero or one.

sensor = FusionCalibrationInertial(sensor, FUSION_IDENTITY_MATRIX, FUSION_VECTOR_ONES, offset);

These optimisations have been verified through profiling. The above line compiles to 25 instructions for a MIPS processor with FPU. This increases to 67 instructions when the function is not inline. That's a 270% improvement in execution speed for a line of code that may be called thousands of times a second in perpetuity on an embedded system.

A reduction in code efficiency is often preferable if it means enhancing readability, conforming to conventions, etc. However, in this case, I believe the SonarCloud suggestion is essentially bad advice that results from the analysis being not quite intelligent enough. I suggest that you implement rule exceptions if possible and where appropriate.

Are there other SonarCloud suggestions that you believe may be improvements to Fusion?

[ladislas]

Thanks a lot for the detailed feedback.

Are there other SonarCloud suggestions that you believe may be improvements to Fusion?

You should be able to see them here, but after reviewing them, they don't all seem relevant based on your previous explanation.

https://sonarcloud.io/project/issues?directories=spikes%2Flk_sensors_imu_lsm6dsox_fusion_calibration%2Ffusion&resolved=false&types=CODE_SMELL&pullRequest=1245&id=leka_LekaOS

The reason this is not a fix or improvement here is because the function is declared static inline within the header file. The function will therefore be implemented inline for each call so that no data is copied to the stack.

I don't know about C, but in C++, inline is considered a hint at the compiler to inline the function, but the behavior depends on optimization and the compiler is always free to do as it wants. Using __attribute__((always_inline)) on the other hand will force inlining.

How can I make sure that on my hardware, STM32F769 (ARM Cortex M7), the functions are inlined and that I can benefit from the optimization you're talking about? How would I benchmark forced inline vs maybe inlined?

[xioTechnologies]

Your description of inline applies to both C and C++. __attribute__((always_inline)) was used in older versions of the library. However, this was converted to inline because profiling indicated identical performance, and it is generally preferable to guide the compiler towards optimisations rather than force it.

How can I make sure that on my hardware, STM32F769 (ARM Cortex M7), the functions are inlined and that I can benefit from the optimization you're talking about? How would I benchmark forced inline vs maybe inlined?

I suggest that when using a mature, third-party library, best practise is to trust the authors and use the library as is. The library code should remain identical to the source (including white space) after integration to your project. This allows updates to be quickly reviewed as diffs, simplifies pull requests, and allows any reader of the code (including yourself in the future) to easily verify the library against the source.

I have created the temporary branch, no-inline that removes the static inline optimisations from FusionMath.h. I suggest you benchmark the performance of this branch against main by profiling the function FusionAhrsUpdateNoMagnetometer. It is important that the function is called using real sensor data and not constant values, and that the result is averaged over several seconds. I have just done this for the PIC32MZ2048EFG100 and found main executes in 4.22 us, and no-inline executes in 7.76 us. In this case, the optimisations allow the algorithm to execute almost twice as fast. I would be interested to hear your results.

Thank you for sharing the SonarCloud issues. Most of the suggested changes are not applicable. For example, using, the reference declarator (&), enum class, and std::varient do not exist in C. I believe that "make the type of this parameter a pointer-to-const" refers to lines 97 and 318. This has been fixed in 4b0bd28.

I disagree with the suggestion to add a default case to the switch statement. A switch statement operating on an enum and implementing all values should not include a default case so that the compiler is able to provide the "enumeration value not handled in switch" warning. Adding a default case would inhibit this warning and prevent the compiler from detecting errors.

I do not have access to see which lines of code the following issues refer to. Please can you confirm.

• Remove the commented out code.
• Define each identifier in a dedicated statement.

[ladislas]

Your description of inline applies to both C and C++. attribute((always_inline)) was used in older versions of the library. [...]

Makes perfect sense.

I suggest that when using a mature, third-party library, best practice is to trust the authors and use the library as is. [...]

This is exactly our workflow for the different external libraries that we use, including yours. And I can say from experience that it has saved us a lot of time and headache!

I have created the temporary branch, no-inline that removes the static inline optimisations from FusionMath.h. I suggest you benchmark the performance of this branch against main by profiling the function [...] I would be interested to hear your results.

I'll try to do the benchmark tomorrow or the day after, it's something we wanted to do anyway to measure the impact on our whole system.

Thank you for sharing the SonarCloud issues. Most of the suggested changes are not applicable. [...]

Yes, our project is written in C++ with a few parts in C, so Sonarcloud is configured that way.

If you were to use Sonarcloud for Fusion, you could set it up to only do C, avoiding all the C++ suggestions.

I disagree with the suggestion to add a default case to the switch statement. [...]

Funny you say that, I had the exact same reasoning during a code review last week. I've actually changed the parameters of Sonarcloud to say that but forgot turn off the previous one, hence the "wrong" suggestion.

I do not have access to see which lines of code the following issues refer to

Those were for our codebase and have been fixed.

Thanks again for the time you take answering all my questions. I've learned a lot and it pushed me to dive deeper into your code and the math behind it. I cannot say I'd be able to do it on my own from the top of my head, but it gave me a better understanding of the whole subject and in the end made me a little better developer than I was before.

I'll keep this open till I can do the benchmark and report the results here.

[xioTechnologies]

The temporary branch, no-inline has now been removed. Thank you for your comments. The project is improved by discussions like this, even if the code is not changed.