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

Add support for Zephyr's native test environment #260

Merged
merged 3 commits into from
Nov 10, 2023

Conversation

aescolar
Copy link
Contributor

@aescolar aescolar commented Oct 11, 2023

Zephyr's native test environment is a setup which allows building the embedded SW with the Zephyr OS and HW models as a Linux executable.

When building for this target the Zephyr integration will set the PROJECT_PROCESSOR as "zephyr_native".

We need to provide appropriate headers for this "processor".
The "metal_cpu_yield()" call, which in other architectures is handled as either a hint for the CPU to yield to another thread or do nothing, and therefore busy waiting, needs replacing with a call out to the Zephyr API which will cause the CPU to waste 1 microsecond, therefore enabling the other CPU in the system to ready whatever we are waiting for in the meanwhile.

@aescolar
Copy link
Contributor Author

@arnopo I'm honestly not sure if this would be the best way to handle this, your feedback would be very welcome
CC @carlocaione

@arnopo
Copy link
Contributor

arnopo commented Oct 11, 2023

I answered in zephyrproject-rtos/libmetal#19:

Seems to me not a platform but an option in the Zephyr build.

You could try to address this in this in https://github.com/OpenAMP/libmetal/blob/main/lib/system/zephyr
and perhaps in https://github.com/OpenAMP/libmetal/edit/main/cmake/options.cmake?

#ifndef __METAL_ZEPHYR_NATIVE_ATOMIC__H__
#define __METAL_ZEPHYR_NATIVE_ATOMIC__H__

#endif /* __METAL_ZEPHYR_NATIVE_ATOMIC__H__ */
Copy link
Contributor

Choose a reason for hiding this comment

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

I would really like to replace all these do nothing headers with a single copy of a generic version. For "cpus" where both files are do nothing they can just define cpu_generic or something like that.

For this case you have something real to do in cpu.h so the cmake can select the generic-atomic.h and you can have the real cpu.h below.

Copy link
Contributor Author

@aescolar aescolar Oct 13, 2023

Choose a reason for hiding this comment

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

I can do that in a separate commit if you want, though if you would not mind, maybe in a follow up PR after we agree this other matter, as it is a bit unrelated and I would prefer to not mingle things too much.

lib/processor/zephyr_native/cpu.h Outdated Show resolved Hide resolved
@aescolar
Copy link
Contributor Author

aescolar commented Oct 13, 2023

Thanks @wmamills ,
note the discussion was happening in zephyrproject-rtos/libmetal#19
( I just updated this PR based on what was discussed there )

@aescolar aescolar force-pushed the zephyr_native_support branch from e68f993 to 37b06af Compare October 13, 2023 16:34
@aescolar
Copy link
Contributor Author

@arnopo The PR has been updated as you requested in zephyrproject-rtos/libmetal#19
You are very welcome to take a look.

@arnopo
Copy link
Contributor

arnopo commented Oct 16, 2023

Please could you re-base your PR on top of the main branch to fix CI issue

@arnopo arnopo self-requested a review October 16, 2023 08:30
Copy link
Contributor

@arnopo arnopo left a comment

Choose a reason for hiding this comment

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

The way of integrating it seems ok to me, I let other reviewers share their opinions

lib/system/zephyr/sys.h Outdated Show resolved Hide resolved
lib/system/zephyr/sys.h Show resolved Hide resolved
lib/system/zephyr/sys.h Outdated Show resolved Hide resolved
@aescolar aescolar force-pushed the zephyr_native_support branch from 153430b to e7591da Compare October 16, 2023 09:13
@aescolar
Copy link
Contributor Author

Thanks @arnopo, corrections according to your feedback have been done.

@aescolar aescolar requested a review from arnopo October 16, 2023 09:14
@arnopo arnopo added this to the Release V2024.04 milestone Oct 16, 2023
@aescolar
Copy link
Contributor Author

@arnopo & others, is there anything that should be different / that prevents this from being merged?

Background: This enables a new type of tests and way of developing for AMP scenarios for Zephyr, so there is quite a few people looking forward to it.

@arnopo
Copy link
Contributor

arnopo commented Oct 19, 2023

Hi @aescolar,
I understand the frustration, but we are in the release process of the library. Therefore, today's code is frozen until the release at the end of the month. Moreover, to merge any new changes, we require two approvals from reviewers.
if it is blocker for Zephyr, we can have an exception in Zephyr libmetal module.
In such case refresh the PR in zephyrproject-rtos/libmetal#19 explaining the need

@aescolar
Copy link
Contributor Author

Thank you very much for the consideration @arnopo . I set the PR in the zephyr module as ready, and provided the motivation in this comment: zephyrproject-rtos/libmetal#19 (comment)
Getting this changes in would be very nice.

@arnopo
Copy link
Contributor

arnopo commented Oct 31, 2023

@wmamills, @tnmysh, @edmooring
Please Could you have a look?

@wmamills
Copy link
Contributor

I still don't want to create yet another ALMOST do nothing "processor". Can we have a PR to do the generic files first and then this can use them. Instead of adding the fake "processor" and then removing it.

Provide the metal_sleep_usec() required for hosted
environments.

Signed-off-by: Alberto Escolar Piedras <alberto.escolar.piedras@nordicsemi.no>
@aescolar aescolar force-pushed the zephyr_native_support branch 2 times, most recently from a73fc2a to c936074 Compare November 1, 2023 08:41
@aescolar
Copy link
Contributor Author

aescolar commented Nov 1, 2023

@wmamills is something like this what you were hoping for?


A Zephyr CI run with the equivalent of this patch can be seen passing in
zephyrproject-rtos/zephyr#64674

Add a new hosted test environment "processor".

This is meant as a build target used when the
code is not built for a real target
but as part of a test in a hosted environment
(for ex. as a test environment like Zephyr's
native_sim target).

When building for this target PROJECT_PROCESSOR
should be set as "hosted".

In this, the "metal_sleep_usec()" call
is expected to be provided by the system folder
headers.

Signed-off-by: Alberto Escolar Piedras <alberto.escolar.piedras@nordicsemi.no>
@aescolar aescolar force-pushed the zephyr_native_support branch from c936074 to b19688f Compare November 1, 2023 10:30
Refactor all processor headers which had the same content
into a generic set, to reduce the number of duplicates.

Signed-off-by: Alberto Escolar Piedras <alberto.escolar.piedras@nordicsemi.no>
@aescolar aescolar force-pushed the zephyr_native_support branch from b19688f to 56366e3 Compare November 1, 2023 10:47
@wmamills
Copy link
Contributor

wmamills commented Nov 1, 2023

@aescolar Yes that look great! Lots of code deletion!
Let me pull it down and look more closely.

At libmetal I would like to land the commits in the other order: generic first and then Zephyr test env.
Do you want to do that or do you want me to do that (and give you credit)

Question: Is zephyr native posix always == test environment or are there some cases where you would not want the sleep for the yield?

@aescolar
Copy link
Contributor Author

aescolar commented Nov 1, 2023

@wmamills Thanks

Question: Is zephyr native posix always == test environment or are there some cases where you would not want the sleep for the yield?

Native_posix/native_sim and the related targets are test/simulation/development environment.
We always want to sleep a little bit in this metal_cpu_yield() case.

@aescolar
Copy link
Contributor Author

aescolar commented Nov 7, 2023

@wmamills are you happy enough with this PR as to approve it? :)

@wmamills
Copy link
Contributor

@aescolar @arnopo yes I am OK with this and even in this order. I guess it make sense as this shows more linear history from a Zephyr POV.

Copy link
Contributor

@wmamills wmamills left a comment

Choose a reason for hiding this comment

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

I approve

@arnopo arnopo merged commit 85fb139 into OpenAMP:main Nov 10, 2023
3 checks passed
@aescolar
Copy link
Contributor Author

Thanks guys :)

@aescolar aescolar deleted the zephyr_native_support branch November 10, 2023 13:06
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.

3 participants