search

OpenAMP / open-amp

The main OpenAMP library implementing RPMSG, Virtio, and Remoteproc for RTOS etc
https://www.openampproject.org/
Other
678 stars 278 forks source link

apps: Move common function declaration to common header #564

Closed glneo closed 2 months ago

glneo commented 3 months ago

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.

tnmysh commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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