That's over-commented code written by a junior developer from my quick look. The first random thing I looked at in grbl/eeprom.c:
...
char old_value; // Old EEPROM value.
char diff_mask; // Difference mask, i.e. old value XOR new value.
cli(); // Ensure atomic operation for the write operation.
...
You can remove the need for the first comment by calling the variable old_eeprom_value. Boom, simple and obvious. Commenting cli() is similarly ridiculous: call the function disable_interrupts() and it's completely obvious what it's doing. Later on:
sei(); // Restore interrupt flag state.
This is incorrect. It's enabling interrupts, not restoring them. If the intent was actually to restore the interrupt disable flag to its original state then this function is buggy and will unintentionally enable them. It would be far better to document the expected sematics in the documentation for the function above, but instead of documenting the expected semantics of the eeprom_put_char() function, you have to read the code to figure out what the semantics are. What would be better is to have a comment in the function description saying "this function can only be called with interrupts enabled" or "this function is atomic and can be safely called from an interrupt handler or with interrupts enabled". Then it's obvious when reading the code which semantics are guaranteed / expected.
So, sure, overly commented code makes it easy to figure things out, but this is a sign of a junior developer that is focused too much on the code and not enough on the overall system. This isn't something I'd like to see a developer pointed at that is looking to learn good habits. Good habits are telling other developer what they can expect from a function. Bad habits are making them read the code to figure that out.
... char old_value; // Old EEPROM value. char diff_mask; // Difference mask, i.e. old value XOR new value.
cli(); // Ensure atomic operation for the write operation. ...
You can remove the need for the first comment by calling the variable old_eeprom_value. Boom, simple and obvious. Commenting cli() is similarly ridiculous: call the function disable_interrupts() and it's completely obvious what it's doing. Later on:
sei(); // Restore interrupt flag state.
This is incorrect. It's enabling interrupts, not restoring them. If the intent was actually to restore the interrupt disable flag to its original state then this function is buggy and will unintentionally enable them. It would be far better to document the expected sematics in the documentation for the function above, but instead of documenting the expected semantics of the eeprom_put_char() function, you have to read the code to figure out what the semantics are. What would be better is to have a comment in the function description saying "this function can only be called with interrupts enabled" or "this function is atomic and can be safely called from an interrupt handler or with interrupts enabled". Then it's obvious when reading the code which semantics are guaranteed / expected.
So, sure, overly commented code makes it easy to figure things out, but this is a sign of a junior developer that is focused too much on the code and not enough on the overall system. This isn't something I'd like to see a developer pointed at that is looking to learn good habits. Good habits are telling other developer what they can expect from a function. Bad habits are making them read the code to figure that out.