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

Copy latest version of arm_math.h from CMSIS github project #12040

Closed
wants to merge 1 commit into from

Conversation

freddan80
Copy link

@freddan80 freddan80 commented Dec 5, 2019

Summary of changes

We're working on integrating CMSIS-NN into TensorflowLite Micro (TFLu). One of the features of TFLu is that can generate an Mbed project. When that is done, CMSIS is downloaded as third party code. For reasons I don't know, the mbed build system prioritizes the arm_math.h in cmsis/TARGET_CORTEX_M/. As a quick fix we want to update this file.

Impact of changes

Migration actions required

Documentation


Pull request type

[*] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[*] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers


@ciarmcom ciarmcom requested a review from a team December 5, 2019 18:00
@ciarmcom
Copy link
Member

ciarmcom commented Dec 5, 2019

@freddan80, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 6, 2019

@freddan80 Please fill in the PR template. Introduce changes and tell us more about them, although this is just an update from upstream.

@ARMmbed/mbed-os-core @kjbracey-arm We used to have a script to fetch the latest cmsis files? Looking at this file, it is time to update. It would be good to update CMSIS files for 6.0 release

@0xc0170 0xc0170 requested a review from a team December 6, 2019 08:22
@freddan80
Copy link
Author

Hi!

I filled in the PR template. If you have a script for updating the file it would be great! There two more occasions of this file in the repo. I only updated the one I needed for now, to have an minimal impact on your repo.

Cheers!

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 6, 2019

Thanks looks fine now.

I checked the importer, it imports only RTX files. I could not locate this file there.

I don't know, the mbed build system prioritizes the arm_math.h in cmsis/TARGET_CORTEX_M/. As a quick fix we want to update this file.

If they have same names, its just an order thing if you include just "name_header.h" in your file, not something more explicit "module/name_header.h" or similar.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 6, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Dec 6, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 6, 2019

@mmahadevan108 The drivers for LPC are made for earlier version of cmsis headers? There are errors in the build, only for LPC55S69_NS:

[Error] fsl_powerquad_cmsis.c@748,6: conflicting types for 'arm_fir_init_f32' [Error] fsl_powerquad_cmsis.c@775,6: conflicting types for 'arm_fir_init_q31' [Error] fsl_powerquad_cmsis.c@801,12: conflicting types for 'arm_fir_init_q15' [Error] fsl_powerquad_cmsis.c@830,6: conflicting types for 'arm_fir_f32' [Error] fsl_powerquad_cmsis.c@855,6: conflicting types for 'arm_fir_q31' [Error] fsl_powerquad_cmsis.c@880,6: conflicting types for 'arm_fir_q15' [Error] fsl_powerquad_cmsis.c@906,6: conflicting types for 'arm_conv_f32' [Error] fsl_powerquad_cmsis.c@918,6: conflicting types for 'arm_conv_q31' [Error] fsl_powerquad_cmsis.c@930,6: conflicting types for 'arm_conv_q15' [Error] fsl_powerquad_cmsis.c@942,6: conflicting types for 'arm_correlate_f32' [Error] fsl_powerquad_cmsis.c@954,6: conflicting types for 'arm_correlate_q31' [Error] fsl_powerquad_cmsis.c@966,6: conflicting types for 'arm_correlate_q15'

@freddan80 The update is not that simple as there are targets in the codebase (CI found just one at the moment), we need an updated driver from NXP to get this in.

@ARMmbed/team-nxp Can we get new driver along with updating cmsis ?

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 6, 2019

Sem ver seems wrong in the header file V1.6.0 from 1.5x - breaking change in there ?

@freddan80
Copy link
Author

@0xc0170, thanks for the info. I'll await the change from @ARMmbed/team-nxp

In the meanwhile, I'll try to proceed with another fix for my issue, since I assume it may take a while?

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 6, 2019

I assume this could be, or we fix the driver. I reviewed the changes, there was a change in constness for cmsis math API. This is causing the errors here.

@mmahadevan108
Copy link
Contributor

Below is the zip file with the updated powerquad drivers
powerquad.zip

@freddan80
Copy link
Author

If they have same names, its just an order thing if you include just "name_header.h" in your file, not something more explicit "module/name_header.h" or similar.

I propagates behind the include I use in TFLu, "arm_nnfunctions.h". This in turn includes "arm_nnsupportfunctions.h", which then includes "arm_math.h"... Is there a way to specific on the make line what to include where? I use this:

mbed compile -m DISCO_F746NG -t GCC_ARM -j8

folder structure below is:

mbed-os/ (ok to use the old arm_math.h)
tensorflow/ (here I want to use: tensorflow/lite/experim
ental/micro/tools/make/downloads/cmsis/CMSIS/DSP/Include/arm_math.h)

Can I accomplish this using the mbed compile command?

Cheers!

@evedon
Copy link
Contributor

evedon commented Dec 6, 2019

@ARMmbed/mbed-os-core @kjbracey-arm We used to have a script to fetch the latest cmsis files? Looking at this file, it is time to update. It would be good to update CMSIS files for 6.0 release

Let's put this on the list for Mbed 6

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 9, 2019

@mmahadevan108 Thanks for sharing the updated driver.

Can I accomplish this using the mbed compile command?

@freddan80 if your code can include this #include DSP/Include/arm_math.h , it would work, not just arm_math.h. If not, you will need this PR to bring the CMSIS math header up to date.

The next Mbed OS version (in this case 6.0.0) should contain the latest CMSIS release, we will update files in the separate ticket.
We can integrate this pull request, if you add files from the zip shared by @mmahadevan108
This will go into 6.0.

Let us if this works for you.

@0xc0170 0xc0170 added the release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 label Dec 9, 2019
@0xc0170
Copy link
Contributor

0xc0170 commented Dec 9, 2019

@mmahadevan108 DSP is provided as a component, how does NXP overwrites default DSP implementation ? Please talk to CMSIS team, they would like to understand the driver powerquad (how to override DSP and provide own implementation). As DSP is a component, if you download/use CMSIS-Packs, you would get duplicate definitions. I assume NXP SDK provide own DSP changed componented (to work with this driver) ?

I'll create a separate issue why arm_math is in our include files, as we do not have DSP files in the tree anymore (unsupported).

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 9, 2019

I created #12054, I'll create separate PR to test now if we can remove this header file completely. That would be better way how to fix the error.

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 11, 2019

As replacement proposed, I'll close this one.

@0xc0170 0xc0170 closed this Dec 11, 2019
@adbridge adbridge removed needs: work release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 labels Feb 17, 2020
@mergify mergify bot added the needs: work label Feb 17, 2020
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.

7 participants