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

modules: Zcbor #43332

Merged
merged 2 commits into from
Apr 6, 2022
Merged

modules: Zcbor #43332

merged 2 commits into from
Apr 6, 2022

Conversation

de-nordic
Copy link
Collaborator

The commit adds zcbor module.

@de-nordic de-nordic added the DNM This PR should not be merged (Do Not Merge) label Mar 1, 2022
@github-actions github-actions bot added area: Modules manifest west manifest-zcbor and removed DNM This PR should not be merged (Do Not Merge) labels Mar 1, 2022
@github-actions
Copy link

github-actions bot commented Mar 1, 2022

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
zcbor N/A zephyrproject-rtos/zcbor@317d3c6 (main) N/A

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

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.

minor observations

@@ -0,0 +1,17 @@
if(CONFIG_ZCBOR)
add_library(ZCBOR INTERFACE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a specific reason for creating a ZCBOR interface lib ?
Are applications expecting to link directly to the interface lib or not, or is it just because you copied code from elsewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes they area supposed to link directly. And yes I have copied the code.
For example mcumgr that currently links TinyCBOR would be linking this one.

west.yml Outdated
Comment on lines 242 to 243
- name: zcbor
remote: nordic
Copy link
Collaborator

Choose a reason for hiding this comment

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

when ready to be taken out of draft we should have a Zephyr fork here: https://github.com/zephyrproject-rtos

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but that requires #43333 to go first.

nordicjm
nordicjm previously approved these changes Mar 3, 2022

config ZCBOR
bool "zcbor library"
depends on !MINIMAL_LIBC
Copy link
Member

Choose a reason for hiding this comment

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

We have to be able to use both minimal libc and newlib with zcbor. The fix here would be to add the required ranges in minimal libc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed

Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

NACKing until the C library issue is resolved

The commit adds zcbor module to the manifest.

Signed-off-by: Dominik Ermel <[email protected]>
The commit adds files for the zcbor to be built as module.

Signed-off-by: Dominik Ermel <[email protected]>
@de-nordic de-nordic marked this pull request as ready for review April 5, 2022 17:36
@de-nordic de-nordic requested a review from nashif as a code owner April 5, 2022 17:36
@carlescufi
Copy link
Member

@lairdjm can you give a +1 so we can then continue in #43878?

Comment on lines +4 to +10
config ZEPHYR_ZCBOR_MODULE
bool

config ZCBOR
bool "zcbor library"
help
Enable zcbor CBOR encoder/decoder library
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I think we can do that, yes. I wonder however if it'd be better to just have that Kconfig file in the zcbor repo and then orsource it from nrf/zephyr?

Copy link
Member

Choose a reason for hiding this comment

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

See #44949

@@ -237,6 +237,9 @@ manifest:
path: modules/tee/tf-m/psa-arch-tests
groups:
- tee
- name: zcbor
revision: 317d3c6cd6dee23a151a3e7982d8564e7e19b2e2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could be 0.4.0 now.

@carlescufi carlescufi merged commit 22c32cf into zephyrproject-rtos:main Apr 6, 2022
@de-nordic de-nordic deleted the zcbor-module branch April 7, 2022 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants