hslink: add button trigger reset #67
Conversation
WalkthroughThis change adds the MultiTimer component as a new Git submodule and integrates it into the build system. It introduces new source and header files implementing hardware reset functions, relocating reset logic from existing files. Button event handling is migrated from manual polling to timer-driven callbacks using MultiTimer. Build configuration and header inclusions are updated accordingly. Changes
Sequence Diagram(s)Button Single Click Reset Flow (New Logic)sequenceDiagram
participant User as User
participant Button as Button
participant MultiTimer as MultiTimer
participant HSLink_Pro_expansion as HSLink_Pro_expansion
participant reset_way as reset_way
User->>Button: Single click
Button->>MultiTimer: Event detected (via timer)
MultiTimer->>HSLink_Pro_expansion: button_ticks() callback
HSLink_Pro_expansion->>HSLink_Pro_expansion: Button event handler
alt Reset mode: SRST
HSLink_Pro_expansion->>reset_way: srst_reset()
else Reset mode: Software
HSLink_Pro_expansion->>reset_way: software_reset()
else Reset mode: POR
HSLink_Pro_expansion->>reset_way: por_reset()
end
MultiTimer Integration in Main ApplicationsequenceDiagram
participant main_cpp as main.cpp
participant MultiTimer as MultiTimer
participant BoardTimer as Board Timer (5ms)
main_cpp->>MultiTimer: Install with millis callback
main_cpp->>BoardTimer: Create timer (5ms interval)
BoardTimer->>MultiTimer: multiTimerYield() (every 5ms)
MultiTimer->>MultiTimer: Process timer callbacks (e.g., button_ticks)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
projects/HSLink-Pro/third_party_components/MultiTimer/MultiTimer (1)
1-1: Consider vendoring vs. submodule to reduce operational overhead.Submodules complicate cloning, CI, and downstream forks. If MultiTimer is small and rarely updated, pulling the source directly (or using a package manager) may simplify maintenance.
projects/HSLink-Pro/src/HSLink_Pro_expansion.cpp (1)
433-438: Consider timer interval alignment.The button timer uses a 10ms interval, but the main timer in
main.cppuses 5ms. Consider aligning intervals or documenting the rationale for different intervals to optimize system timing.- multiTimerStart(&timer_btn, 10, true, button_ticks_, NULL); + multiTimerStart(&timer_btn, 5, true, button_ticks_, NULL);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitmodules(1 hunks)projects/HSLink-Pro/src/CMakeLists.txt(1 hunks)projects/HSLink-Pro/src/DAP_config.h(1 hunks)projects/HSLink-Pro/src/HSLink_Pro_expansion.cpp(3 hunks)projects/HSLink-Pro/src/dp_common.c(0 hunks)projects/HSLink-Pro/src/dp_common.h(1 hunks)projects/HSLink-Pro/src/main.cpp(2 hunks)projects/HSLink-Pro/src/reset_way.cpp(1 hunks)projects/HSLink-Pro/src/reset_way.h(1 hunks)projects/HSLink-Pro/src/third_party_components/CMakeLists.txt(1 hunks)projects/HSLink-Pro/third_party_components/MultiTimer/CMakeLists.txt(1 hunks)projects/HSLink-Pro/third_party_components/MultiTimer/MultiTimer(1 hunks)
💤 Files with no reviewable changes (1)
- projects/HSLink-Pro/src/dp_common.c
🔇 Additional comments (13)
projects/HSLink-Pro/third_party_components/MultiTimer/MultiTimer (1)
1-1: Validate submodule metadata (.gitmodules, license, reproducibility).The file only records the pinned commit (
fe77487…) but gives no visibility into:
- Whether
.gitmoduleswas updated with the correct URL/branch.- The upstream licence compatibility with HSLink-Pro.
- Whether we intend to track a tag/release instead of a floating SHA for long-term reproducibility.
Please double-check these points before merge.
projects/HSLink-Pro/src/dp_common.h (1)
10-10: LGTM - Code refactoring aligns with modularization.The removal of reset function declarations from this header is appropriate as they've been moved to the dedicated
reset_way.hmodule. This improves code organization and separation of concerns.projects/HSLink-Pro/src/CMakeLists.txt (1)
95-95: LGTM - Proper integration of new reset module.The addition of
reset_way.cppto the build configuration is necessary and correctly placed among other source files to support the new reset functionality..gitmodules (1)
28-30: Submodule configuration verified
- The GitHub URL returns a 301 redirect (expected for
.gitendpoints), confirming the repo is accessible- The submodule directory is present at
projects/HSLink-Pro/third_party_components/MultiTimer/MultiTimer- Follows existing submodule patterns—no further changes needed
projects/HSLink-Pro/src/third_party_components/CMakeLists.txt (2)
26-27: Clarify the crc32 component addition.The
crc32component addition wasn't mentioned in the PR objectives or AI summary. Please verify if this is intentionally part of this PR or if it should be in a separate change.
28-28: LGTM - MultiTimer integration follows established pattern.The MultiTimer subdirectory addition is correctly configured and aligns with the PR objective to integrate timer-based button handling.
projects/HSLink-Pro/src/DAP_config.h (1)
638-638: LGTM - Proper header inclusion for refactored reset functionality.The inclusion of
reset_way.his correctly placed before thePIN_nRESET_OUTfunction that uses the reset functions (software_reset()andpor_reset()on lines 650 and 653). This supports the code refactoring that moved reset functionality to a dedicated module.projects/HSLink-Pro/third_party_components/MultiTimer/CMakeLists.txt (1)
1-2: LGTM! Clean third-party component integration.The CMake configuration correctly sets up the MultiTimer component with proper include path and source file compilation.
projects/HSLink-Pro/src/main.cpp (2)
14-14: LGTM! Proper header inclusion.The MultiTimer header is correctly included to support the timer functionality.
73-74: Audit MultiTimer callback ISR safetyThe only
multiTimerStartcallbacks found are:
- projects/HSLink-Pro/src/reset_way.cpp
•timer_cblambdas callingPower_Turn(...)andgpio_write_pin(...)- projects/HSLink-Pro/src/HSLink_Pro_expansion.cpp
•button_ticks_lambda callingbutton_ticks()Please confirm that all three routines—
Power_Turn(),gpio_write_pin(), andbutton_ticks()—are safe to invoke in an ISR context (no blocking calls, minimal execution time). If any perform heavy work or OS/API calls, defer that logic outside the ISR (e.g., post a flag or enqueue to a background task).projects/HSLink-Pro/src/reset_way.h (1)
5-22: LGTM! Well-structured header file.The header properly uses include guards, C++ compatibility, and appropriate function declarations. The structure follows good practices for a modular reset interface.
projects/HSLink-Pro/src/HSLink_Pro_expansion.cpp (1)
18-30: LGTM! Proper header inclusions.The new headers correctly support the enhanced reset functionality and timer management.
projects/HSLink-Pro/src/reset_way.cpp (1)
22-31: LGTM! Correct software reset implementation.The function properly checks debug port type and implements ARM Cortex-M system reset via AIRCR register with appropriate error handling.
| if (SETTING_GET_RESET_MODE(HSLink_Setting.reset, RESET_NRST)) { | ||
| srst_reset(); | ||
| } | ||
| if (SETTING_GET_RESET_MODE(HSLink_Setting.reset, RESET_ARM_SWD_SOFT)) { | ||
| software_reset(); | ||
| } | ||
| if (SETTING_GET_RESET_MODE(HSLink_Setting.reset, RESET_POR)) { | ||
| por_reset(); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify reset function ISR safety.
The button callback now calls reset functions (srst_reset(), software_reset(), por_reset()) which will execute in ISR context due to the timer-driven approach. Ensure these functions are ISR-safe and don't perform blocking operations.
🏁 Script executed:
#!/bin/bash
# Description: Analyze reset functions for ISR safety
# Expected: Functions should be lightweight and avoid blocking operations
echo "=== Checking reset function implementations ==="
ast-grep --pattern 'uint8_t software_reset(void) {
$$$
}'
ast-grep --pattern 'void por_reset(void) {
$$$
}'
ast-grep --pattern 'void srst_reset(void) {
$$$
}'Length of output: 275
🏁 Script executed:
#!/bin/bash
echo "=== Locating reset function implementations ==="
rg -n "software_reset" -C5
rg -n "por_reset" -C5
rg -n "srst_reset" -C5Length of output: 9914
Heavy reset calls in ISR: software_reset() and por_reset() are not ISR-safe
software_reset() (reset_way.cpp:22–31) invokes swd_read_word() and swd_write_word(), which perform bit-banged SWD transfers with inherent delays.
por_reset() (reset_way.cpp:33–43) calls Power_Turn() and multiTimerStart(), scheduling timers and manipulating power rails.
These operations can block or re-enter timer APIs and must not run in interrupt context.
• srst_reset() (reset_way.cpp:45–50) merely toggles a GPIO and is ISR-safe.
• Defer software_reset() and por_reset() to a task or the main loop (e.g., post an event or set a flag) so they execute outside the ISR.
🤖 Prompt for AI Agents
In projects/HSLink-Pro/src/HSLink_Pro_expansion.cpp around lines 411 to 419,
software_reset() and por_reset() are called directly in an ISR, but these
functions are not ISR-safe due to blocking operations and timer usage. To fix
this, remove direct calls to software_reset() and por_reset() from the ISR and
instead set flags or post events indicating these resets are needed. Then,
handle these flags or events in the main loop or a dedicated task to perform the
resets safely outside the interrupt context.
| void por_reset(void) { | ||
| if ((!VREF_ENABLE && HSLink_Setting.power.power_on) | ||
| || VREF_ENABLE) { | ||
| Power_Turn(false); | ||
| static MultiTimer timer_; | ||
| static auto timer_cb = [](MultiTimer *timer, void *user_data) { | ||
| Power_Turn(true); | ||
| }; | ||
| multiTimerStart(&timer_, 10, false, timer_cb, NULL); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix potential concurrency issue with static timer.
The static MultiTimer timer_ could cause issues if por_reset() is called multiple times before the timer expires. Each call would overwrite the previous timer state.
void por_reset(void) {
if ((!VREF_ENABLE && HSLink_Setting.power.power_on)
|| VREF_ENABLE) {
Power_Turn(false);
- static MultiTimer timer_;
- static auto timer_cb = [](MultiTimer *timer, void *user_data) {
+ static MultiTimer timer_por;
+ static bool timer_active = false;
+ if (timer_active) return; // Prevent overlapping calls
+ timer_active = true;
+ static auto timer_cb = [](MultiTimer *timer, void *user_data) {
Power_Turn(true);
+ timer_active = false;
};
- multiTimerStart(&timer_, 10, false, timer_cb, NULL);
+ multiTimerStart(&timer_por, 10, false, timer_cb, NULL);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void por_reset(void) { | |
| if ((!VREF_ENABLE && HSLink_Setting.power.power_on) | |
| || VREF_ENABLE) { | |
| Power_Turn(false); | |
| static MultiTimer timer_; | |
| static auto timer_cb = [](MultiTimer *timer, void *user_data) { | |
| Power_Turn(true); | |
| }; | |
| multiTimerStart(&timer_, 10, false, timer_cb, NULL); | |
| } | |
| } | |
| void por_reset(void) { | |
| if ((!VREF_ENABLE && HSLink_Setting.power.power_on) | |
| || VREF_ENABLE) { | |
| Power_Turn(false); | |
| static MultiTimer timer_por; | |
| static bool timer_active = false; | |
| if (timer_active) | |
| return; // Prevent overlapping calls | |
| timer_active = true; | |
| static auto timer_cb = [](MultiTimer *timer, void *user_data) { | |
| Power_Turn(true); | |
| timer_active = false; | |
| }; | |
| multiTimerStart(&timer_por, 10, false, timer_cb, NULL); | |
| } | |
| } |
🤖 Prompt for AI Agents
In projects/HSLink-Pro/src/reset_way.cpp around lines 33 to 43, the static
MultiTimer timer_ inside por_reset() can cause concurrency issues if por_reset()
is called multiple times before the timer expires, as it reuses the same timer
instance. To fix this, avoid using a static timer variable; instead, allocate a
new MultiTimer instance for each call or manage timer instances to ensure they
do not overwrite each other, preventing state conflicts.
| void srst_reset(void) { | ||
| gpio_write_pin( | ||
| HPM_FGPIO, | ||
| GPIO_GET_PORT_INDEX(PIN_SRST), | ||
| GPIO_GET_PIN_INDEX(PIN_SRST), | ||
| !HSLink_Global.reset_level | ||
| ); | ||
| static MultiTimer timer_; | ||
| static auto timer_cb = [](MultiTimer *timer, void *user_data) { | ||
| gpio_write_pin( | ||
| HPM_FGPIO, | ||
| GPIO_GET_PORT_INDEX(PIN_SRST), | ||
| GPIO_GET_PIN_INDEX(PIN_SRST), | ||
| HSLink_Global.reset_level | ||
| ); | ||
| }; | ||
| multiTimerStart(&timer_, 50, false, timer_cb, NULL); | ||
| } |
There was a problem hiding this comment.
Fix potential concurrency issue with static timer.
Similar to por_reset(), the static MultiTimer timer_ could cause issues if srst_reset() is called multiple times. Also, using the same variable name timer_ in both functions could be confusing.
void srst_reset(void) {
gpio_write_pin(
HPM_FGPIO,
GPIO_GET_PORT_INDEX(PIN_SRST),
GPIO_GET_PIN_INDEX(PIN_SRST),
!HSLink_Global.reset_level
);
- static MultiTimer timer_;
- static auto timer_cb = [](MultiTimer *timer, void *user_data) {
+ static MultiTimer timer_srst;
+ static bool timer_active = false;
+ if (timer_active) return; // Prevent overlapping calls
+ timer_active = true;
+ static auto timer_cb = [](MultiTimer *timer, void *user_data) {
gpio_write_pin(
HPM_FGPIO,
GPIO_GET_PORT_INDEX(PIN_SRST),
GPIO_GET_PIN_INDEX(PIN_SRST),
HSLink_Global.reset_level
);
+ timer_active = false;
};
- multiTimerStart(&timer_, 50, false, timer_cb, NULL);
+ multiTimerStart(&timer_srst, 50, false, timer_cb, NULL);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In projects/HSLink-Pro/src/reset_way.cpp between lines 45 and 62, the static
MultiTimer variable timer_ inside srst_reset() can cause concurrency issues if
srst_reset() is called multiple times, and sharing the same variable name with
por_reset() is confusing. To fix this, remove the static keyword from timer_ to
ensure a new timer instance is created on each call, and rename the timer
variable to a unique name like srst_timer to avoid confusion with other
functions.
|
por_reset异步的话会被hsp loop输出控制回来。。 |
|
引入定时器之后感觉事情就变得更复杂了……能不能换成rtt nano作为操作系统呢 |
|
下个大版本咯 |
Summary by CodeRabbit
New Features
Chores