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

arm math: DSP header file should be part of CMSIS DSP, not alone in cmsis #12054

Closed
0xc0170 opened this issue Dec 9, 2019 · 6 comments · Fixed by #12055
Closed

arm math: DSP header file should be part of CMSIS DSP, not alone in cmsis #12054

0xc0170 opened this issue Dec 9, 2019 · 6 comments · Fixed by #12055

Comments

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 9, 2019

Description of defect

File arm_math.h: https://github.com/ARMmbed/mbed-os/blob/master/cmsis/TARGET_CORTEX_M/arm_math.h is part of this repository. It was added 106f34e but I believe it was in the tree longer as we used to have DSP library part of Mbed 2. I recall this header contained some implementation we used (CLZ for example) but that was removed in this header. We should not need this header file.

It causes a problems, as CMSIS DSP is a component (providing implementation), not any API we could overwrite. We should review this header file, and possibly remove it (it is outdated, our import script ignores it). Plus it causes a conflicts, for applications that use DSP, our outdated header file gets in and causes errors.

Ideally, DSP gets in via separate module as we do not provide this functionality in Mbed OS.

Target(s) affected by this defect ?

Cortex-M targets (all)

Toolchain(s) (name and version) displaying this defect ?

All, generic file

What version of Mbed-os are you using (tag or sha) ?

5.14.0

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

Not relevant

How is this defect reproduced ?

Not relevant

cc @ARMmbed/mbed-os-core

@0xc0170 0xc0170 changed the title arm math: arm math: DSP header file should be part of CMSIS DSP, not alone in cmsis Dec 9, 2019
0xc0170 added a commit to 0xc0170/mbed-os that referenced this issue Dec 9, 2019
Fixes ARMmbed#12054

Not needed file, should be part of DSP. The functionality is part of the library provided
by CMSIS.
@kjbracey
Copy link
Contributor

kjbracey commented Dec 9, 2019

We have had a small non-zero number of queries about why we don't include the CMSIS DSP/NN libraries.

Would be nice to bring them in as a feature/component/whatever to make people's life easy.

But certainly we shouldn't be supplying this old fragment that interferes with someone bringing them in themselves.

@ciarmcom
Copy link
Member

ciarmcom commented Dec 9, 2019

Internal Jira reference: https://jira.arm.com/browse/MBOTRIAGE-2454

@0Grit
Copy link

0Grit commented Dec 9, 2019

@AGlass0fMilk

@AGlass0fMilk
Copy link
Member

My solution up to now was to find which old version of CMSIS DSP that header was from and import the appropriate source 😅.

It's probably easier to just have people import CMSIS DSP/NN themselves (using mbed add or something). That way Mbed-OS doesn't have to maintain a copy/link to another repo. If they're using DSP/NN (fairly advanced) they will likely be familiar with adding a library.

What I would like to see is a C++ DSP framework for Mbed built with CMSIS DSP :)

@0Grit
Copy link

0Grit commented Dec 9, 2019

yeah would be nice if CMSIS-packs were mature enough to be used for dependency management. Or if an official and well specified C/C++ dependency management tool were selected to be used with Mbed...

@0xc0170
Copy link
Contributor Author

0xc0170 commented Dec 10, 2019

It's probably easier to just have people import CMSIS DSP/NN themselves (using mbed add or something). That way Mbed-OS doesn't have to maintain a copy/link to another repo. If they're using DSP/NN (fairly advanced) they will likely be familiar with adding a library.

That is the plan, this header file was there from times when we had DSP in the tree. It causes issues now.

yeah would be nice if CMSIS-packs were mature enough to be used for dependency management. Or if an official and well specified C/C++ dependency management tool were selected to be used with Mbed...

Wish the same 👍

0xc0170 added a commit to 0xc0170/mbed-os that referenced this issue Jan 3, 2020
Fixes ARMmbed#12054

Not needed file, should be part of DSP. The functionality is part of the library provided
by CMSIS.
0xc0170 added a commit to 0xc0170/mbed-os that referenced this issue Jan 13, 2020
Fixes ARMmbed#12054

Not needed file, should be part of DSP. The functionality is part of the library provided
by CMSIS.
manchoz pushed a commit to manchoz/mbed-os that referenced this issue Feb 28, 2020
Fixes ARMmbed#12054

Not needed file, should be part of DSP. The functionality is part of the library provided
by CMSIS.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants