Skip to content

fix zephyr conflicting types atomic - #4058

Merged
lgirdwood merged 1 commit into
thesofproject:mainfrom
hongshui3000:atomic_fix
Apr 26, 2021
Merged

fix zephyr conflicting types atomic#4058
lgirdwood merged 1 commit into
thesofproject:mainfrom
hongshui3000:atomic_fix

Conversation

@hongshui3000

@hongshui3000 hongshui3000 commented Apr 20, 2021

Copy link
Copy Markdown
Contributor

fix #4049

Signed-off-by: yxh yangxiaohuamail@gmail.com

@sofci

sofci commented Apr 20, 2021

Copy link
Copy Markdown
Collaborator

Can one of the admins verify this patch?

reply test this please to run this test once.

@gkbldcig

Copy link
Copy Markdown
Collaborator

Can one of the admins verify this patch?

Comment thread src/arch/host/include/arch/atomic.h Outdated

@dbaluta dbaluta Apr 20, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please use a prefix for your commit subject.

E.g include: Fix zephyr conflicting types

then please explain in the commit message WHY do we need this change. Do you get a compiler error?

Please give more information so that people can understand the intention of your change.

@lgirdwood
lgirdwood requested a review from lyakh April 20, 2021 19:09
@lgirdwood

Copy link
Copy Markdown
Member

@zrombel good to merge ? CI pending....

@zrombel

zrombel commented Apr 21, 2021

Copy link
Copy Markdown

PR was build and it looks taht it's good to merge.

@lgirdwood

Copy link
Copy Markdown
Member

@lyakh I assume this is good for you ? Any idea why you have not seen this ?

@iuliana-prodan

Copy link
Copy Markdown
Contributor

@lyakh I assume this is good for you ? Any idea why you have not seen this ?

@lgirdwood @lyakh I'm also working on imx support for SOF with Zephyr and I didn't encounter this issue.
For building and adding support I've followed the steps from documentation , the same as those from this pull request: #4060

@hongshui3000

hongshui3000 commented Apr 22, 2021

Copy link
Copy Markdown
Contributor Author

@lyakh I assume this is good for you ? Any idea why you have not seen this ?

@lgirdwood @lyakh I'm also working on imx support for SOF with Zephyr and I didn't encounter this issue.
For building and adding support I've followed the steps from documentation , the same as those from this pull request: #4060

This kind of issue won't occur when you just compile a simple sample. When you use src/include/sof/atomic.h (this header file may also be included by others) and zephyr at the same time. The issue will reappear。Please note that my modification is in host arch, and other platforms are not considered.Because my platform is not in the official support list

@lyakh

lyakh commented Apr 22, 2021

Copy link
Copy Markdown
Collaborator

@lyakh I assume this is good for you ? Any idea why you have not seen this ?

@lgirdwood because SOF code includes sof/atomic.h whereas Zephyr code includes sys/atomic.h... So, I'm not sure this is a complete fix. A complete one should also remove the definition of atomic_t and include sys/atomic.h instead?

Comment thread src/include/sof/atomic.h Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

wouldn't it be better and more correct to move this #ifndef 3 lines up and to #include <sys/atomic.h> on the #else branch? Or better invert the condition to #ifdef to avoid a negation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree

@hongshui3000
hongshui3000 force-pushed the atomic_fix branch 3 times, most recently from a4464c5 to 04e2655 Compare April 22, 2021 09:17
@hongshui3000

hongshui3000 commented Apr 22, 2021

Copy link
Copy Markdown
Contributor Author

I re-reveiw the sof code, the Cmakefile in the zephyr directory contains two header file paths.

target_include_directories(SOF INTERFACE ../zephyr/include)
target_include_directories(SOF INTERFACE ${SOF_SRC_PATH}/include)

When we compile sof, #include <sof/atomic.h>, then the header files under sof/zephyr will be found first. So we don't see the issue of type conflicts.
But when I compile the app, rtos zephyr includes its own atomic.h by default, and if I use sof with an atomic header, the atomic.h in the sof/src/include directory will be found at the same time. The issue I said happened.

Therefore, there is another modification scheme that does not change the sof code, but it needs to introduce the header files in the sof/zephyr/include directory when the external app is compiled, overwriting the header file path of sof itself
APP BUID:
Originally wrong Cmakefile:

zephyr_library_include_directories(sof_driver PUBLIC
    ${sof_module}/src/arch/${ARCH}/include
    ${sof_module}/src/include
  )

WORK:

zephyr_library_include_directories(sof_driver PUBLIC
    ${sof_module}/src/arch/${ARCH}/include
    ${sof_module}/zephyr/include                        -------------------------->The repetition provided by atomic and zephyr, but can 
                                                                                           overwrite the atomic of sof itself
    ${sof_module}/src/include
  )

But I don’t think this is a good way

@lgirdwood

Copy link
Copy Markdown
Member

@hongshui3000 the plan is to remove all the SOF ARCH header paths as we continue and use more and more native Zephyr APIs (today we still have some in wrapper.c) i.e. SOF include path would only be for audio APIs.
@marc-hb what is the status of your native Zephyr APIs work ? as this should solve all these problems.

Comment thread src/include/sof/atomic.h Outdated
The atomic code is implemented in zephyr. When sof is used as
a zephyr module, there is no need to reimplement atomic.
The modification here is to solve the problem of repeated
definition of atomic related content.

Signed-off-by: Yang XiaoHua <yangxiaohuamail@gmail.com>
@jgunn0262

Copy link
Copy Markdown
Contributor

I think using Zephyr is a great idea, thanks @hongshui3000.

@lgirdwood

Copy link
Copy Markdown
Member

Jenkins shows unrelated DUT boot failure.

@lgirdwood
lgirdwood merged commit 3500d0e into thesofproject:main Apr 26, 2021
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.

[BUG]zephyr conflicting types "atomic"

9 participants