Add code formatting and checking targets

[ikstream]

Signed-off-by: Stefan Venz [email protected]

Related Issues & PRs

fix #38

Description

• Add separate Docker for code processing
• Make targets added
• Formatting file added

Context

Add Code formatting and static code analysis to improve overall code quality

Testing

• tested by me
• Feedback about code structure after formatting

To test this PR:
change to the flavor you want to check e.g. cd lifensensor
make check-code to see static code analysis (nothing wrong with the code so far)
make format-code to apply new formatting style

Documentation

• Documented both new features

[nopeslide]

is there a way to check if the source code conforms with the format?
if there is, we should integrate such check as target in the makefile

[nopeslide]

is a persistent home needed?

[ikstream]

As we want to change our src files, i think so

[nopeslide]

I don't see anything that is created in the home directory,
therefore I don't see the necessity.
Did I overlook sth?
If not: also remove the clean-format

[ikstream]

the c and header files are adapted to the style, nothing is created

[nopeslide]

the files this container modifies are mounted and have nothing to do with the container home dir. the other docker containers feature a persistent home for custom configs and caches of the toolchain (i.e. ccache, .gdbinit, ...). this is not needed in this case (why customize the format style?), hence my initial question.

[jhonnyam]

Would be nice to get some feedback which files would be/where changed.

[nopeslide]

-typedef struct {
-       const char *name;
-       StaticTask_t tcb;
-       TaskHandle_t handle;
-       StackType_t *stack;
-       UBaseType_t priority;
+typedef struct
+{
+    const char * name;
+    StaticTask_t tcb;
+    TaskHandle_t handle;
+    StackType_t *stack;
+    UBaseType_t  priority;
 } _spo2_task;

-typedef struct {
-       UBaseType_t length;
-       UBaseType_t item_size;
-       uint8_t *buffer;
-       StaticQueue_t queue;
-       QueueHandle_t handle;
+typedef struct
+{
+    UBaseType_t   length;
+    UBaseType_t   item_size;
+    uint8_t *     buffer;
+    StaticQueue_t queue;
+    QueueHandle_t handle;
 } _spo2_queue;

the pointer 'star' placement seems to be kind of random.

[nopeslide]

-       spo2_queue->handle = xQueueCreateStatic(
-               spo2_queue->length,
-               spo2_queue->item_size,
-               spo2_queue->buffer,
-               &spo2_queue->queue
-       );
+    spo2_queue->handle =
+        xQueueCreateStatic(spo2_queue->length, spo2_queue->item_size,
+                           spo2_queue->buffer, &spo2_queue->queue);

the current settings seem to stuff as much parameters as possible in one line.
if we break up parameters over multiple lines shouldn't we give each parameter its own line?

[nopeslide]

-static gpio_config_t gpio_agc_inc_config = {
-       .pin_bit_mask = SPO2_DCP_INC_PIN,
-       .mode = GPIO_MODE_OUTPUT,
-       .pull_down_en = GPIO_PULLDOWN_ENABLE,
-       .pull_up_en = GPIO_PULLUP_DISABLE,
-       .intr_type = GPIO_PIN_INTR_DISABLE
-};
+static gpio_config_t gpio_agc_inc_config = {.pin_bit_mask = SPO2_DCP_INC_PIN,
+                                            .mode         = GPIO_MODE_OUTPUT,
+                                            .pull_down_en =
+                                                GPIO_PULLDOWN_ENABLE,
+                                            .pull_up_en = GPIO_PULLUP_DISABLE,
+                                            .intr_type = GPIO_PIN_INTR_DISABLE};

multi-line initializations are hard to read when attributes don't get their own line. probably the same issues as with function parameters

[nopeslide]

-       sample->ird_dc = spo2_dc_lowpass(sample->ird_dc,
-                                        (uint16_t)raw_data->ird_dc);
-       sample->red_dc = spo2_dc_lowpass(sample->red_dc,
-                                        (uint16_t)raw_data->red_dc);
+    sample->ird_ac =
+        spo2_ac_lowpass_no_offset(sample->ird_ac, (uint16_t)raw_data->ird_ac);
+    sample->red_ac =
+        spo2_ac_lowpass_no_offset(sample->red_ac, (uint16_t)raw_data->red_ac);

breaking up assignments should be a last resort, would prefer parameter breaking here.

[nopeslide]

-Counter
-counter_reset
-(void);
+Counter counter_reset(void);

Giving function declarations/definitions at least an extra line for the return value seemed like a good idea

[nopeslide]

Would be nice to get some feedback which files would be/where changed.

this would be possible by moving the find instruction inside the container. will look into it

[nopeslide]

Would be nice to get some feedback which files would be/where changed.

not absolutely happy with the implementation, but here you go
make check-format lists all files that need formatting and errors if any file needs formatting
make format-code now lists all files it touches (regardless if format was needed)

[nopeslide]

not absolutely happy with the implementation, but here you go
make check-format lists all files that need formatting and errors if any file needs formatting
make format-code now lists all files it touches (regardless if format was needed)

on second thought, I think these targets are unneeded. If you format your code git shows what happened. if you want to check if any code needs formatting, format it and ask git if changes occurred.

[ikstream]

We could run clang-format twice, once with -i and once without. This would show the changes prior applying it and only one command would be needed.

[nopeslide]

We could run clang-format twice, once with -i and once without. This would show the changes prior applying it and only one command would be needed.

clang-format dumps the whole file, making it kind of useless output. would prefer something like this
to check format (line diff)

git ls-files -ico -x '**/*.c' -x '**/*.h' | xargs -I % sh -c "clang-format % | git diff --exit-code --no-index --color % -"

to check format (word diff, fancy for these minimal whitespace changes)

git ls-files -ico -x '**/*.c' -x '**/*.h' | xargs -I % sh -c "clang-format % | git -c color.diff.new='normal 22' -c color.diff.old='normal 166' diff --exit-code --no-index --color --word-diff=color --word-diff-regex=. % -"

to apply format

git ls-files -ico -x '**/*.c' -x '**/*.h' | xargs -I % clang-format % 

[ikstream]

As there seems to be something broken with the config, besides the unfix-able problem, I will close this in favor of #70