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

apps: Move common function declaration to common header #564

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

glneo
Copy link
Contributor

@glneo glneo commented Mar 15, 2024

Several platform_*() functions are common across the example machines. They actually have to be as they are consumed by example apps that build across these machines. Move these common declarations to common a header.

Several platform_*() functions are common across the example machines.
They actually have to be as they are consumed by example apps that build
across these machines. Move these common declarations to common a header.

Signed-off-by: Andrew Davis <afd@ti.com>
@tnmysh
Copy link
Collaborator

tnmysh commented Mar 15, 2024

Hi @glneo,

We are not taking new changes in apps Xilinx platform specific directories.
This code should be moved to openamp-system-reference.

So, this refactoring can be avoided.

@glneo
Copy link
Contributor Author

glneo commented Mar 15, 2024

Yes it should be moved, I was the one who originally suggested that (I'm Andrew with TI in case my username hid that).

But this fix has been bugging me and I got tired of waiting until these examples are moved. Looks like the current plan is to move them over by October 2024 so only 7 more months from now.. Would like to just fix it here first.

@tnmysh
Copy link
Collaborator

tnmysh commented Mar 15, 2024

Yes it should be moved, I was the one who originally suggested that (I'm Andrew with TI in case my username hid that).

Yeah I got that.

But this fix has been bugging me and I got tired of waiting until these examples are moved.

In such case, right PR would be to move demos to openamp-system-reference and removing it from here. Few PRs in apps area have been rejected before for the same reason. I believe correct PR would be to remove it from this repo first. We can have it as part of #501.

Looks like the current plan is to move them over by October 2024 so only 7 more months from now.. Would like to just fix it here first.

@glneo
Copy link
Contributor Author

glneo commented Mar 15, 2024

In such case, right PR would be to move demos to openamp-system-reference and removing it from here. Few PRs in apps area have been rejected before for the same reason. I believe correct PR would be to remove it from this repo first. We can have it as part of #501.

Would you like me to do that then? I've asked if I could make that change here before but was told some Xilinx internal testing still depended on it and so y'all would move it. That was 8 months ago..

@tnmysh
Copy link
Collaborator

tnmysh commented Mar 19, 2024

In such case, right PR would be to move demos to openamp-system-reference and removing it from here. Few PRs in apps area have been rejected before for the same reason. I believe correct PR would be to remove it from this repo first. We can have it as part of #501.

Would you like me to do that then? I've asked if I could make that change here before but was told some Xilinx internal testing still depended on it and so y'all would move it. That was 8 months ago..

Xilinx's internal testing is moved to downstream repo. Also, there was discussion started to create platform agnostic baremetal demos in openamp-system-reference. I prefer not to add/remove any apps code in Xilinx's platform until new demos are available in openamp-system-reference. However, I leave decision to others.

@arnopo arnopo requested a review from tnmysh March 21, 2024 17:43
@arnopo
Copy link
Collaborator

arnopo commented Mar 29, 2024

@glneo @tnmysh
I propose to discuss this point in next system reference meeting.

Copy link
Collaborator

@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.

A discussed in system-reference meeting waiting migration of the samples and tests in system-reference repo we can merge this one ( after @tnmysh approve)

@arnopo arnopo added this to the Release V2024.04 milestone Apr 23, 2024
@arnopo arnopo merged commit fd6a6fd into OpenAMP:main Apr 23, 2024
3 checks passed
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.

4 participants