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.

Sign up for GitHub

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

Jump to bottom

manifest: add cmsis_6 #82022

Merged
merged 1 commit into from
Dec 16, 2024
Merged

manifest: add cmsis_6 #82022

merged 1 commit into from
Dec 16, 2024

Conversation

tomi-font
Copy link
Collaborator

@tomi-font tomi-font commented Nov 26, 2024 •
edited
Loading

Add the CMSIS v6 repo as a module.

Fixes #77220.

@zephyrbot
Copy link
Collaborator

zephyrbot commented Nov 26, 2024 •
edited
Loading

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff
cmsis_6 🆕 N/A (Added) zephyrproject-rtos/CMSIS_6@783317a (main) N/A

⛔ DNM label due to: 1 added project

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@zephyrbot zephyrbot added manifest manifest-cmsis_6 DNM This PR should not be merged (Do Not Merge) labels Nov 26, 2024
@tomi-font tomi-font linked an issue Nov 26, 2024 that may be closed by this pull request
Module for CMSIS v6 #77220
Closed
pdgendt
pdgendt reviewed Nov 26, 2024
View reviewed changes
@tomi-font
Copy link
Collaborator Author

FYI @ithinuel @microbuilder @povergoing

Let me know if anyone wants to be added/removed as collaborator/maintainer.

@tomi-font tomi-font force-pushed the cmsis6_module_addition branch from e32f24b to 207a338 Compare November 26, 2024 08:13
pdgendt
pdgendt reviewed Nov 26, 2024
View reviewed changes
@tomi-font tomi-font force-pushed the cmsis6_module_addition branch 2 times, most recently from 6e7b368 to 502c888 Compare November 26, 2024 08:21
nordicjm
nordicjm reviewed Nov 26, 2024
View reviewed changes
nordicjm
nordicjm previously approved these changes Nov 26, 2024
View reviewed changes
@ithinuel
Copy link
Collaborator

Thank you @tomi-font for creating this!
Would you mind adding myself & @wearyzen to the collaborator list please ?

@tomi-font
Copy link
Collaborator Author

Thank you @tomi-font for creating this!
Would you mind adding myself & @wearyzen to the collaborator list please ?

Sure. How about I give you the maintainer hat as well?

@tomi-font tomi-font force-pushed the cmsis6_module_addition branch from 502c888 to a525720 Compare November 26, 2024 12:55
@tomi-font tomi-font requested a review from ithinuel November 26, 2024 12:56
henrikbrixandersen
henrikbrixandersen requested changes Nov 26, 2024
View reviewed changes
MAINTAINERS.yml Outdated
collaborators:
- tomi-font
- wearyzen
- henrikbrixandersen
Copy link
Member

Choose a reason for hiding this comment

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

Please remove me from here.

@tomi-font
manifest: add cmsis_6
76fa6d6 
Add the CMSIS v6 repo as a module.

Signed-off-by: Tomi Fontanilles <[email protected]>
@tomi-font tomi-font force-pushed the cmsis6_module_addition branch from a525720 to 76fa6d6 Compare November 26, 2024 13:20
tejlmand
tejlmand requested changes Nov 26, 2024
View reviewed changes
Copy link
Collaborator

@tejlmand tejlmand left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please explain why we should have both CMSIS (v5) and CMSIS_6 in the manifest.

Unless a good explanation is given, then I think the cmsis 5 entry should be removed from the manifest.

Ref:

zephyr/west.yml

Lines 118 to 122 in 6d87bd6

- name: cmsis
revision: 4b96cbb174678dcd3ca86e11e1f24bc5f8726da0
path: modules/hal/cmsis
groups:
- hal

west.yml
@@ -126,6 +126,10 @@ manifest:
- name: cmsis-nn
revision: ea987c1ca661be723de83bd159aed815d6cbd430
path: modules/lib/cmsis-nn
- name: cmsis_6
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we really want to have both CMSIS 5 and CMSIS 6 in the manifest ?
and if yes, why ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#77220 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, that piece of info had been nice to have in the PR description.

you can still add it, so that others easily find it and not have to scroll through all comments 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah true. I had already linked the issue from this PR but it's not that visible without the text in the PR description.

tejlmand
tejlmand approved these changes Nov 26, 2024
View reviewed changes
Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

lgtm.

It would be nice to know if there is any timeline to getting everything to work with cmsis v6, so that cmsis v5 can be removed from the manifest, as I find it messy to have to different cmsis versions pointed to by the manifest.

stephanosio
stephanosio approved these changes Nov 27, 2024
View reviewed changes
Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

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

Far from ideal; but, not everything can be ideal.

@nashif nashif removed the DNM This PR should not be merged (Do Not Merge) label Dec 15, 2024
@tomi-font tomi-font mentioned this pull request Dec 16, 2024
Closed
@kartben kartben merged commit f930d97 into zephyrproject-rtos:main Dec 16, 2024
27 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pdgendt pdgendt pdgendt left review comments

@henrikbrixandersen henrikbrixandersen henrikbrixandersen approved these changes

@ithinuel ithinuel ithinuel approved these changes

@tejlmand tejlmand tejlmand approved these changes

@stephanosio stephanosio stephanosio approved these changes

@nordicjm nordicjm nordicjm left review comments

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@nashif nashif Awaiting requested review from nashif

@d3zd3z d3zd3z Awaiting requested review from d3zd3z

@carlescufi carlescufi Awaiting requested review from carlescufi

Assignees
No one assigned
Labels
area: Process manifest manifest-cmsis_6
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Module for CMSIS v6