search

MarlinFirmware / Marlin

Marlin is an optimized firmware for RepRap 3D printers based on the Arduino platform. Many commercial 3D printers come with Marlin installed. Check with your vendor if you need source code for your specific machine.
https://marlinfw.org
GNU General Public License v3.0
16.31k stars 19.24k forks source link

Coding Standards #1306

Closed thinkyhead closed 9 years ago

thinkyhead commented 9 years ago

Thought it would be a good idea to start a thread deciding on coding standards and how to keep the code tidy as we go along. Personally, I'm an obsessive code-tidier, preferring to be able to see more lines of code, keep things pretty concise, but clear, and using comments only to clarify what might be confusing. I have used Doxygen comment-based code documentation in the past to produce very nice code documentation, which helps to provide a bird's-eye view of the project structure. (I'm not sure if Github automagically parses Doxygen comments, but wouldn't it be cool if they did? That would provide a class navigator combined with documentation, which is why I have used Doxygen to do in the past.)

Anyways, let me propose some standards, and some ways of making any initial code cleanup as painless to the project as possible. At first, the point is just to produce a small section of bullet-points for the README document that will be clear and available for all.

The point of coding standards isn't to become indentation-nazis or style-nazis, or to create another bother for developers. So, I propose:

  1. That some developer(s) (initially, me) should take responsibility to do occasional sweeps and submit "very small" pull requests (under 10 changes to "active" code) at a time, implementing standards without altering code behavior.
  2. That collaborators ask contributors who submit code that obviously doesn't fit in with the code around it to clean it up and resubmit, pointing them to the standards section of the README. (At the moment, the standards are based on the existing code, so standards basically differ from file to file).

As for the standards themselves, at this time I propose just a few:

The documentation in the README can show illustrated examples....

gregrebholz commented 9 years ago

I'll go along with whatever, but I prefer to avoid underscores in naming, and 2-space indents I find harder to rapidly scan than 4 space indents. Maybe consider picking a previously published style guide (e.g. http://www.maultech.com/chrislott/resources/cstyle/) so it can be included by reference instead?

thinkyhead commented 9 years ago

Either indentation is fine with me. Screens are pretty wide now, but they seem to be getting shorter every month...

Literal spaces seems to be the better choice at this point with the Marlin code (rather than tabs set at 4 spaces) because most code is using spaces (with only a couple of exceptions). (As long as it's not 8-space tabs - You have to use those for 6502 assembler!)

I agree, if most variables in the code now, and most standards (I will post links to some good style-guides later) are using camelCaseNaming then I think we should stick with that. I've used UpperCamelCase for methods names, but I have been told that's sometimes uncool. I found that it didn't cause confusion with Class names (since in C++ you never call Constructors directly as functions). For method naming, we can just just stick with current conventions as they stand in each class.

boelle commented 9 years ago

+1 from me on this idea... and might i add that we cant get started to fast :-D

2015-01-04 8:45 GMT+01:00 Scott Lahteine notifications@github.com:

Thought it would be a good idea to start a thread deciding on coding standards and how to keep the code tidy as we go along. Personally, I'm an obsessive code-tidier, preferring to be able to see more lines of code, keep things pretty concise, but clear, and using comments only to clarify what might be confusing. I have used Doxygen comment-based code documentation in the past to produce very nice code documentation, which helps to provide a bird's-eye view of the project structure. (I'm not sure if Github automagically parses Doxygen comments, but wouldn't it be cool if they did? That would provide a class navigator combined with documentation, which is why I have used Doxygen to do in the past.)

Anyways, let me propose some standards, and some ways of making any initial code cleanup as painless to the project as possible. At first, the point is just to produce a small section of bullet-points for the README document that will be clear and available for all.

The point of coding standards isn't to become indentation-nazis or style-nazis, or to create another bother for developers. So, I propose:

  1. That some developer(s) (initially, me) should take responsibility to do occasional sweeps and submit "very small" pull requests (under 10 changes to "active" code) at a time.
  2. That collaborators ask contributors who submit code that obviously doesn't fit in with the code around it to clean it up and resubmit, pointing them to the standards section of the README.

As for the standards themselves, at this time I propose just a few:

  • Indentation: 2 spaces. All blocks indented, including #if blocks and other non-brace compiler blocks
  • Bracket-style: "1TBS" - Almost all opening braces at the end of lines, including declarations
  • Comments: Doxygen-style headings (documenting in .h files), C++ single-line style // for under 3 lines
  • Comments: Multi-line use asterisks in the second column
  • Capitalization: ClassMethod, classData (or class_data?), localVar (or local_var?)

The documentation in the README can show illustrated examples....

— Reply to this email directly or view it on GitHub https://github.com/ErikZalm/Marlin/issues/1306.

boelle commented 9 years ago

thoughts... since we only have 1 issue left to clear the first milestone and it has been done in far less time i thought of first...

should we add more issues to the first milestone... and then set a new one which will only deal with code cleanup ?

boelle commented 9 years ago

anyway i added a few more issues to the first milestone... there are about 55 days left

lets see how many we can get cleared

https://github.com/MarlinFirmware/Marlin/milestones/Bug%20Fixing%20Round%201

gregrebholz commented 9 years ago

Yeah, true space indentation seems to be the norm for most programming these days, and a decent editor will convert keypresses and auto-tabbing to spaces automatically.

Re: moving the milestone, I wouldn't. I have not been employed as an 'agile developer' but I believe the philosophy is to set short, relatively easy to obtain goals, achieve them, throw yourselves a party, and then set new goals. So maybe the 1.0.2 release isn't everything it should be, but at least it's more stable than 1.0.1, and 1.0.4 will be better, and maybe that's ready in only 30 days... but the idea is to keep setting short goals and accomplishing them I think.

boelle commented 9 years ago

oki... its easy to remove the issues again...

so will remove them and only keep that single one

alexborro commented 9 years ago

+1 for coding standard.. The current code is quite a mess..

I'm studying the planner.cpp and stepper.cpp these days and found A LOT of

ifdef... Many barely used features messing all the code. Is there a better

way to do this, I mean, avoiding #ifdefs? I don't think so but it is worth asking experienced guys.

Cheers.

Alex. Em 04/01/2015 13:23, "Bo Herrmannsen" notifications@github.com escreveu:

oki... its easy to remove the issues again...

so will remove them and only keep that single one

— Reply to this email directly or view it on GitHub https://github.com/ErikZalm/Marlin/issues/1306#issuecomment-68636387.

gregrebholz commented 9 years ago

Well, Marlin's use of the #ifdef is to get minimum binary size for the Arduino, and that should also be possible to some extent with compiler optimizations to remove unreachable code. For example, if you had "if (0) { // 1000 lines of code; }" that should not add to the binary size at all. Likewise if you had "if (feature) {" and feature was a "static const char" and initialized with a 0. Same for enums. And maybe even if feature was not const but was never part of an assignment operation. How far we can take that toward readability without affecting code size would require tests to see how clever the avr-gcc optimizations are.

There's also the concept of "dead code" removal in compiler optimizations which can pare down the binary size. That's where some computation is done but the results are never used. (i.e. if you're doing some delta geometry math, but the use of those results were limited to unreachable code which was removed, the compiler can remove the delta geometry math too). While in theory that could remove the need for some #ifdef use, the actual implementation in the compiler has to be very conservative to avoid altering the behavior of the program. The classic example is some "a = b/c;" where a is never used. It looks like perfectly dead code, but the fact that the division operation can throw divide-by-zero exceptions means the code can't be removed without changing the behavior of the software, so the compiler leaves it in.

The trouble with all this is it has to be tested specifically to determine effects on binary size, whereas pre-compiler directives are very obvious, assuming you know the value of all the defines as you're reading. There's so many now in Marlin that's very difficult, unfortunately. But because we're dealing with extremely tiny amounts of memory, just about every byte counts so we can't ignore the problem.

I'm not sure what tradeoffs are worth making between readability and code size. For another example, the eeprom code only allocates space for certain variables based on the features like SCARA or DELTA, rather than a more easily read version that would just have one data structure in eeprom and code that either used the variables or not. We're talking about saving tens of bytes in the eeprom, which probably isn't worth the trouble. Or maybe the added memory required at runtime to hold the values is a problem. Only way to find out is testing.

galexander1 commented 9 years ago

I'm a big advocate of putting things in their own functions:

ifdef MAGIC_HOMING

 magic_homing();

endif

ifdef SUPER_HOMING

 super_homing();

endif

a few lines of readable code instead of .. well, instead of what's there. A lot of these functions have the sets of ifdefs interleaved, i.e., MAGIC_HOMING then SUPER_HOMING then MAGIC_HOMING then SUPER_HOMING. There are a few factoring approaches for that, and with a little creativity we can turn that into readable code too..

As far as whether that violates our timing constraints, that is an interesting question. For a lot of this stuff, it obviously doesn't matter, but for the stuff that is in the path of an interrupt handler, I could imagine it being pretty sensitive. OTOH, I think we're using -O all around, so simple functions will be inlined. And complicated functions benefit even more from being factored out..

nophead commented 9 years ago

It's C++, so just declare the functions inline to get zero overhead.

galexander1 commented 9 years ago

heh. I do C++ compiler for a living. inline keyword has a semantic meaning, guaranteeing it is possible to inline it (a definition is available to every compilation unit that calls it), but has little to no effect on the decision of whether it is actually inlined.

So it is probably a good idea, but hardly a guarantee.

On Mon, Jan 05, 2015 at 06:45:23AM -0800, Chris wrote:

It's C++, so just declare the functions inline to get zero overhead.

Reply to this email directly or view it on GitHub: https://github.com/ErikZalm/Marlin/issues/1306#issuecomment-68716507

nophead commented 9 years ago

I also use to write C++ for a living both embedded and on Windows and Linux. It is not guaranteed but I think as long as it small or is only called once then it will be as that gives both the smallest and fastest code. When optimised for space it may be not inlined if it is longer than the call overhead and called multiple times. Also code compiled for debugging tends to not inline.

alexborro commented 9 years ago

Using function like @galexander1 said is a great idea. It makes the code way more simple with almost no drawbacks.

I think it will be nice adding to the coding standard.

Cheers.

Alex.

whosawhatsis commented 9 years ago

Strongly agree about functions. The firmware retraction code was terrible to try to read until I broke it out into a function.

I think that every G/M code should point to a named function like this. That giant switch statement is a nightmare.

gregrebholz commented 9 years ago

Second the idea of functions for each G/M code. It's what I expected to find the first time I dove in anyway. I'm reasonably confident that defined functions that are never called get left out of the binary too, so there isn't even a need to ifdef around them. If we end up with a few different functions based on compile-time features we can have a single function for the code that does pre-processor ifdef work so only one of the resulting functions is called. The compiler should strip out the rest.

galexander1 commented 9 years ago

The only caveat about the compiler stripping unused functions is that it can only strip them if they are marked 'inline' or 'static'. Otherwise, we're counting on the linker to strip them out after the fact, which...well, it's possible, I haven't checked.

stv0g commented 9 years ago

I've three more suggestions for a coding standard:

lionello commented 9 years ago

My 2c: don't use #defines for old code. We have GIT for that. Code that's old/obsolete/superseded should just be removed, preferably in a single commit with an explanatory commit message.

stv0g commented 9 years ago

I started writing a markdown document which we can put in the root directory of the repo. Before creating a pull request, we shall work on it collaboratively:

https://stackedit.io/editor#!provider=couchdb&id=STVTTU4IKyAWE9mCJvWpvqik

bkubicek commented 9 years ago

please consider that the avr gcc chain driven from arduino does not compile using optimizations. Hence it is often necessary to have compromise. I hate the ifdefs, but I think they are necessary evil. I hate that in the gcode parser, there it the actual code and no named functions. But, I think that is necessary, until somebody with a disassembler proves otherwise.

Bernhard

On Wed, Jan 7, 2015 at 10:37 AM, Steffen Vogel notifications@github.com wrote:

I started writing a markdown document which we can put in the root directory of the repo. Before creating a pull request, we shall work on it collaboratively:

https://stackedit.io/editor#!provider=couchdb&id=STVTTU4IKyAWE9mCJvWpvqik

— Reply to this email directly or view it on GitHub https://github.com/MarlinFirmware/Marlin/issues/1306#issuecomment-68998380 .

stv0g commented 9 years ago

My Arduino version (1.0.6) compiles space-optimized code:

Compiler:

[...]/tools/avr/bin/avr-g++ -c -g -Os -Wall -fno-exceptions -ffunction-sections -fdata-sections \
-mmcu=atmega2560 -DF_CPU=16000000L -MMD \
 -DUSB_VID=null -DUSB_PID=null -DARDUINO=106 [...]

Linker:

[...]/tools/avr/bin/avr-gcc -Os -Wl,--gc-sections,--relax -mmcu=atmega2560 \
-o \[...]//build5106915976840868010.tmp/Marlin.cpp.elf [...] \
/build5106915976840868010.tmp/BlinkM.cpp.o
[...]
 -L/var/folders/p6/hg40p3rs7qxclyx44_mvng840000gp/T/build5106915976840868010.tmp -lm

But there are compiler options used -ffunction-sections and -Wl,--gc-sections which are far more interesting:

Don't include unused function and data -ffunction-sections -fdata-sections

Generally used with --gc-sections. This causes each function to be placed into a separate internal memory section, which --gc-sections can then discard if the section (function) is unreferenced. Those two together will only help if you have a bunch of unused functions in your code, such as when you are using a library compiled with the option and linked statically. Note that if you have any "naked" functions in the placed in an .initN or .finiN section, you should also mark the function with a "used" attribute to prevent it's being optimised out. (Thanks to David Boone.)

Source: http://www.tty1.net/blog/2008/avr-gcc-optimisations_en.html

stv0g commented 9 years ago

Conclusions:

boelle commented 9 years ago

me happy as soon this is not the first logo for the firmware: http://vecto.rs/1024/vector-of-a-cartoon-injured-guy-using-a-laptop-in-a-wheelchair-coloring-page-outline-by-ron-leishman-15533.jpg

2015-01-07 11:08 GMT+01:00 Steffen Vogel notifications@github.com:

Conclusions:

  • we don't have to care about it [image: :smile:]
  • we only need #ifdefs for the first invocation, sp that the code isn't referenced and gets removed

— Reply to this email directly or view it on GitHub https://github.com/MarlinFirmware/Marlin/issues/1306#issuecomment-69001686 .

galexander1 commented 9 years ago

Cheap function calls are one of the first innovations of compilers.

http://repository.readscheme.org/ftp/papers/ai-lab-pubs/AIM-443.pdf

If we can't assume cheap function calls we would be using assembly.

Looking at the generated code, it is not perfect, but gcc is only saving the "registers" that are used, and there is no big boilerplate prologue/epilogue. If it is a simple function, the call is cheap. If it is a complicated function, even inlining it is expensive.

Also, I added a small function just to see, and it got inlined. I'm using the Makefile -- if the Arduino IDE is defective then we need to learn how to configure the IDE, that's hardly an excuse for bad coding practices.

Anyways, most code is not actually performance critical. For example, the various G28/G29 paths are in desperate need of refactoring and are absolutely not critical. We will need to make sure the ISRs don't get slower, and that Marlin as a whole doesn't get much bigger, but there is definitely no reason for the un-factored mass of #ifdefs in general.

ErikZalm commented 9 years ago

One advantage of using #ifdef compared to if(static value) is that it is clear that it is not dynamic. I know stuf needs refactoring. But the avr compiler is not the best one. Be careful to change things in critical loops. Always check the compiler output. The compiler bit me many times in the past.

galexander1 commented 9 years ago

Oh I would never advocate for if() instead of #ifdef -- compilers are spotty at best when it comes to constant evaluation -- I would just advocate for factoring either way. Either wrap the call to the function or the body of the function in a #ifdef, depending on what makes sense in that case.

alexxy commented 9 years ago

There is a nice tool that can help to autofromat code. Its called uncrustify.

thinkyhead commented 9 years ago

@stv0g Been adding to / editing the StackEdit document. I might add more notes about the macros and other utilities that this codebase uses specifically. Also about single-line if blocks and ternary operator style.

thinkyhead commented 9 years ago

@alexxy I've done some code formatting automation, but mostly editing by hand because I want to know the code pretty intimately and to make adjustments along the way. Those who maintain specialized forks in sync with MarlinFirmware tend to find it annoying to merge sweeping formatting changes, so except for the more stale code, we've historically tried to introduce formatting changes gradually (in "stale" code regions where not much change ever happens). The code's getting a little more pretty every day, but a few of the more central files need some attention. I'm just about done with a major cleanup to temperature.cpp and after that will focus on planner.cpp and ultralcd.cpp.

github-actions[bot] commented 2 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.