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. #1599

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Masmiseim36
Copy link
Contributor

@Masmiseim36 Masmiseim36 commented Dec 17, 2022

As already discussed in the pull request #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.

@ARM-software ARM-software deleted a comment from grasci-arm Dec 19, 2022
@Masmiseim36
Copy link
Contributor Author

Hello all,
I have some difficulties with the configuration of the CICD system. Maybe someone can give me a hint there.
But first of all, I would like to discuss the structure I proposed. I would be happy to implement the modifications discussed.

@JonatanAntoni
Copy link
Member

JonatanAntoni commented Dec 21, 2022

Hi @Masmiseim36,

We repeatedly think about refactoring the layout but typically we end up that this is not worth the effort. The benefit seems to be small but there is a high risk of breaking existing applications.

Your proposal looks basically fine. Removing the prefixes in the generic folder sounds problematic, though. We might easily create name clashes when using too generic names like compiler.h or gcc.h.

Please take special note that the cmsis_iccarm.h's are provided and maintained by IAR.

Regarding CI/CD: You should be able to run the CoreValidation tests on your machine as long as you have the required toolchains installed. Today, this are Arm Compiler 6 and GCC. You need CMSIS-Toolbox and probably want to use the Python build.py with its dependencies in requirements.txt. The workflow is documented in the GitHub action files. Let me take this as the opportunity to write it down into a README.

Your changes break the current tests because you didn't update the pack description, see ARM.CMSIS.pdsc. As you changed the required include paths you need to update the references here.

Cheers,
Jonatan

@Masmiseim36 Masmiseim36 force-pushed the feature/AvoidCodeDupplication branch from 92751bd to a8b2f56 Compare December 21, 2022 21:13
@Masmiseim36
Copy link
Contributor Author

Hello @JonatanAntoni,

Thank you for your feedback. I see your point about the risk. The code duplication should also be solvable with keeping the current folder structure. We could introduce a new folder ‘Core_Generic’:

  • CMSIS
    • Core_Generic
      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
      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_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_R
      All Cortex-R specific files are placed here
      • For future use.

If you agree I can implement it accordingly. The CICD Issues should also be solved with this change.
Can someone from IAR review my modifications in the associated files?

Copy link
Member

@JonatanAntoni JonatanAntoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can rename Core to Core_M in order to be aligned with Core_A and Core_R. We just need to be careful how to name the generic part. It could be Core but the individual header files should to have cmsis_ prefix in their names.

ARM.CMSIS.pdsc Outdated Show resolved Hide resolved
@Masmiseim36 Masmiseim36 force-pushed the feature/AvoidCodeDupplication branch from a8b2f56 to 9e47d03 Compare December 22, 2022 14:25
CMSIS/Core_M/Include/cmsis_gcc.h Fixed Show resolved Hide resolved
@Masmiseim36
Copy link
Contributor Author

Hello @JonatanAntoni
I’ve added the prefix ‚ cmsis_generic_’ to the core independent files. With this we can distinguish the generic files with the specific files while still having the ‘cmsis’ prefix. Do you agree?

@Masmiseim36 Masmiseim36 force-pushed the feature/AvoidCodeDupplication branch 3 times, most recently from 753ffe5 to a4b8c27 Compare December 23, 2022 19:15
@Masmiseim36
Copy link
Contributor Author

Your proposal looks basically fine. Removing the prefixes in the generic folder sounds problematic, though. We might easily create name clashes when using too generic names like compiler.h or gcc.h.

Regarding CI/CD: You should be able to run the CoreValidation tests on your machine as long as you have the required toolchains installed. Today, this are Arm Compiler 6 and GCC. You need CMSIS-Toolbox and probably want to use the Python build.py with its dependencies in requirements.txt. The workflow is documented in the GitHub action files. Let me take this as the opportunity to write it down into a README.

Hello JonatanAntoni,
The CICD seems to be okay now, and the prefix is also added for the generic file. Any additional feedback is appreciated.
But I was not able to get the CoreValidation running locally. When using build.py I get a crash in the matrix-runner package. I used python in Version 3.11.1 and matrix-runner in version 1.1.0, the required toolchains should also be available.
I have only little experience in python, but after a quick check I was not able to find the root of the issue.

python -m build -c GCC -d CM3 -o low build run
[GCC][Cortex-M3][low](build:cbuild) Command 'cbuild' not found in current environment.
[GCC][Cortex-M3][low](build:cbuild) cbuild Validation.GCC_low+CM3/Validation.GCC_low+CM3.cprj
Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "D:\Daten\git\CMSIS_5\CMSIS\CoreValidation\Project\build.py", line 253, in <module>
    main()
  File "C:\Users\Markus\AppData\Roaming\Python\Python311\site-packages\matrix_runner\runner.py", line 278, in __init__
    success = self.run(argv)
              ^^^^^^^^^^^^^^
  File "C:\Users\Markus\AppData\Roaming\Python\Python311\site-packages\matrix_runner\runner.py", line 219, in run
    self.run_config(args.action, config)
  File "C:\Users\Markus\AppData\Roaming\Python\Python311\site-packages\matrix_runner\runner.py", line 230, in run_config
    results = action(config)
              ^^^^^^^^^^^^^^
  File "C:\Users\Markus\AppData\Roaming\Python\Python311\site-packages\matrix_runner\action.py", line 57, in __call__
    results.append(cmd())
                   ^^^^^
  File "C:\Users\Markus\AppData\Roaming\Python\Python311\site-packages\matrix_runner\command.py", line 101, in __call__
    self._execute(cmdline, result)
  File "C:\Users\Markus\AppData\Roaming\Python\Python311\site-packages\matrix_runner\command.py", line 278, in _execute
    success = self._execute_with_lock(cmdline, result)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Markus\AppData\Roaming\Python\Python311\site-packages\matrix_runner\command.py", line 272, in _execute_with_lock
    return self._execute_with_retry(cmdline, result)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Markus\AppData\Roaming\Python\Python311\site-packages\matrix_runner\command.py", line 248, in _execute_with_retry
    success = self._execute_with_rest(cmdline, result)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Markus\AppData\Roaming\Python\Python311\site-packages\matrix_runner\command.py", line 242, in _execute_with_rest
    return self._execute_with_timeout(cmdline, result)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Markus\AppData\Roaming\Python\Python311\site-packages\matrix_runner\command.py", line 223, in _execute_with_timeout
    rc = Command._popen(cmdline, result, needs_shell=self._needs_shell, encoding=self._encoding,
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\Markus\AppData\Roaming\Python\Python311\site-packages\matrix_runner\command.py", line 199, in _popen
    with Popen(cmdline, stdout=PIPE, stderr=PIPE, shell=needs_shell, encoding=encoding) as proc, \
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Program Files\Python311\Lib\subprocess.py", line 1024, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "C:\Program Files\Python311\Lib\subprocess.py", line 1493, in _execute_child
    hp, ht, pid, tid = _winapi.CreateProcess(executable, args,
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [WinError 2]

@JonatanAntoni
Copy link
Member

@Masmiseim36, the problem is right in the first line. It's not a Python issue even if the thrown exception is not properly handled.
The issue is here:

[GCC][Cortex-M3][low](build:cbuild) Command 'cbuild' not found in current environment.

You do not have cbuild on your env PATH. You need to have CMSIS-Toolbox installed and its bin folder in PATH.

Cheers,
Jonatan

@JonatanAntoni JonatanAntoni requested review from RobertRostohar and a user January 9, 2023 12:07
@Masmiseim36 Masmiseim36 force-pushed the feature/AvoidCodeDupplication branch from a4b8c27 to 5754493 Compare April 16, 2023 12:06
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants