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

Cordio lib #492

Merged
merged 38 commits into from
Mar 21, 2023
Merged

Cordio lib #492

merged 38 commits into from
Mar 21, 2023

Conversation

kevin-gillespie
Copy link
Contributor

Building a Cordio library. I used the peripheral driver layout as a reference.

TODO: Select files that need to be re-built for each application.

@kevin-gillespie
Copy link
Contributor Author

/clang-format-run

1 similar comment
@kevin-gillespie
Copy link
Contributor Author

/clang-format-run

kevin-gillespie and others added 14 commits March 15, 2023 22:01
This will allow us to build application with our without trace, without
experiencing link errors. We will still have an issue with code size if
the library is built with TRACE=2 and the application is built with
TRACE=0. We won't see the trace messages, but the code size will be
elevated.

In the case where we go from TRACE=0 to TRACE=2, we won't see the
print statements at all.
@kevin-gillespie kevin-gillespie marked this pull request as ready for review March 17, 2023 20:26
@@ -218,7 +218,31 @@ void WsfTrace(const char *pStr, ...)
}
}
}
#endif
#if WSF_TRACE_ENABLED_VERBOSE == TRUE
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will allow the build to pass, but will cause problems when the trace options change. Still not sure which outcome is better.

TRACE options could require a re-build between applications. If the library is built with TRACE=0 it will be optimized for code size and remove all of the trace messaging. Adjacent applications will still build and link with TRACE=1, but the trace messages will not be printed until the library is re-built.

If the library is built with TRACE=2 and adjacent applications build with TRACE=0, the messages will not be printed, but the text will remain in the code space until the library is rebuilt with the appropriate option.

@kevin-gillespie
Copy link
Contributor Author

Pay special attention to Libraries/Cordio, specifically the build folder.

@kevin-gillespie
Copy link
Contributor Author

@Jake-Carter @EdwinFairchild

This is ready for review. Please focus on the changes made to Libraries/Cordio. I'm most concerned about the TRACE changes.

Copy link
Contributor

@Jake-Carter Jake-Carter left a comment

Choose a reason for hiding this comment

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

@kevin-gillespie From a build system perspective I think this looks good and is a major improvement.

Since TRACE affects the build significantly, I made a small tweak so the different trace levels build as their own variants. See 7c8efb4

This makes TRACE changes transparent to the user and removes some documentation overhead. project.mk can be updated to remove the comment about rebuilding Cordio on TRACE changes. What do you think?

Copy link
Contributor

@EdwinFairchild EdwinFairchild left a comment

Choose a reason for hiding this comment

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

this does not require anything new to passed on to CI test right? since everything is built from scratch.

@kevin-gillespie
Copy link
Contributor Author

this does not require anything new to passed on to CI test right? since everything is built from scratch.

Correct. We shouldn't have to modify the CI.

@kevin-gillespie
Copy link
Contributor Author

@kevin-gillespie From a build system perspective I think this looks good and is a major improvement.

Since TRACE affects the build significantly, I made a small tweak so the different trace levels build as their own variants. See 7c8efb4

This makes TRACE changes transparent to the user and removes some documentation overhead. project.mk can be updated to remove the comment about rebuilding Cordio on TRACE changes. What do you think?

I thought about this and I think it will work fine. My concern is that the variants will grow out of control if we're not careful.

I will update the comments and docs to reflect this.

@Jake-Carter
Copy link
Contributor

@kevin-gillespie From a build system perspective I think this looks good and is a major improvement.
Since TRACE affects the build significantly, I made a small tweak so the different trace levels build as their own variants. See 7c8efb4
This makes TRACE changes transparent to the user and removes some documentation overhead. project.mk can be updated to remove the comment about rebuilding Cordio on TRACE changes. What do you think?

I thought about this and I think it will work fine. My concern is that the variants will grow out of control if we're not careful.

I will update the comments and docs to reflect this.

Yeah we definitely don't want to over-rely on this. I also kicked around the idea of writing all the options into a text file for each build. We could update the rule to build the library to compute a hash for the old file vs the new file. On hash difference we trigger a full rebuild. This adds a lot of external dependencies to the rule though. Maybe a task for a future PR, but could be applicable towards other libs as well.

@kevin-gillespie kevin-gillespie merged commit 185bcfc into main Mar 21, 2023
@kevin-gillespie kevin-gillespie deleted the cordio-lib branch March 21, 2023 21:14
@yc-adi
Copy link
Contributor

yc-adi commented Mar 23, 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.

4 participants