Coding Standard refactoring

[Sickeroni]

TL/DR

I read the Coding Standards to contribute code to the Project. After reading it I found (in my opinion) not optimal rules.

abbreviations

• CCG - C++ Core Guidelines

the rules

Filenames (optional):

even trough the CCG allows .h [CCG-SF.1]. It is good practice to use differending endings like .hpp or .hh to differential between C-Headers, C-only-Headers and C++-only-headers.

• one of many stackoverflowdiscussions
• example of a boost-header with templated code

Capitalization

okay but the reason to use camelCase is not valid. Namespaces are there to prevent namingproblems. Thats why we use C++ not C. The Rule should then not allow camelCase but a new rule should force using of namespaces.

Language Features

• Should be updated for newer Marlinversions to get access for newer compiler and languagefeatures. Arduino 1.6.8 is from 9 march 2016.

• Dissallowing STL is really a bummer. I understand the conflict that big parts of the standard are note usable in bare metal but to forbid useful, working, powerful parts makes no sense.

Primitive Types

First rule is really good. I'm missing it in almost every project! Second rule is is suboptimal. Because of roundingerrors floating point datatypes should be forbidden.

• just check 0.1f+0.2f. For every positioning or other things integers must be used. (didnt checked the usage in code)

Memory Usage

if macros are forbidden (later in this issue) a static can be annoying with enabled compilerwarnings

-Wunused-function

Warn whenever a static function is declared but not defined or a non-inline static function is unused. This warning is enabled by -Wall.

SRC

Minimize Repetition

• seriously. This example and the rule are the worst part in the document. You are dividing understandable code to a less good readable one? just to save 18 (19characters saved - new newline) Characters? Even in UTF8 it is nothing. That is not clever. Its overengineered without benefits.
• Macros are bad. They are not debuggable in Debuggers. Use Variables, Enums etc. CCGEnum.1 Even trough some Controllerboards use Debugpins for other functions there are some that work. Other ones can be misused.

  Best-Practices for #include

  you can save many problems with namespaces and a filestructure that mirrors it and with #pragma once (include only one time). developer discipline is still needed but static tools can help

Preprocessor directives

we use C++11. #define is the enemy of every C++-Coder.

• #define instead of const?
  1. const is a extrem helpful. If you need a runtimevariable that should be not rewritten you use it. That can happen in specific structs.
  2. Language Features saidsomething about constexpr. Use it and not #defines. You get the benefits of #define but with debuggable code. already mentioned but C++11 > C
• as already linked. Enums > Macros aka CCGEnum.1
• not about this topic but arguments are still valid [CCGES.30]
• Don’t use macros for constants or “functions” [CCGES.31]

Macros

• already said something about macros
• already said something about floating point numbers

Math Macros

same as Macros

Adding a New Feature

i don't want to repeat myself.

[Roxy-3D]

CamelCase has a lot of fans... Myself included. And it would be nice to get rid of floating point just because of the speed issues associated with its use. But we need it for both the precision and the dynamic range of the numbers.

[p3p]

Macros are bad

I'd run while you still can before thinkyhead sees this, joking aside we need Macros and defines in Marlin for the level of control over the build and what code is even parsed by the compiler, in the future constexpr ifs(c++17) could do a much better job (and multiline constexpr function from c++14) but at the moment we are stuck with the evil that is the preprocessor.

[thinkyhead]

While it's certainly important to hear differing opinions on how Marlin should have been coded in the past and how it should evolve going forward, it's always better to have the actual code changes submitted for evaluation as pull requests so we can see the results and whether they keep the code lean and fast for the sake of 128K MCUs. Simply saying "this and that could be better" is unlikely to get us to stop the presses and rewrite the existing code when we have so much on our plates. If you have good ideas, join the project and help make the needed changes.

[Sickeroni]

CamelCase has a lot of fans... Myself included. And it would be nice to get rid of floating point just because of the speed issues associated with its use. But we need it for both the precision and the dynamic range of the numbers.

• i understand the need for dynamic ranges but the where do we need this? bigger printers need more steps? that is known at compiletime and then we must use something ugly like a 64 or 128bit integer. That should be still fast enough compared to Floats.
• on the other side we dont have that precision on floats. [source] (its easier to copy this proposal with its explaination on this topic)

I'd run while you still can before thinkyhead sees this, joking aside we need Macros and defines in Marlin for the level of control over the build and what code is even parsed by the compiler, in the future constexpr ifs(c++17) could do a much better job (and multiline constexpr function from c++14) but at the moment we are stuck with the evil that is the preprocessor.

I don't understand why if constexpr()is the solution here. Its only helping if only called elements exists. Because i can't explain it that well here some code:

extern int x; // no definition of x required
int f() {
if constexpr (true)
    return 0;
else if (x)
    return x;
else
    return -x;
}

[SRC] constexpr variables and optimization like -O1 are enough. (Arduino IDE uses -Os) proof in Godbolts Compiler Explorer

While it's certainly important to hear differing opinions on how Marlin should have been coded in the past and how it should evolve going forward, it's always better to have the actual code changes submitted for evaluation as pull requests so we can see the results and whether they keep the code lean and fast for the sake of 128K MCUs. Simply saying "this and that could be better" is unlikely to get us to stop the presses and rewrite the existing code when we have so much on our plates. If you have good ideas, join the project and help make the needed changes

My intention here was to see the feedback on this topic. If people have complete different opinions on solutions and problems, they cant work together. I just don't wanted to hear "nah we dont want it your way. We love the old one" after working for weeks on changes and my code will definitely break the coding standard of this project.

[p3p]

I don't understand why if constexpr()is the solution here. Its only helping if only called elements exists. Because i can't explain it that well here some code:

It's a solution to having preprocessor like capabilities in the compiler, constexpr ifs can be a 'replacement' for preprocessor ifdefs, they allow the same precision in what code is actually compiled allowing for the architecture and feature specific code paths that Marlin needs while being able to use constants rather than definitions.

Marlin is very reliant on the preprocessor at this point, I don't like that, but I understand why it was designed that way at the time, and how fundamentally intertwined it currently is but if you could remove the preprocessor usage from Marlin and maintain binary size and performance across all architectures and configurations while allowing us to do runtime configuration on the more powerful boards I'm pretty sure we would all be very happy.

[thinkyhead]

If people have complete different opinions on solutions and problems, they cant work together.

Everything is mutable. I wrote up the coding standards because there weren't any, and the code style was all over the place. The intent was to formalize the de facto standards which were already prevalent in the codebase and to have something we could refer contributors to instead of having to reiterate over and over.

Since we actually work with the codebase every day, we're hugely aware of its shortcomings, but there are only so many hours in the day, and we have a lot of things on our plates just patching up existing issues, implementing feature requests, and trying to move on from the 1.1.x codebase and get rolling with the unified 2.0.x codebase.

Much of what you suggest is already very much on our minds. We've had a lot of debate and discussion around these things, but as it takes time away from the work we really need to get done right now, I will keep insisting on less talk and more action.

That said, let me briefly go over your points with some first impressions…

• .h versus .hh or .hpp. I'm pretty "meh" about this, since I would prefer all the code within the Marlin folder to be under C++ semantics, but if it helps the compiler then I have no problem with it.
• Capitalization: I'm really not a fan of camelCase. I much prefer lowercase with underscores, as I just find it more readable. Currently this is the most prevalent style in Marlin, so I think we should just stick with it.
• Language features: The 1.1.x branch is meant to be the last version usable with Arduino 1.6.8, and until we retire the 1.1.x branch with the release of 1.1.9 we're not going to go beyond C++11 features. However, for the 2.0.x codebase it would be great to be able to use whatever C++14 (etc.) features we can that make the code better.
• STL: Marlin was originally built in C, not C++, and we have been evolving it slowly to be more C++-centric over the last few years. That said, we still want to be considerate of 128K boards and keep Marlin as small and performant as possible. That has meant leaving out libraries that may bloat the code and expand SRAM-based data structures. If STL can be used without bloating the code and data, that would be preferable. We don't care about objects, per se, we don't care about instances, and we cannot generally use new or malloc.
• Floating-point: We'd like to use fixed-point for some stuff, but sometimes it's better to use float. Fixed-point has rounding errors too. Sometimes we want the float used as input to be printed and give back the same value as the input. Depending on the mantissa, fixed-point can transmute the value even worse than float rounding. Anyway, converting everything away from float is a huge pile of work that simply no one has volunteered for.
• Includes and #pragma once: Sure, now that we can use #pragma once, great, let's use it everywhere we can! We also like to not-include headers (by feature) and in many cases not-compile whole .cpp files (by feature). One area that needs much help is the HAL, which has a lot of redundancies and some confusion about both internal and external dependencies. Up to now I've been staying out of the way so that the HAL could just get to a working state, and then I was planning to get into a proper cleanup after 1.1.9 is released, when we're focusing all attention on 2.0.x.
• Replacing macros: Where macros can be replaced with constexpr, const, etc., that would be great. Sometimes this is possible, and sometimes this is not so simple. Again, if someone wants to get all up in the style thing and truly improve the codebase, we would welcome such contributions.
• Avoid #define: We're not going to change all the configuration files to use constexpr in place of #define at this stage. We rely on the preprocessor to only compile the code we want and keep everything else out. There's a lot that the compiler can do to optimize things out, but #ifdef can keep the compiler from wasting time on it in the first place and speed up compile time.

So anyway, you're not the first (and I'm sure won't be the last) to point out all the things we wish we had done yesterday but (due to the linear nature of time) have not yet gotten around to doing. If you look back on Marlin 1.0.0 and follow its evolution over the last 5 years you will see that a huge amount of work has gone into establishing some basic standards, cleaning up the code, trying to make its patterns more evident, and waiting for Arduino IDE to catch up and allow the use of C++11 features. Right now we are at the very tail end of a two year process of molding the 1.1.x codebase into a final form so that we can move on and get into architectural improvements. I've personally wanted to rewrite all the LCD code for over a year, for example, but simply haven't had the time and space to delve into it.

If, as it appears, you would like to help us to modernize the code-base while also keeping it lean and performant, please get on board and start making iterative improvements to the bugfix-2.0.x branch. We will evaluate them on their own merits and accept any and all that make clear improvements. Just, please, don't be one of those people who is always pointing out how stupid things (and by extension, we) are.

[thinkyhead]

Here’s the video I was thinking of, apropos to this topic, that every Marlin developer should watch. Using C++17 (and in some cases 14 and 11j to write code that compiles with the least possible overhead, even when using the stdlib. The example used is a Pong game on the Commodore 64, with intermediate compilation to X86 Assembler. This is the video that got me using const to an obsessive degree, certainly more than is necessary, to help the compiler optimize wherever it can. In particular the use of a const std::array in the video has a dramatic effect compared to a non-const instance. (And with C++17 a constexpr Instance will also be possible.)

https://youtu.be/zBkNBP00wJE

[Roxy-3D]

.h versus .hh or .hpp. I'm pretty "meh" about this, since I would prefer all the code within the Marlin folder to be under C++ semantics, but if it helps the compiler

.h is fine for both C and C++. There is no reason to do anything different.

Floating-point: We'd like to use fixed-point for some stuff, but sometimes it's better to use float. Anyway, converting everything away from float is a huge pile of work that simply no one has volunteered for.

Soon, with 32-bit platforms we can just declare everything as double. I'll take that over fixed point.

Avoid #define: We're not going to change all the configuration files to use constexpr in place of #define at this stage.

Agreed...

Includes and #pragma once: Sure, now that we can use #pragma once, great, let's use it everywhere we can! We also like to not-include headers (by feature) and in many cases not-compile whole .cpp files (by feature).

If, as it appears, you would like to help us to modernize the code-base while also keeping it lean and performant, please get on board and start making iterative improvements to the bugfix-2.0.x branch. We will evaluate them on their own merits and accept any and all that make clear improvements. Just, please, don't be one of those people who is always pointing out how stupid things (and by extension, we) are.

crickets???

[thinkyhead]

Soon, with 32-bit platforms we can just declare everything as double. I'll take that over fixed point.

With many (if not all) of the ARM chips, interrupts cannot use the FPU. So, fixed-point would still be optimal within the stepper ISR and other interrupt contexts.