Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid duplicated code by placing general code for all three Core-Types in a generic folder. #12

Conversation

Masmiseim36
Copy link
Contributor

This Pull request is ported from the CMSIS5 Repository (--> ARM-software/CMSIS_5#1599)

As already discussed in the pull request 1587 in the CMSIS5 Repository (ARM-software/CMSIS_5#1587) there are a lot of code duplication between Cortex-A and Cortex-M implementations. This applies in particular to the compiler specific files (cmsis_compiler.h, cmsis_gcc.h, ...). The compiler specific abstractions seem to have been copied from the Core_M implementation to the Core_A implementation sometime in the past and developed apart.
Therefor I like to make a suggestion to have a generic part which is compatible with Cortex A/R/M and a specific part for the respective cores. The folder structure could look like this:

  • CMSIS
    • Core
      All generic code files are placed here, the prefix is extended to ‘cmsis_generic’ to distinguish with the core specific files.
      • cmsis_generic_armcc.h
      • cmsis_generic_armclang.h
      • cmsis_generic_compiler.h
      • cmsis_generic_gcc.h
      • cmsis_generic_iccarm.h
    • Core_A
      • All Cortex-A specific files are placed here
      • cmsis_armcc.h
      • cmsis_armclang.h
      • cmsis_compiler.h
      • cmsis_gcc.h
      • cmsis_iccarm.h
      • core_ca.h
    • Core_M
      • All Cortex-M specific files are placed here
      • cmsis_armcc.h
      • cmsis_armclang.h
      • cmsis_compiler.h
      • cmsis_gcc.h
      • cmsis_iccarm.h
      • core_cmx.h
    • Core_R
      • All Cortex-R specific files are placed here
      • For future use.

The folder structure is slightly different. The Core-Folder which contains the Cortex-M specific files is renamed to Core_M. This creates a consistent naming scheme with the other architectures and allows to use the Core folder for generic code.
This is a small incompatibility for Cortex-M devices as the path is changed. But this path is usually configured by the development environment and not in the user-project. As the development environment usually also contains the CMSIS library, the include folder can easily adapted when the library is updated by the compiler vendor. The change will therefor be invisible by the user of the library.

The Generic files are also better comparable now. As the structure and order of the functions is aligned to each other. Especially the IAR specific file had to be reordered for this. In the IAR file the comments were also added.

As it touches many files, it got quite big. Sorry for that.

@JonatanAntoni
Copy link
Member

@ReinhardKeil, any thought about such a restructuring?
I basically like the idea to reduce duplicated code. Nevertheless, I have some doubts that this could cause issue.

@Masmiseim36 Masmiseim36 force-pushed the feature/AvoidCodeDuplication branch from b363c6b to 6bdb250 Compare June 22, 2023 20:36
@Masmiseim36
Copy link
Contributor Author

Hello Jonatan

I think it is beneficial to extract common parts. If support for more Cortex-A or Cortex-R types is added, the code duplication will be even worse.
If there is something I can improve I would appreciate feedback.

Best Regards

…s in a generic folder. The Core-Folder contains the generic code only.

The specific code is placed in the folder Core_A/R/M and includes the generic code. CoreValidation: Adapt path changes for Cortex-M core
@Masmiseim36 Masmiseim36 force-pushed the feature/AvoidCodeDuplication branch from 68e8a8b to cb7d6cf Compare July 4, 2023 20:00
@Masmiseim36
Copy link
Contributor Author

Hello,

Based on this pull request, I have added initial support for further Cortex-A and Cortex-R derivatives in another branch. The whole thing is not yet completely finished.
Compare Masmiseim36@ebcdc91
I can create a separate pull request for this in the future.

Regards

@JonatanAntoni
Copy link
Member

Hi @Masmiseim36,

I had further discussion with colleagues. On one hand I fear the effort of doing this work right and future proof. The little differences between architectures and cores were the reason for going for duplication years back in time.

At the other hand the duplication might feel not well maintainable. Hence, people suggested to join back Core and Core_A and just have CMSIS-Core again. In another step one could move common parts from core headers into arch or peripheral headers which are included via the core headers; like I already did with MPU.

I think we should draw such an impacting decision wisely.

Cheers
Jonatan

@Masmiseim36
Copy link
Contributor Author

Hello @JonatanAntoni,

I generalized the parts that were identically existing in the Core-M and Core-A variants. The first step was to align the parts that had diverged (see ARM-software/CMSIS_5@7f6a7b6). The fact that they were no longer maintained and developed together could be seen well in the differences. So the lack of maintainability is very real.
With this PR I separated the shared code parts into a separate file.

Combining all core variants into a common folder I think is also a good idea. I did a quick test and think this can work well (--> #26). What do you think?

Regards
Markus

@Masmiseim36 Masmiseim36 closed this Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants