Support for STM32 #302
Support for STM32 #302
Conversation
naichenzhao
force-pushed
the
stm32
branch
3 times, most recently
from
736320a
to
625aafd
Compare
|
@lhstrh marked this as ready for review, but I'm not sure I remember why since IIRC we agreed that we are waiting for a cross-compilation smoke test (just to show STM32 support building successfully on a regular laptop). I will review this when the tests are passing and such a smoke test is available; feel free to ping me if I forget. |
|
I was being optimistic. We hit some snags with setting up tests. |
There was a problem hiding this comment.
@naichenzhao, based on a cursory review, I think that we should try to get this merged soon, but, please:
- sanitize your comments :-) (suggestions provided)
- resolve the conflicts
@elgeeko1 (ex-BlackRock Neurotech) is willing to do a deeper review, which you might appreciate given the emphasis of safety in your project. If you can do the requested clean ups this weekend, he'll complete the review early next week. Let's aim to have this merged by the end of next week.
There was a problem hiding this comment.
I added comments inline, and my general comments follow. At a high level, the most difficult aspects of this implementation are understanding concurrent access to volatile variables and enabling/disabling interrupts. Without more documentation, it's hard to know if these mechanisms are safe.
- Reference counting for enabling / disabling interrupts: It's not clear how reference counting is managed, and whether or not it is handled in a way that is safe. Can you add documentation that indicates when this value changes and what could lead to an error?
_lf_single_threaded_notify_of_eventis volatile, but it appears it's accessed only by a single-threaded LF application. Is this variable accessed by an ISR?- Unclear ownership of
_lf_num_nested_crit_sec: It's unclear when this value is incremented and decremented. Consequently, it's possible that race conditions exist that can result in data corruption (two processes write to the value at the same time). Can you comment more about the use of this variable, who accesses it when, and how to guarantee that it is accessed in a predictable way? - Consider the names
lf_critical_section_enterandlf_critical_section_exitinstead oflf_disable_interupts_nestedandlf_enable_interrupts_nested. This conveys that purpose of the method more clearly. - Inconsistent return values. Zero is consistently success, in some cases 1 is an error, others -1 is an error. Consider standardizing on 1 as success (as it evaluates to true in boolean expressions), and zero as failure.
- Remove references to "I" or the thought process that lead you to the code. Instead, document what happens and why it is implemented this way. This is the most concise way for another developer to understand the code.
I hope this is helpful towards hardening your application. I will be happy to review any new updates.
core/platform/lf_STM32f4_support.c
Outdated
| * and force it to reset once. Its really jank but its the only way I could | ||
| * find to make it work | ||
| */ | ||
| TIM5->CNT = 0xFFFFFFFE; |
There was a problem hiding this comment.
A timer prescaler is typically loaded without a guard, meaning that it won't take event until the next update event of the clock. This is meant to ensure the prescaler doesn't take effect mid-phase, which could cause strange artifacts in your application. When you enable the peripheral, it will start with whatever was in the prescaler register at the time it was enabled, possibly leading to a large delay before the timer ISR fires. The new prescaler value will be loaded after the preceding update event.
I'm not intimately familiar with this chipset, but generally hardware peripherals are configured first and then enabled. You may consider loading the prescaler before you enable the counter. If that doesn't work, a reset is fine, but I suggest a simpler comment "reset to effect new prescaler".
This is important in safety-critical systems, you need to know when your I/O is stable and safe to use. This is known as "I/O stability".
| #define PRINTF_MICROSTEP "%d" | ||
|
|
||
| #define LF_TIME_BUFFER_LENGTH 80 | ||
| #define _LF_TIMEOUT 1 |
There was a problem hiding this comment.
What units is this #def? Should it be a const instead?
There was a problem hiding this comment.
I kinda just copied this over when I compared it against the rp2040 platform - I wasnt sure what this was used for so I kept it just incase. If its not used in LF proper, ill just delete it
WalkthroughThe changes introduce support for the STM32F4 platform in the build system and source code. This includes new configuration files, conditional compilation directives, and specific implementations for clock management, sleep operations, and interrupt handling. The enhancements allow the Lingua Franca runtime to manage hardware-specific functions for STM32F4-based systems, improving its versatility and hardware compatibility. 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 Configration File (
|
lhstrh
dismissed stale reviews from elgeeko1 and themself
Thanks for you help with this, Jeff!
|
Note that the only test failing is due to code formatting issues. Please run |
There was a problem hiding this comment.
Hi @naichenzhao, this looks promising. I have a few requests for changes. I think the most important is to support low-power sleep_until.
Do you have any docs for how to install SDK and the right compiler so I can test it out at my end?
| do { | ||
| _lf_clock_gettime(&now); | ||
| // Exit when the timer is up or there is an exception | ||
| } while (!_lf_async_event && (now < wakeup_time)); |
There was a problem hiding this comment.
This should be low-power sleep, not busy-waiting. Is there anything in the HAL for doing low-power sleep?
There was a problem hiding this comment.
There seems to be a low power sleep mode, but its only interrupt based. It seems most timed delay implementations just use a while loop. Im not sure it itll be worth it trying to create some workaround now? or just merge now and maybe improve the implementation later.
Co-authored-by: erling <erling.jellum@gmail.com>
Co-authored-by: erling <erling.jellum@gmail.com>
Co-authored-by: erling <erling.jellum@gmail.com>
Co-authored-by: erling <erling.jellum@gmail.com>
Co-authored-by: erling <erling.jellum@gmail.com>
Summary by CodeRabbit
New Features
Documentation