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

Refactor example code #12

Merged
merged 10 commits into from
Jul 21, 2023
Merged

Refactor example code #12

merged 10 commits into from
Jul 21, 2023

Conversation

glneo
Copy link
Contributor

@glneo glneo commented Jul 12, 2023

This helps make these example files shorter and less intimidating for new users. Useful when trying to showcase OpenAMP to new users and teams.

@arnopo
Copy link
Collaborator

arnopo commented Jul 18, 2023

@TanmayShah-xilinx : could you confirm that the PR does not introduce regression on your platform?

examples/linux/common/common.c Show resolved Hide resolved
examples/linux/common/common.c Show resolved Hide resolved
@@ -1,6 +1,6 @@

APP = echo_test
APP_OBJS = echo_test.o
APP_OBJS = echo_test.o ../common/common.o
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for your patch.

We should include common.o in clean target as well.
So when we do make clean, it re-builds common.o as well.
or probably introduce new target (distclean), that will remove common.o as well. clean will only remove local directory *.o and distclean will remove all dependencies as well such as common.o. I am fine with either approach.

Thanks.

Copy link
Contributor Author

@glneo glneo Jul 19, 2023

Choose a reason for hiding this comment

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

I'm not sure how the 'clean' option would interact when one wanted only to clean the current project but I'd assume that would be rare anyway. Easy enough to just use APP_OBJS as the rm targets.

Fixing out of tree builds (or using CMake or similar) would fix all that, but that would be a task for another day :)

Copy link
Collaborator

@tnmysh tnmysh left a comment

Choose a reason for hiding this comment

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

LGTM

@tnmysh
Copy link
Collaborator

tnmysh commented Jul 18, 2023

@TanmayShah-xilinx : could you confirm that the PR does not introduce regression on your platform?

@arnopo I verified matrix multiply and proxy app demo. It is working fine without introducing regressions on zcu102 qemu platform. I have some comments, but apart from that it looks good to me.

glneo added 10 commits July 19, 2023 14:06
Signed-off-by: Andrew Davis <afd@ti.com>
Signed-off-by: Andrew Davis <afd@ti.com>
Make matrix_print() into a common function and use it for both printing
the input and output matrices. Also add 3 wide padding so the output
looks more like a table when the numbers are not all the same length.

Signed-off-by: Andrew Davis <afd@ti.com>
Usually considered bad practice and the code is more readable without.

Signed-off-by: Andrew Davis <afd@ti.com>
Signed-off-by: Andrew Davis <afd@ti.com>
The file should be closed when returning from this function in the error
path. Fix this here.

Signed-off-by: Andrew Davis <afd@ti.com>
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.

LGTM

@arnopo arnopo merged commit db1f74a into OpenAMP:main Jul 21, 2023
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