[refactoring] General maintenance

[Nightwalker-87]

The following tasks should be achieved to improve the codebase of the toolset:

• modularise the codebase
• reduce cross-dependencies between headers and source files where possible
• separate device-specific settings from source code ([refactoring] Moved chip-specific parameters into separate files #1129)
• rewrite and modularisation of: chipid.c chipid.h common.c stlink.h stm32.h
• allow general use of core-IDs for MCU identification ([code-structure] Add all STM32 Core-IDs to the codebase for MCU identification #903)

Some intentions on general proceedings (priorised):

1. Simplification / Clean-up / Restructuring
2. Correction of data types based on C/C++ Reference and common recommendations
3. Evaluation on how to commonly include Cortex-M core ids (CPUTAPID) in the workflow of device identification
4. Definition of structures to prepare for future ABI compatibilty
5. Update for the stlink-lib documentation

[Nightwalker-87]

@Ant-ON @slyshykO I have just pushed a new commit to branch chipid_cleanup with some further preparations.
Can you have a look at it, give some feedback and verify functionality. I did some basic tests with a STM32 VL Discovery board but not too many yet. Also additional hardware to test with is currently out of reach...

[Nightwalker-87]

Based on the original intention to solely complete the separation of chip-specific parametres out of the source code and into device-specific files led to some more general clean-up and restructuring. Nevertheless work is not complete of course...
I believe that the source code and header files stm32.h, chipid.c and chipid.h are on a good way to reach structure and content as we may expect them to be and to serve as.

For now I am going to make a halt here to give everybody a chance to follow the changes which appear quite huge already.
Please let me know if you deserve a PR to be opened before continuing with other topics.

[Nightwalker-87]

... and BTW: When does

#ifdef __cplusplus
    extern "C" {
#endif

[...]

#ifdef __cplusplus
}
#endif

exactly make sense?

[slyshykO]

... and BTW: When does

#ifdef __cplusplus
    extern "C" {
#endif

[...]

#ifdef __cplusplus
}
#endif

exactly make sense?

It needs for whom who uses our includes in C++ code.

[Nightwalker-87]

Tell me a story, I got that far, but the question is: Where is it necessary from the technical perspective. It is not in general, because otherwise it would be defined in every source code file, which isn't the case and compilation and execution in C++ is known to be working anyway.

[hydroconstructor]

What is "modularisation of common.c"? Divide it to several files?
What is cross-dependencies between headers and source files? Rewrite sources to reduce number of included headers for each source?

[slyshykO]

Where is it necessary from the technical perspective?

It is necessary when someone uses libstlink in a custom C++ application because C++ mangle functions names by default and without extern "C" declaration linker didn't find proper symbols.

Also, this extern guard is not needed in all sources. It is required only in the main header, for example, in stlink.h in our case.

[Nightwalker-87]

@hydroconstructor common.c is basically an bunch of stuff, partially hardcoded and/or duplicated from implementations that were written later. Generally spoken it's somehow the historical root of the whole project and holds many remnants from earlier times. We want that sorted, separated and integrated into the source code structure that was introduced and extended by many valuable contributions in the more recent years. "Modular" actually means that different parts of the code that handle different operations or functionality should not mix up and should only have cross dependencies that are either really necessary on the technical level or implemented in such a way that changes in one module or specific part do not imply several other necessary changes in other parts of the code.

[Nightwalker-87]

Please test the branch chipid_cleanup with various boards and report any issues.
The focus should be on st-info mostly.

[gszy]

🔶 As discussed with @Nightwalker-87, I’m adding another cleanup-related idea, originally described at c854df5#commitcomment-63941871.

enum stm32_flash_type describes the different flash types that STM32 MCUs have. The enumeration constants have different, manually assigned, integer values, beginning with special STM32_FLASH_TYPE_UNKNOWN = 0 and ending with special STM32_FLASH_TYPE_UNDEFINED = 12. I’ve suggested removing the manual assignments, as the C compiler would automatically assign precisely the same values, but I 🧟 forgot the—somewhat arbitrarily chosen—values have actual practical meaning: they are used in the chip description files as the values of the flash_type property.

I suggest the following:

• the chip description flash_type property should be a human-readable string describing the flash type, for example L4_L4P or G0;
• enum stm32_flash_type constant values should be automatically assigned, with STM32_FLASH_TYPE_UNKNOWN being the first and STM32_FLASH_TYPE_UNDEFINED being the last;
• whichever stlink part parses chip files, should convert the string descriptions to the enum values.

[Nightwalker-87]

@gszy I've replaced the list of defines with an enum holding the flash types in human readable form and added the individual types to the .chip files, replacing the integer values there. However this implies that the following excerpt in void process_chipfile(char *fname) { } in chipid.c needs to be rewritten to use the enum and convert the respective types from enum to int.

[...]
        } else if (strcmp (word, "flash_type") == 0) {
            if (sscanf(value, "%i", (int *)&ts->flash_type) < 1) {
                fprintf(stderr, "Failed to parse flash type\n");
            } else if ((ts->flash_type < STM32_FLASH_TYPE_UNKNOWN) || (ts->flash_type >= STM32_FLASH_TYPE_UNDEFINED)) {
                fprintf(stderr, "Unrecognized flash type\n");
            }
[...]

I'm unsure yet on the most reasonable approach. So far this unfinished attempt is only a local version for testing purposes.

[gszy]

The simplest solution would be a bunch of this kind of comparisons:

if (strcmp(value, "G0") == 0) {
    &ts->flash_type = STM32_FLASH_TYPE_G0;
}

It may be a part of process_chipfile(), or extracted to a separate function.

Alternatively we could have a const array somewhere that would map the enum values to the string values, something like:

const char *x[] = {
    [STM32_FLASH_TYPE_G0] = "G0",
};

then loop through the array when parsing a flash type.

[Nightwalker-87]

I'd prefer the first attempt to make it work. Anything seems better than the #defines used currently. A first implementation is found in commit 8d96e76. Please comment in #1216 respectively.