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

Update CORTEX R4 demo to use linked resources #1272 #1273

Merged
merged 2 commits into from
Oct 10, 2024

Conversation

mryall-mawson
Copy link
Contributor

@mryall-mawson mryall-mawson commented Sep 26, 2024

Replace batch script in Cortex R4 demo with linked resources, to support non-Windows dev platforms.

Description

Two commits:

  • Remove the batch script, FreeRTOS/Demo/CORTEX_R4_RM48_TMS570_CCS5/CreateProjectDirectoryStructure.bat, and replace it with linked resources
  • Ignore the output directories and Eclipse user settings files in this demo

Test Steps

I tested this by doing a clean build of the demo project with Code Composer Studio 12. It compiles successfully with the symlinks instead of copied files.

I've also tested it on a TMS570-based board and it loaded, ran and debugged successfully.

Checklist:

  • [ x ] I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

#1272

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@@ -0,0 +1,59 @@
#!/bin/sh
Copy link
Member

Choose a reason for hiding this comment

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

Should we instead remove both these scripts and use linked resources instead?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I would second this. Just symlink the resources that are copied and get rid of the scripts. The only real caveat against this is that prior to Vista, Windows did not support symlinks, but I would consider that to be a negligible issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I will update this PR with that change instead. I have it working locally after a bit of messing around with relative paths.

Where does the documentation live? This section in the docs will also need updating since this script will no longer exist.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @mryall-mawson,
I will update the documentation on the website for you.
Best,
Jason Carroll

@mryall-mawson mryall-mawson changed the title Update CORTEX R4 demo script to support non-Windows platforms #1272 Update CORTEX R4 demo to use symlinks instead of copying dependencies #1272 Sep 27, 2024
jasonpcarroll
jasonpcarroll previously approved these changes Sep 27, 2024
@mryall-mawson
Copy link
Contributor Author

The "Kernel Unit Tests" build is failing here due to an unrelated problem, which I've raised as #1274.

I have opened PR #1275 to fix the build issue, If we can get that approved, we can check it fixes the build, then merge it to get main green before proceeding with this one. Thanks!

@aggarg
Copy link
Member

aggarg commented Sep 30, 2024

Instead of keeping the symboic links, I was thinking of using Eclipse's linked folder and resource filters to filter the files in those folders. Please try the following patch and let me know what you think about it - 0001-Update-CORTEX_R4_RM48_TMS570_CCS5-project.patch.

Note that I used CCS v12.

@mryall-mawson
Copy link
Contributor Author

Instead of keeping the symboic links, I was thinking of using Eclipse's linked folder and resource filters ...

Ah okay. I've updated the PR to use this approach. There were a few problems with your patch that I've fixed up:

  • The patch imported the FreeRTOS folder as "FreeRTOS-Kernel", but that path wasn't updated in the include paths for three of the four build configurations. So that broke the build for the TMS570 target I was testing. I restored the resource link to the previous name of just "FreeRTOS", since it means fewer changes and the directory also includes some non-kernel stuff nested under as well.
  • The filtering included all the Demo/Common/Minimal/*.c files, which caused quite a few compilation errors for me. I updated the filter to just include the files which were copied by the previous script.
  • Likewise, there was no filtering on the kernel *.c files, so I added filters for just the files needed.
  • The patch didn't delete the old batch file, which is no longer needed.

Testing note: I found that Eclipse retains some state for linked resources even after the resource is removed from the .project file and the CCS project is closed and reopened. In my case, I had to manually remove the FreeRTOS-Kernel link after it was renamed to just FreeRTOS. Perhaps a restart of CCS would have fixed it; I didn't try that.

Please try the following patch and let me know what you think about it …

After playing around with it, I think this approach relies on a lot of IDE magic and obscure settings screens. So I would probably find the symlinks easier to grok coming at the project fresh. But it's your project, so happy to go with your recommendation.

Note that I used CCS v12.

I'm also using CCS 12. Perhaps we should remove the "_CCS5" suffix from the demo project and path name? I don't think preserving compatibility with an old, unsupported IDE version should be a goal for this demo. I'd suggest doing that as a separate ticket+PR however, so we don't delay this work with further changes.

@mryall-mawson
Copy link
Contributor Author

I also tested this on real TMS570 hardware today (using the symlinked version), and it worked fine.

@mryall-mawson mryall-mawson changed the title Update CORTEX R4 demo to use symlinks instead of copying dependencies #1272 Update CORTEX R4 demo to use linked resources #1272 Sep 30, 2024
@mryall-mawson
Copy link
Contributor Author

Updates complete - please re-review when you get a chance, thanks @aggarg @jasonpcarroll.

@aggarg
Copy link
Member

aggarg commented Sep 30, 2024

There were a few problems with your patch that I've fixed up:

Thank you for addressing these problems. I guess I made these changes for only one target and did not realize that there were 3 more.

When I built the project with the current changes, I needed to update the "Runtime support library" to automatic so that CSS builds the library when the project is used first time after installation - 0001-Set-library-management-to-autometic.patch.

Please take a look at this and let me know what you think. Thank you for your contribution!

@aggarg
Copy link
Member

aggarg commented Oct 7, 2024

@mryall-mawson Did you get a chance to look at the change above?

@mryall-mawson
Copy link
Contributor Author

That sounds like a handy fix. I had to work out how to do that manually with mklib, which was quite a pain.

Unfortunately I haven’t had time to review or test it yet. I’m at a conference next week as well, so it may be two weeks until I get a chance.

We could potentially split this into a separate issue and PR if you want?

@aggarg
Copy link
Member

aggarg commented Oct 10, 2024

Thank you for confirming. Lets do it this way. And thank you for your contribution!

@DakshitBabbar DakshitBabbar self-requested a review October 10, 2024 09:50
@DakshitBabbar DakshitBabbar merged commit 6dc7737 into FreeRTOS:main Oct 10, 2024
25 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