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

Improvements of support for watchdogs #209

Merged
merged 70 commits into from
May 17, 2023
Merged

Improvements of support for watchdogs #209

merged 70 commits into from
May 17, 2023

Conversation

edwardalee
Copy link
Contributor

@edwardalee edwardalee commented May 15, 2023

This PR, co-authored by @Benichiwa , is a companion to PR 1730 in lingua-franca, which adds watchdog support for the C target. It also factors out the watchdog support into its own .c and .h files.

@edwardalee edwardalee mentioned this pull request May 15, 2023
@lhstrh lhstrh changed the title Watchdogs eal2 Support for watchdogs May 15, 2023
lhstrh
lhstrh previously requested changes May 15, 2023
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

This is looking pretty good! My comments are minor...

core/reactor_common.c Outdated Show resolved Hide resolved
core/reactor_common.c Show resolved Hide resolved
core/threaded/watchdog.c Outdated Show resolved Hide resolved
core/threaded/watchdog.c Outdated Show resolved Hide resolved
core/threaded/watchdog.c Show resolved Hide resolved
@lhstrh
Copy link
Member

lhstrh commented May 15, 2023

Here are some more recommendations regarding conditional compilation:

Prefer to compile out entire functions, rather than portions of functions or portions of expressions. Rather than putting an ifdef in an expression, factor out part or all of the expression into a separate helper function and apply the conditional to that function.

If you have a function or variable which may potentially go unused in a particular configuration, and the compiler would warn about its definition going unused, mark the definition as __maybe_unused rather than wrapping it in a preprocessor conditional. (However, if a function or variable always goes unused, delete it.)

Within code, where possible, use the IS_ENABLED macro to convert a Kconfig symbol into a C boolean expression, and use it in a normal C conditional:

if (IS_ENABLED(CONFIG_SOMETHING)) {
        ...
}

The compiler will constant-fold the conditional away, and include or exclude the block of code just as with an #ifdef, so this will not add any runtime overhead. However, this approach still allows the C compiler to see the code inside the block, and check it for correctness (syntax, types, symbol references, etc). Thus, you still have to use an #ifdef if the code inside the block references symbols that will not exist if the condition is not met.

At the end of any non-trivial #if or #ifdef block (more than a few lines), place a comment after the #endif on the same line, noting the conditional expression used. For instance:

#ifdef CONFIG_SOMETHING
...
#endif /* CONFIG_SOMETHING */

This is from: https://www.kernel.org/doc/html/v4.10/process/coding-style.html.

@edwardalee edwardalee merged commit 9573fff into main May 17, 2023
13 of 25 checks passed
@edwardalee edwardalee deleted the watchdogs-eal2 branch May 18, 2023 18:17
@lhstrh lhstrh added the feature New feature label Aug 28, 2023
@lhstrh lhstrh changed the title Support for watchdogs Improvements of support for watchdogs Aug 28, 2023
@lhstrh lhstrh added enhancement Enhancement of existing feature and removed feature New feature labels Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants