-
Notifications
You must be signed in to change notification settings - Fork 24
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
Support for Patmos platform #383
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. There has been some changes to the reactor-c layout since you started so you need to merge your changes to the CMakeLists.txt into the new CMakeLists.txt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please fix the merge conflicts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but as Marten says, you need to merge main
into your branch and resolve any conflict. Also, you should run our clang-format. You need to install it, and the run make format
in the root of reactor-c
f4d1af2
to
b3dcbac
Compare
Note that the Arduino tests are failing with this following error:
|
You need an |
@EhsanKhodadad and @erlingrj could you guys please go over the conversations and resolve whatever issues have been resolved? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. We are still missing support in lfc for generating a CMakeListst.txt that integrates with the Patmos SDK and toolchain.
@EhsanKhodadad, could you respond to this? It seems we're super close to merging this, so let's try to get this across the finish line. |
WalkthroughThe recent changes enhance platform support for the Patmos architecture within the Lingua Franca framework. Key updates include new header files providing necessary APIs, modifications to CMake build configurations, and the introduction of critical support functions for managing interrupts and timing on the Patmos platform. These improvements aim to expand compatibility without impacting existing functionality or logic. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (2)
low_level_platform/api/platform/lf_patmos_support.h (1)
51-52
: Use standard format for closing include guard.Ensure consistency with the standard format.
- #endif // LF_PATMOS_SUPPORT_H + #endif /* LF_PATMOS_SUPPORT_H */low_level_platform/impl/src/lf_patmos_support.c (1)
112-114
: Use standard format for closing preprocessor directive.Ensure consistency with the standard format.
- #endif // PLATFORM_PATMOS + #endif /* PLATFORM_PATMOS */
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- core/CMakeLists.txt (5 hunks)
- low_level_platform/api/low_level_platform.h (1 hunks)
- low_level_platform/api/platform/lf_patmos_support.h (1 hunks)
- low_level_platform/impl/CMakeLists.txt (3 hunks)
- low_level_platform/impl/src/lf_atomic_irq.c (1 hunks)
- low_level_platform/impl/src/lf_patmos_support.c (1 hunks)
Files skipped from review due to trivial changes (1)
- core/CMakeLists.txt
Additional comments not posted (8)
low_level_platform/api/platform/lf_patmos_support.h (2)
38-42
: Verify the necessity of the NO_TTY macro.Ensure that the
NO_TTY
macro is required for the Patmos platform.
44-50
: LGTM!The include statements and format specifiers are appropriate.
low_level_platform/impl/src/lf_atomic_irq.c (1)
1-1
: LGTM! Verify the integration.The new preprocessor directive is correctly integrated.
Ensure that the integration does not affect existing functionality.
Verification successful
The PLATFORM_PATMOS directive is correctly integrated and follows the same pattern as other platform-specific directives.
low_level_platform/api/low_level_platform.h
low_level_platform/impl/src/lf_patmos_support.c
low_level_platform/impl/src/lf_atomic_irq.c
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of the new PLATFORM_PATMOS directive. # Test: Search for the PLATFORM_PATMOS directive. Expect: Correct integration without affecting existing functionality. rg --type c -A 5 $'PLATFORM_PATMOS'Length of output: 1709
Script:
#!/bin/bash # Description: Verify conditional compilation blocks and usage of PLATFORM_PATMOS directive. # Search for conditional compilation blocks in low_level_platform/api/low_level_platform.h rg --type c -A 5 '#elif defined(PLATFORM_PATMOS)' low_level_platform/api/low_level_platform.h # Search for conditional compilation blocks in low_level_platform/impl/src/lf_patmos_support.c rg --type c -A 5 '#if defined(PLATFORM_PATMOS)' low_level_platform/impl/src/lf_patmos_support.c # Search for conditional compilation blocks in low_level_platform/impl/src/lf_atomic_irq.c rg --type c -A 5 'defined(PLATFORM_PATMOS)' low_level_platform/impl/src/lf_atomic_irq.cLength of output: 283
Script:
#!/bin/bash # Description: Verify the usage and integration of the PLATFORM_PATMOS directive with broader context. # Search for PLATFORM_PATMOS directive and its surrounding context in low_level_platform/api/low_level_platform.h rg --type c -C 5 'PLATFORM_PATMOS' low_level_platform/api/low_level_platform.h # Search for PLATFORM_PATMOS directive and its surrounding context in low_level_platform/impl/src/lf_patmos_support.c rg --type c -C 5 'PLATFORM_PATMOS' low_level_platform/impl/src/lf_patmos_support.c # Search for PLATFORM_PATMOS directive and its surrounding context in low_level_platform/impl/src/lf_atomic_irq.c rg --type c -C 5 'PLATFORM_PATMOS' low_level_platform/impl/src/lf_atomic_irq.cLength of output: 1392
low_level_platform/impl/src/lf_patmos_support.c (3)
92-110
: Ensure correct handling of nested critical sections.The logic for nested critical sections should be correctly implemented.
73-90
: Ensure up-to-date clock functionality.The clock functionality should be correctly implemented and up-to-date.
31-38
: Include missing headers.Ensure that all necessary headers are included.
+ #include "platform/lf_patmos_support.h" + #include "low_level_platform.h"Likely invalid or redundant comment.
low_level_platform/impl/CMakeLists.txt (1)
42-46
: LGTM! Patmos platform support added correctly.The conditional block for the Patmos platform is well-structured and includes the necessary source files.
low_level_platform/api/low_level_platform.h (1)
51-52
: LGTM! Patmos platform header included correctly.The
#elif
block for the Patmos platform is well-structured and includes the necessary header file.
Co-authored-by: Marten Lohstroh <[email protected]>
Co-authored-by: Marten Lohstroh <[email protected]>
Co-authored-by: erling <[email protected]>
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores