Feedback - synthetos/TinyG GitHub Wiki

I realize this is unsolicited feedback. The TinyG authors may not want this and may not like it. But, I think it's useful for the authors to know and useful to other developers working with the TinyG codebase.

The info was hard fought while extending the tool for our application. I consider myself a very skilled developer. None-the-less, these are opinions and others may not agree. I do not intend to upset, but it can be hard to receive criticism. I don't like it either. But, it can be useful for achieving excellence.

The codebase has a well-considered design and a relatively consistent implementation. That's great and thank you. The application seems to work well, to be reliable and run fast. So many good things to say about it. But, with so much software, working well is very different from being written well.

We extended the codebase in rather modest ways to support our system design. In fact, other devs modified it and I came onboard to push the project across the finish line. I had to fix various problems; bugs. And, I struggled terribly with understanding the TinyG design and with understanding the code.

I just found the wiki and was immediately turned off by: "The best way to understand the code is to read it. It's pretty well commented." I do not think the best way to understand this or any non-trivial codebase is to read it. I agree that reading code is a good way to understand a codebase deeply, but not to start with; not at a high-level. Much thought goes into writing a non-trivial application such as TinyG, and much of the info transcends particular functions and files/modules. I agree that one can understand the higher-level concepts by reading the implementation. But, it's hard to do that. So, much easier if there's a document that explains the concepts. I'm so glad this wiki exists and its a great place for this information. But, own it. Say that the code is not the best place to learn these higher level concepts. Provide an intro that is the starting place. It's fine to encourage developers to read the source code, but don't suggest it's the best place to start.

FYI, this is not the first time I've been told to read the source code to answer any questions I have. But, it's lame and it's lazy. I think people suggest reading the source code because they find it both hard and unpleasant to write documentation. I get it. Writing documentation is not for everyone. It is something I'm good at so maybe I can help. This here is about describing shortcomings that I see. I am capable and willing to help make things better.

I do not think the codebase is well commented. It does lots of comments, but in general they are low quality. For the most part the comments are not wrong which is better than in some codebases. But, I find many to be poorly worded, confusing, lacking organization, lacking context.

Pluses

Some pluses of the codebase:

  • Relatively consistent in terms of formatting, commenting, layout and naming. That does help with understanding/reading.
  • There is some logical separation of the code by responsibility. It's not just one giant file. Capabilities are separated from each other to some extent which makes understanding easier. It also makes modifying and extending the codebase easier and safer.
  • main.c and main() are relatively small which is great since this code is hard if not impossible to test

Deficiencies

Cryptic names

nvObj_t, NV_FORMAT_LEN: what does NV stand for? Can't find that; don't use abbreviations

get_flt, get_grp: Lazy to abbreviate float and group

set_01, nv_reset_nv: huh?

Misleading names

From reading config.c, I find that it has multiple responsibilities (which is not good), but none would I call configuration -- which implies design-time or startup behavior. But this is primarily about runtime behavior. It stores state that is used internally. But I think the more interesting aspect is how it supports client requests. It is a simple database and is specially designed for the REST-like client request capability. It's cool. But, it's not configuration.

json_parser and text_parser: These are parsers yes. But they are also request dispatchers and response formatters. Calling them parsers is confusing.

Function names should start with a verb. For example, based on it's name, what does json_parser do? Can't tell. The name confuses module with function. Could suggest something like parse_json (except that it does much more than just parse, but that's a different issue).

Lacks unit tests

Unit testing, testing a module in isolation, tends to force better/modular design and make code more robust and easier to change (without fear of breaking it).

I see tests in the codebase but they are not unit-level.

Include files too long

Too much info in include files. Include files are a necessary evil of C that allow files (modules) to consume each other. The length of include files affects build time since they get included into potentially multiple c files. They should have as little info as possible.

Also, they should start with the include check. Starting with licensing info slows the build. I don't see the value of having the license info in include files (or any files for that matter), but if you want that in there, put it after the include check.

Put developer comments in the c file. The include files are for the compiler and it doesn't need explanatory info.

Unusual/bad commenting

Comments to the right of code

Although somewhat common, it is somewhat outdated style (from assembly days) and generally considered poor style. Reasons include: super wide lines which leads to easy to miss comments as they are off screen. Also, it's better that the code describe itself rather than have a comment saying what it does. IOW, less comments; better naming.

Commenting multiple functions in same block

Commenting multiple functions in one block may have seemed like a good idea when writing the code originally, but for maintenance it's a bad idea; confusing; not a helpful layout. Consider

/* Generic gets()

  • get_nul() - get nothing (returns STAT_PARAMETER_CANNOT_BE_READ)
  • get_ui8() - get value as 8 bit uint8_t
  • get_int() - get value as 32 bit integer
  • get_data() - get value as 32 bit integer blind cast
  • get_flt() - get value as float
  • get_format() - internal accessor for printf() format string */

The definition of get_flt() is way further down. One looking at the function definition might not realize there is a comment for it. Well, that comment is not all that useful, but this patter is used throughout. Not a good pattern.

Note that get_format is not defined! Surely it used to, but then got deleted. But it's comment didn't. This is one reason to keep related code as close as possible. If the comment had been right above the function declaration, then almost surely the comment would have been deleted when the function was deleted.

Use Doxygen

In general, leverage industry standard tools such as doxygen instead of re-inventing the wheel. Commenting does follow a pattern which is good, but it's overly clearly; overly original. And these are not good things. Using standards; at least where it makes sense. And it makes sense here.

Much conditional compilation

Why so much conditional compilation around axis counts and such. Why needed. Tell us.

Does the __ARM conditional compilation have value? Is so, tell me.

Does the __cplusplus conditional compilation have value? I don't see any.

Wiki is poorly organized

Yes, even the wiki is poorly organized. I can help with that.

Example

Choosing a function somewhat at random that is somewhat representative of the codebase:

/*
* _get_next_axis() - return next axis in sequence based on axis in arg
*
*	Accepts "axis" arg as the current axis; or -1 to retrieve the first axis
*	Returns next axis based on "axis" argument and if that axis is flagged for homing in the gf struct
*	Returns -1 when all axes have been processed
*	Returns -2 if no axes are specified (Gcode calling error)
*	Homes Z first, then the rest in sequence
*
*	Isolating this function facilitates implementing more complex and
*	user-specified axis homing orders
*/

static int8_t _get_next_axis(int8_t axis)
{
#if (HOMING_AXES <= 4)
//    uint8_t axis;
//    for(axis = AXIS_X; axis < HOMING_AXES; axis++)
//        if(fp_TRUE(cm.gf.target[axis])) break;
//    if(axis >= HOMING_AXES) return -2;
//    switch(axis) {
//        case -1:        if (fp_TRUE(cm.gf.target[AXIS_Z])) return (AXIS_Z);
//        case AXIS_Z:    if (fp_TRUE(cm.gf.target[AXIS_X])) return (AXIS_X);
//        case AXIS_X:    if (fp_TRUE(cm.gf.target[AXIS_Y])) return (AXIS_Y);
//        case AXIS_Y:    if (fp_TRUE(cm.gf.target[AXIS_A])) return (AXIS_A);
//#if (HOMING_AXES > 4)
//        case AXIS_A:    if (fp_TRUE(cm.gf.target[AXIS_B])) return (AXIS_B);
//        case AXIS_B:    if (fp_True(cm.gf.target[AXIS_C])) return (AXIS_C);
//#endif
//        default:        return -1;
//    }
	if (axis == -1) {	// inelegant brute force solution
		if (fp_TRUE(cm.gf.target[AXIS_Z])) return (AXIS_Z);
		if (fp_TRUE(cm.gf.target[AXIS_X])) return (AXIS_X);
		if (fp_TRUE(cm.gf.target[AXIS_Y])) return (AXIS_Y);
		if (fp_TRUE(cm.gf.target[AXIS_A])) return (AXIS_A);
		return (-2);	// error
	} else if (axis == AXIS_Z) {
		if (fp_TRUE(cm.gf.target[AXIS_X])) return (AXIS_X);
		if (fp_TRUE(cm.gf.target[AXIS_Y])) return (AXIS_Y);
		if (fp_TRUE(cm.gf.target[AXIS_A])) return (AXIS_A);
	} else if (axis == AXIS_X) {
		if (fp_TRUE(cm.gf.target[AXIS_Y])) return (AXIS_Y);
		if (fp_TRUE(cm.gf.target[AXIS_A])) return (AXIS_A);
	} else if (axis == AXIS_Y) {
		if (fp_TRUE(cm.gf.target[AXIS_A])) return (AXIS_A);
	}
	return (-1);	// done

#else

	if (axis == -1) {
		if (fp_TRUE(cm.gf.target[AXIS_Z])) return (AXIS_Z);
		if (fp_TRUE(cm.gf.target[AXIS_X])) return (AXIS_X);
		if (fp_TRUE(cm.gf.target[AXIS_Y])) return (AXIS_Y);
		if (fp_TRUE(cm.gf.target[AXIS_A])) return (AXIS_A);
		if (fp_TRUE(cm.gf.target[AXIS_B])) return (AXIS_B);
		if (fp_TRUE(cm.gf.target[AXIS_C])) return (AXIS_C);
		return (-2);	// error
	} else if (axis == AXIS_Z) {
		if (fp_TRUE(cm.gf.target[AXIS_X])) return (AXIS_X);
		if (fp_TRUE(cm.gf.target[AXIS_Y])) return (AXIS_Y);
		if (fp_TRUE(cm.gf.target[AXIS_A])) return (AXIS_A);
		if (fp_TRUE(cm.gf.target[AXIS_B])) return (AXIS_B);
		if (fp_TRUE(cm.gf.target[AXIS_C])) return (AXIS_C);
	} else if (axis == AXIS_X) {
		if (fp_TRUE(cm.gf.target[AXIS_Y])) return (AXIS_Y);
		if (fp_TRUE(cm.gf.target[AXIS_A])) return (AXIS_A);
		if (fp_TRUE(cm.gf.target[AXIS_B])) return (AXIS_B);
		if (fp_TRUE(cm.gf.target[AXIS_C])) return (AXIS_C);
	} else if (axis == AXIS_Y) {
		if (fp_TRUE(cm.gf.target[AXIS_A])) return (AXIS_A);
		if (fp_TRUE(cm.gf.target[AXIS_B])) return (AXIS_B);
		if (fp_TRUE(cm.gf.target[AXIS_C])) return (AXIS_C);
	} else if (axis == AXIS_A) {
		if (fp_TRUE(cm.gf.target[AXIS_B])) return (AXIS_B);
		if (fp_TRUE(cm.gf.target[AXIS_C])) return (AXIS_C);
	} else if (axis == AXIS_B) {
		if (fp_TRUE(cm.gf.target[AXIS_C])) return (AXIS_C);
	}
	return (-1);	// done

#endif
}

I have so many questions about it -- especially about the comments.

  • Header: Don't include the function name in the function header comment. I can see the function name in the code. DRY, KISS
  • Header: What does 'current axis' imply? In general, avoid current as it's always confusing.
  • Header: What does 'processed' imply? processed by what? in what way?
  • Header: What is the 'isolation' comment about? isolated from what? I don't know what is implied by 'more complex... orders'
  • Header: What's an 'order'? a command? ordering?
  • What's up with big block of commented out code?
  • What's up with conditional compilation? In general conditional compilation is confusing and error prone. Avoid.
  • What does "inelegant brute force solution" imply? What is the value of this comment?