HSLink-Pro: 2.3.2 #30
Conversation
WalkthroughThe changes update the HSLink-Pro project’s build and source configuration. GitHub workflows and submodule paths have been revised, and board support files were reorganized. New bootloader functionality was introduced through added enumeration, a function to set the bootloader reason, and watchdog initialization. LED control functions have been modified, and WS2812 support is now conditionally compiled. Several documentation, configuration (YAML, CMake), and peripheral initialization files (board and pinmux) were added. Legacy subproject commit references and outdated inclusion methods have been removed or updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant Board
participant Watchdog
participant Bootloader
Main->>Board: board_init()
Main->>Watchdog: EWDG_Init()
Main->>Bootloader: Check keep_bootloader flag
alt Bootloader mode active
Bootloader->>LEDs: show_rainbow()/show_fade_on()
Main->>Watchdog: Periodic ewdg_refresh()
else Application mode
Main->>Application: Jump to application
end
Poem
✨ Finishing Touches
🪧 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 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: 2
🧹 Nitpick comments (24)
projects/HSLink-Pro/common/hslinkpro/README_zh.md (4)
9-11: Review: Console Information Section
The “console 信息打印” section explains the use of UART0 for console output clearly. Consider adding additional details such as baud rate or configuration parameters if they are available, to further aid users.
13-24: Review: BOOT切换 Instructions
The step-by-step instructions for boot mode switching are well laid out. Verify that the bullet indentation and list formatting display correctly in all markdown renderers.
65-70: Review: I2C引脚 (I2C Pins) Section
The I2C pin documentation is concise. For consistent markdown styling, consider ensuring each table row ends with a trailing pipe if your markdown renderer requires it.
96-105: Review: GPTMR引脚 (GPTMR Pins) Section
The GPTMR section includes a detailed table describing each pin and its corresponding function. For improved consistency, consider adding trailing pipes in the table rows if required by your markdown processor.projects/HSLink-Pro/common/hslinkpro/CMakeLists.txt (1)
5-7: Ensure Proper SDK IntegrationThe commands
sdk_inc(.),sdk_src(pinmux.c), andsdk_src(board.c)correctly add the SDK header and source files to the build process. Please verify that these SDK macros (or functions) are defined in your build system and reference the correct paths relative to the project structure for maintainability and clarity.projects/HSLink-Pro/bootloader/bootuf2/bootuf2_config.h (1)
13-13: Purpose of DEFAULT_REASON macroThis new macro appears to define a default reason string that would be used when no specific reason is provided for bootloader actions. The repetitive nature of the string suggests it's meant to be easily identifiable. This likely supports the new watchdog functionality mentioned in the PR objectives where the bootloader needs to track boot failures.
I recommend adding a brief comment before this definition to explain its purpose and how it's used within the bootloader system.
projects/HSLink-Pro/common/BL_Setting_Common.h (1)
17-18: Good improvement to the bootloader robustness with fail counterThe renaming from
is_updatetokeep_bootloaderimproves readability by better reflecting the field's purpose. The addition of thefail_cntfield with 3 bits supports the new watchdog feature mentioned in the PR objectives, allowing the bootloader to track up to 7 consecutive boot failures (sufficient for the 5-failure threshold mentioned).Consider adding a define for the maximum failure threshold (e.g.,
#define MAX_BOOT_FAILURES 5) to make the code more maintainable and self-documenting.projects/HSLink-Pro/src/third_party_components/CMakeLists.txt (1)
3-5: Properly disabled WS2812 as mentioned in PR objectivesThe commented-out WS2812 configuration aligns with the PR objective to disable this component due to hardware reasons.
Consider adding a brief comment explaining why WS2812 is disabled for future reference (e.g.,
# Disabled due to hardware limitations in HSLink-Pro 2.3.2).projects/HSLink-Pro/bootloader/CMakeLists.txt (1)
19-21: Consider removing commented code instead of commenting it outWhile commenting out WS2812 configuration aligns with the PR objective, consider removing these lines completely if WS2812 support is being permanently disabled for this board. If there's a possibility of re-enabling it in the future, add a comment explaining why it's disabled.
projects/HSLink-Pro/third_party_components/tlsf/CMakeLists.txt (1)
6-6: Document purpose of linker wrapping functionsThe wrapped memory allocation functions are correctly specified, but consider adding a comment explaining why these functions are being wrapped (e.g., for custom memory management, tracking, or debugging).
Also, add a newline at the end of the file to follow best practices.
projects/HSLink-Pro/common/hslinkpro/README_en.md (2)
88-89: Correct grammar in TinyUF2 Pin notes.There are subject-verb agreement issues in these sentences.
- PA9 connect GND， and press reset, board enter DFU mode, then PA9 connect 3.3V, drag app to U disk, will download app and enter app directly if successfully; - PA9 connect 3.3V，and press reset, board enter bootloader mode, if flash has the valid app, will directly enter app； + PA9 connects to GND, and when you press reset, board enters DFU mode, then if PA9 connects to 3.3V, drag app to U disk, it will download app and enter app directly if successful; + PA9 connects to 3.3V, and when you press reset, board enters bootloader mode, if flash has a valid app, it will directly enter app;🧰 Tools
🪛 LanguageTool
[uncategorized] ~88-~88: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...PA9 connect GND， and press reset, board enter DFU mode, then PA9 connect 3.3V, drag a...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~89-~89: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...PA9 connect 3.3V，and press reset, board enter bootloader mode, if flash has the valid...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
91-93: Correct table header language.The table header is in Chinese (功能) while the rest of the document is in English.
-| 功能 | 位置 | +| Function | Position |projects/HSLink-Pro/src/DAP_config.h (3)
49-49: Double-check necessity of new include.The added
#include <hpm_romapi.h>might be unused in this file. If it's essential for certain function calls, ensure they are indeed required; otherwise, consider removing to keep the codebase lean.
658-666: Check LED index usage for connected indicator.Hard-coding index
0forWS2812_MixPixelandWS2812_ReverseMixPixelis valid if only one physical LED is used. Otherwise, verify that restricting the effect to index 0 matches your design expectation. It might limit connected states to a single LED if there are multiple LEDs in the strip.
685-693: Check LED index usage for running indicator.Similar to the connected indicator, ensure that only controlling LED index 0 is the intended behavior for displaying the "running" state. If multiple LEDs exist, consider parameterizing this index for greater flexibility.
projects/HSLink-Pro/bootloader/src/main.c (4)
67-104: Add rainbow LED pattern.The
show_rainbowfunction is functional, but frequent calls may cause performance or flooding of debug logs due toprintf. Consider removing or making the debug prints conditional if high-frequency LED updates degrade performance.
105-115: Fade-on effect logic.Similarly,
show_fade_oncallsprintfin a loop. This is acceptable for debugging, but watch for performance or console noise. The rest of the code logic is straightforward.
191-191: Incrementing fail count.The fail count logic helps track repetitive boot failures. Just confirm you have an upper bound or wrap-around check to avoid integer overflow if the device reboots extremely often.
248-253: Rainbow effect in bootloader loop + watchdog refresh.Continuously refreshing the watchdog and updating the LED effect is fine. Just watch out for overhead if
show_rainbow()is called too often. Performance seems acceptable for a bootloader environment.projects/HSLink-Pro/common/hslinkpro/pinmux.h (1)
14-29: Add descriptive documentation for each pin initialization function.
These functions handle critical hardware pin setups, but currently lack doc comments or usage notes. Adding concise docstrings clarifying their purpose, expected parameters, and side effects improves maintainability and makes the codebase more approachable for future contributors.projects/HSLink-Pro/common/hslinkpro/pinmux.c (2)
29-40: Provide a default or error-handling branch for unrecognized peripheral pointers.
Currently, each pin initialization function supports a specific subset of peripherals (e.g.,HPM_UART0,HPM_UART3) and silently ignores other pointer values with an emptyelse. For long-term reliability, consider either asserting, logging a warning, or handling additional pointers to avoid silent no-ops when an unsupported pointer is passed.Example approach (showing one function; apply similarly to others):
else { - ; + // Log or assert for unexpected pointer + // For instance: + // HPM_ASSERT(false && "Unsupported UART pointer provided"); }Also applies to: 56-69, 82-96, 98-112, 115-126, 196-223
196-223: Use a descriptive enumeration instead of a boolean parameter.
Thebool as_compparameter ininit_gptmr_channel_pinis functional but might be more expressive as an enum (e.g.,GPTMR_PIN_MODE_CAPTvs.GPTMR_PIN_MODE_COMP). This makes the code more self-documenting and reduces the risk of confusion over boolean usage.projects/HSLink-Pro/common/hslinkpro/board.h (1)
24-223: Consider grouping related macro definitions and adding inline documentation.
This file houses numerous defines for peripheral addresses, IRQ lines, clock names, etc. Using short doc comments or grouping them by functionality (e.g., “// UART Defines”, “// SPI Defines”) could enhance readability and reduce confusion.projects/HSLink-Pro/common/hslinkpro/board.c (1)
417-433: Avoid indefinite blocking when I2C lines are stuck.
If the system detects a low SCL or SDA line, it enters a spin loop (while (1)). Instead, consider returning an error code or implementing a backoff and retry mechanism, so higher-level logic can handle bus failures gracefully.Example diff to return an error code:
- printf("CLK is low, please power cycle the board\n"); - while (1) { - } + printf("CLK is low. Returning error.\n"); + // Return an error code or boolean + return BOARD_I2C_ERR_LINE_STUCK;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
projects/HSLink-Pro/common/hslinkpro/doc/hpm5301evklite.pngis excluded by!**/*.png
📒 Files selected for processing (29)
.github/workflows/HSLinkPro-build.yml(2 hunks).gitmodules(1 hunks)projects/HSLink-Pro/WS2812(0 hunks)projects/HSLink-Pro/bootloader/CMakeLists.txt(2 hunks)projects/HSLink-Pro/bootloader/bootuf2/bootuf2.c(3 hunks)projects/HSLink-Pro/bootloader/bootuf2/bootuf2.h(1 hunks)projects/HSLink-Pro/bootloader/bootuf2/bootuf2_config.h(1 hunks)projects/HSLink-Pro/bootloader/src/main.c(3 hunks)projects/HSLink-Pro/common/BL_Setting_Common.h(1 hunks)projects/HSLink-Pro/common/hslinkpro/CMakeLists.txt(1 hunks)projects/HSLink-Pro/common/hslinkpro/README_en.md(1 hunks)projects/HSLink-Pro/common/hslinkpro/README_zh.md(1 hunks)projects/HSLink-Pro/common/hslinkpro/board.c(1 hunks)projects/HSLink-Pro/common/hslinkpro/board.h(1 hunks)projects/HSLink-Pro/common/hslinkpro/hslinkpro.yaml(1 hunks)projects/HSLink-Pro/common/hslinkpro/pinmux.c(1 hunks)projects/HSLink-Pro/common/hslinkpro/pinmux.h(1 hunks)projects/HSLink-Pro/src/CMakeLists.txt(2 hunks)projects/HSLink-Pro/src/DAP_config.h(3 hunks)projects/HSLink-Pro/src/HSLink_Pro_expansion.c(3 hunks)projects/HSLink-Pro/src/main.cpp(3 hunks)projects/HSLink-Pro/src/rapidjson(0 hunks)projects/HSLink-Pro/src/setting.c(1 hunks)projects/HSLink-Pro/src/third_party_components/CMakeLists.txt(1 hunks)projects/HSLink-Pro/src/third_party_components/port/tlsf/malloc.c(1 hunks)projects/HSLink-Pro/third_party_components/WS2812(1 hunks)projects/HSLink-Pro/third_party_components/rapidjson/CMakeLists.txt(1 hunks)projects/HSLink-Pro/third_party_components/rapidjson/rapidjson(1 hunks)projects/HSLink-Pro/third_party_components/tlsf/CMakeLists.txt(1 hunks)
💤 Files with no reviewable changes (2)
- projects/HSLink-Pro/src/rapidjson
- projects/HSLink-Pro/WS2812
🧰 Additional context used
🪛 LanguageTool
projects/HSLink-Pro/common/hslinkpro/README_en.md
[uncategorized] ~88-~88: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...PA9 connect GND， and press reset, board enter DFU mode, then PA9 connect 3.3V, drag a...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~89-~89: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...PA9 connect 3.3V，and press reset, board enter bootloader mode, if flash has the valid...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
🪛 Cppcheck (2.10-2)
projects/HSLink-Pro/common/hslinkpro/board.c
[error] 244-244: There is an unknown macro here somewhere. Configuration is required. If SDK_DECLARE_EXT_ISR_M is a macro then please configure it.
(unknownMacro)
[error] 78-78: There is an unknown macro here somewhere. Configuration is required. If ATTR_PLACE_AT is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (61)
projects/HSLink-Pro/common/hslinkpro/README_zh.md (11)
1-2: Review: Title and File Header
The title clearly identifies the board with “# HPM5301EVKLITE开发板”. It sets the context properly for a Chinese documentation file.
3-5: Review: Overview Section ("概述")
The overview succinctly introduces the board and its key features. The description is clear and informative.
7-7: Review: Image Inclusion
The image is embedded with an appropriate caption. Please verify that the relative image path (doc/hpm5301evklite.png) is correct and that the image renders as expected in various markdown viewers.
25-30: Review: Key Function Table
The table describing the key states (OFF/ON) and their functions is concise and useful. If available, you might consider adding any extra contextual notes regarding these modes.
31-39: Review: 按键 (Buttons) Section
The section includes an anchor label and a table listing the buttons and their functions. The formatting is clear, and the use of an anchor (e.g.,(lab_hpm5301evklite_board_buttons)=) can help in cross-referencing if used elsewhere.
40-55: Review: 引脚描述 (Pin Description - UART)
The documentation for UART pins provides necessary details along with a table for quick reference. The information is presented clearly and is easy to follow.
56-64: Review: SPI引脚 (SPI Pins) Section
The SPI pin table is formatted consistently and details the pin positions adequately. It may be beneficial to double-check that the ordering aligns perfectly with the hardware reference.
71-78: Review: ACMP引脚 (ACMP Pins) Section
The ACMP pin table is clear and communicates the necessary information effectively without any issues.
79-85: Review: ADC16引脚 (ADC16 Pins) Section
The ADC16 pin table is straightforward and provides the required details. No changes are needed here.
86-95: Review: TinyUF2引脚 (TinyUF2 Pins) Section
This section effectively explains the board’s DFU and bootloader modes, including clear step-by-step instructions. The additional notes on wiring and mode activation are helpful.
106-111: Review: SPI模拟I2S CS引脚 (SPI-simulated I2S CS Pin) Section
The final section clearly documents the SPI模拟I2S CS pin with a well-formatted table. It is clear and concise, and no changes are necessary.projects/HSLink-Pro/third_party_components/rapidjson/rapidjson (1)
1-1: Submodule Commit Update RecordedThe new subproject commit reference for RapidJSON is correctly recorded. This aligns with the recent restructure of third-party components. Please ensure that the commit
24b5e7a8b27f42fa16b96fc70aade9106cf7102fis verified as the intended update and that any associated documentation or configuration references have been updated accordingly.projects/HSLink-Pro/common/hslinkpro/CMakeLists.txt (1)
1-2: Licensing Information is CorrectThe licensing details on lines 1-2 are clearly stated and comply with the BSD-3-Clause requirements.
projects/HSLink-Pro/third_party_components/WS2812 (1)
1-1: Submodule Commit Update VerifiedThe file now records the submodule commit "9554022c88752af33942f357d492d16be7d9877e". Please ensure that this update reflects the intended revision of the WS2812 component and that it aligns with the updated submodule path in the repository.
projects/HSLink-Pro/common/hslinkpro/hslinkpro.yaml (1)
1-14: LGTM: Custom board configuration meets PR objectivesThis new YAML configuration file properly defines the custom board specifications for the HSLink-Pro, which aligns with the PR objective "the use of a custom board in the compiler options." The configuration correctly specifies the SoC, device identifier, OpenOCD settings, and flash memory characteristics.
The file structure follows standard YAML format with appropriate indentation and organization of board properties.
projects/HSLink-Pro/src/third_party_components/port/tlsf/malloc.c (1)
3-3:Details
❓ Verification inconclusive
Include path updated for relocated TLSF module
The include path has been simplified from a nested path to a direct reference, which aligns with the PR objective of "moving some submodules to different locations." This change suggests that the TLSF header file is now expected to be in the same directory or in a standard include path.
Verify that the build system correctly identifies the location of
tlsf.hafter this reorganization to prevent potential compilation errors.
🏁 Script executed:
#!/bin/bash # Check if tlsf.h is accessible from the expected location fd -t f "tlsf.h" --exec echo "Found tlsf.h at: {}" # Examine the include paths used during compilation rg -A 3 "tlsf" --glob "CMakeLists.txt" projects/HSLink-Pro/Length of output: 1004
TLSF Include Path Update – Verify Build Setup
The PR now simplifies the include in
malloc.cto:#include "tlsf.h"This aligns with moving the TLSF submodule, and the CMake configuration (notably in
projects/HSLink-Pro/src/third_party_components/CMakeLists.txtand the TLSF module’s own CMakeLists.txt) shows that the TLSF directory is properly added (withsdk_app_inc(tlsf)).However, the file search for
tlsf.hdid not return any results. Please double-check that thetlsf.hfile exists in the expected include path (likely within the TLSF submodule directory) so the build system can locate it and avoid compilation issues.projects/HSLink-Pro/third_party_components/rapidjson/CMakeLists.txt (1)
1-2: RapidJSON configuration after module relocationThis new CMake configuration properly sets up RapidJSON after its relocation, which aligns with the PR objective of "moving some submodules to different locations." The configuration:
- Sets a reasonable default chunk capacity (8KB) for the RapidJSON allocator
- Correctly references the include directory for RapidJSON headers
These settings ensure that applications using RapidJSON will compile correctly with appropriate memory optimization.
.github/workflows/HSLinkPro-build.yml (2)
52-52: Target board configuration updatedThe build configuration has been updated to use the custom board
hslinkproinstead of the previoushpm5301evklite. This change aligns with the PR objective of using a custom board in the compiler options.
63-63: Target board configuration updated consistentlyThe
flash_uf2build type has also been updated to use the customhslinkproboard, maintaining consistency with theflash_xipbuild type.projects/HSLink-Pro/src/setting.c (1)
33-34: Improved code documentationThe added comments help clarify the size calculations for the EEPROM management area, which improves code readability and maintainability.
projects/HSLink-Pro/bootloader/bootuf2/bootuf2.h (1)
105-105:Details
✅ Verification successful
Changed immutable Content pointer to mutable
The
Contentmember in thebootuf2_FILEstructure has been changed fromconst void *consttovoid *, removing both const qualifiers. This change allows for modifying the content data, which is likely necessary for the new bootloader watchdog functionality.Verify that this change doesn't introduce any unintended side effects by checking where this structure is used:
🏁 Script executed:
#!/bin/bash # Find where bootuf2_FILE structure and its Content member are accessed rg -A 5 -B 5 "bootuf2_FILE" --glob "*.{c,cpp,h,hpp}" rg -A 3 -B 3 "Content" --glob "projects/HSLink-Pro/bootloader/**/*.{c,cpp}"Length of output: 7160
Action: Verification Complete – Mutable Pointer Change Approved
The change from a
const void *constto a mutablevoid *for theContentmember in thebootuf2_FILEstructure has been verified. The searches in the codebase confirmed that:
- The
Contentmember is accessed in a manner consistent with a mutable pointer (e.g., assignments and arithmetic operations in bootuf2.c).- No unintended side effects were detected from this change, and the modified usage aligns with the new bootloader watchdog functionality requirements.
Overall, the change is appropriate and safe.
projects/HSLink-Pro/src/main.cpp (5)
6-6: Added external watchdog driver headerThe inclusion of the external watchdog driver header supports the new watchdog functionality described in the PR objectives.
45-66: Implemented watchdog initializationThe new
EWDG_Init()function properly initializes the external watchdog with a 5-second timeout, which aligns with the PR objective of adding a watchdog feature to ensure system reliability.The implementation includes:
- Clock configuration for the watchdog
- Timeout reset configuration
- Error handling for initialization failures
This is a good safety mechanism to prevent system hangs.
72-72: Added watchdog initialization to startup sequencePlacing the watchdog initialization after
board_init()ensures the system will be monitored for hangs early in the boot process.
89-90: Reset bootloader failure counterThis line resets the bootloader failure counter upon successful boot, which works in conjunction with the bootloader watchdog feature mentioned in the PR objectives. If the system fails to boot properly multiple times, the bootloader will remain active.
92-92: Added watchdog refresh in main loopThe watchdog is refreshed in the main loop, which prevents the system from resetting during normal operation. This is essential for the watchdog to function correctly - it will only reset the system if the main loop stops executing.
projects/HSLink-Pro/src/third_party_components/CMakeLists.txt (2)
1-2: LGTM: Path configuration looks goodThe path configuration for third-party components is correctly set up relative to the current source directory.
7-10: LGTM: TLSF and RapidJSON components correctly configuredThe configuration for the TLSF memory allocator and RapidJSON is properly set up, with appropriate binary output directories.
projects/HSLink-Pro/bootloader/CMakeLists.txt (1)
4-4: LGTM: Board search path addedThe addition of
BOARD_SEARCH_PATHaligns with the PR objective of using a custom board in compiler options.projects/HSLink-Pro/third_party_components/tlsf/CMakeLists.txt (1)
1-4: LGTM: TLSF configuration looks goodThe TLSF memory allocator is properly configured with debug disabled and the necessary includes and source files.
.gitmodules (3)
7-8: Reorganized WS2812 submodule path.The path for the WS2812 submodule has been changed to place it under a new
third_party_componentsdirectory, which provides better organization for external dependencies.
10-11: Reorganized tlsf submodule path.Moved the tlsf submodule under the new
third_party_componentsdirectory structure, improving project organization.
13-15: Reorganized rapidjson submodule path.Relocated the rapidjson submodule to the
third_party_componentsdirectory, completing the reorganization of all external components.projects/HSLink-Pro/src/CMakeLists.txt (2)
11-11: Added custom board search path.Set a board search path to the common directory, enabling the build system to find board configuration files. This supports the use of a custom board in compiler options as mentioned in the PR objectives.
128-128: Refactored third-party components inclusion.Instead of including individual third-party components directly, now including them through a central
third_party_componentsdirectory. This change aligns with the restructuring of submodules in the.gitmodulesfile.projects/HSLink-Pro/common/hslinkpro/README_en.md (1)
1-6: Good documentation for the HPM5301EVKLITE board.The README provides comprehensive information about the board's features and capabilities, which is valuable for users of this custom board.
projects/HSLink-Pro/src/HSLink_Pro_expansion.c (4)
21-23: Made WS2812 inclusion configurable.WS2812 header is now conditionally included, allowing it to be disabled through configuration. This supports the PR objective of disabling the WS2812 component due to hardware reasons.
299-301: Made WS2812 initialization configurable.WS2812 initialization is now conditional, ensuring it's only called when the component is enabled through configuration.
327-329: Made WS2812 update configurable and updated function call.Updated WS2812_Update call to include a parameter (true) and made it conditional, aligning with the configurable approach to WS2812 support.
336-336: Updated bootloader setting variable name.Changed from
is_updatetokeep_bootloader, which appears to be part of the watchdog feature implementation mentioned in the PR objectives, ensuring the bootloader remains active if boot process fails.projects/HSLink-Pro/src/DAP_config.h (1)
52-55: Conditional WS2812 inclusion.Guarding WS2812 logic under
#if defined(CONFIG_WS2812) && CONFIG_WS2812 == 1is a sound approach for optional LED support. Just confirm that the build configuration properly defines and setsCONFIG_WS2812to avoid accidental compilation or missing references.projects/HSLink-Pro/bootloader/bootuf2/bootuf2.c (4)
10-10: Bootloader info string updated.Appending
"Bootloader Reason: "andDEFAULT_REASONhelps clarify the reason for entering the bootloader. Make sure the combined length fits in flash comfortably; no major concerns detected.
41-46: Improved readability with enumerated file indices.Defining enum constants for file array indices (
FILE_ID,FILE_INFO, etc.) is a good practice, improving readability and maintainability of the code.
49-52: Structures for file content initialization.Defining each file with
.Content = NULLuntil set later is a valid approach. Confirm that your code always assigns content before usage if a file is required. Otherwise, zero file size and null content will hide it from the filesystem.
294-297: Ensure default reason is assigned only once if needed.Setting the default reason is good for handling an uninitialized or null scenario. Verify that other code paths don’t inadvertently reassign it. The logic here is fine for typical usage.
projects/HSLink-Pro/bootloader/src/main.c (15)
6-6: Include for watchdog functionality.Including
<hpm_ewdg_drv.h>is appropriate to use the external watchdog. No concerns here.
10-10: Include for PPOR driver usage.Including
<hpm_ppor_drv.h>is necessary for software reset operations. Looks good.
13-15: Optional WS2812 LED support.Conditionally including
<WS2812.h>aligns with a feature flag approach. This is consistent with the rest of the code.
120-120: Turning off LED with blocking update.Ensuring the LED is updated with
WS2812_Update(true)and then waiting forWS2812_IsBusy()to finish works, but confirm that the blocking approach is acceptable in your real-time flow.
145-167: External watchdog initialization.Initializing the EWDG with a 5-second timeout is a sensible default. For production, confirm that this interval suits your system’s reset strategy. Also consider checking error codes more thoroughly if
ewdg_initfails.
168-171: Adding extern references.Exposing
file_INFO,msc_bootuf2_init, andbootuf2_SetReasonis straightforward. Double-check that they are declared consistently with their definitions to avoid linkage issues.
175-175: Watchdog initialization in main.Calling
EWDG_Init()early is standard for ensuring immediate coverage by the watchdog. Consider ensuring you refresh it promptly if boot steps run long.
181-184: Conditional LED usage in main.Initializing and turning off the LED only if
CONFIG_WS2812is enabled. This pattern is consistent with your conditional LED approach, no further concerns.
197-201: Handle keep_bootloader flag.Setting the reason to
"APP_REQ_KEEP_BL"and jumping to bootloader entry is a clear approach for an app-requested stay in bootloader. Looks good.
203-212: 2s delay check for button press.This ensures the user can force the bootloader. The approach is fine. Just confirm that a 2-second wait is acceptable for your power or performance constraints.
214-220: Detect invalid app and remain in bootloader.If the app signature is invalid, setting
"APP_INVALID"is a clear diagnostic measure. The logic is correct.
222-230: Fail count over threshold.Clearing
fail_cntwhen the threshold is exceeded resets the attempt cycle and logs the reason. This logic is straightforward. No issues detected.
231-231: Proceed to application.The code transitions to
__entry_app. This jump path is consistent with the rest of the logic in your boot sequence.
244-246: Clearing keep_bootloader before resetting.Ensuring that the next boot no longer requests the bootloader is sensible. Also, the PPOR-based software reset is a valid approach for hardware-level restarts.
257-264: Gracefully exiting into the application.Turning off the LED and jumping to the app, then halting if anything goes wrong, is a standard approach. No issues detected.
projects/HSLink-Pro/common/hslinkpro/board.c (1)
77-79: Configure your static analysis tool to recognize custom macros.
Macros likeATTR_PLACE_AT(line ~78) andSDK_DECLARE_EXT_ISR_M(line ~244) trigger "unknown macro" warnings from some analyzers. They are not indicative of a functional error but might require explicit configuration in your static analysis settings.Also applies to: 244-245
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 78-78: There is an unknown macro here somewhere. Configuration is required. If ATTR_PLACE_AT is a macro then please configure it.
(unknownMacro)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
projects/HSLink-Pro/bootloader/bootuf2/bootuf2.c(3 hunks)
🔇 Additional comments (4)
projects/HSLink-Pro/bootloader/bootuf2/bootuf2.c (4)
10-14: File information updated to include bootloader reason.The change makes
file_INFOconstant and adds a bootloader reason placeholder that can be replaced at runtime. This is a good improvement for diagnostics.
41-46: Improved code readability with file index enumeration.Replacing hardcoded numeric indices with named constants through an enumeration is an excellent practice. This enhances code maintainability and reduces the risk of errors when referencing array indices.
49-52: Good use of enums in array initialization.The files array initialization now uses the enum values instead of hardcoded indices, with proper comments indicating that the INFO file will be populated during initialization.
320-322: Good initialization practice with conditional check.The added check ensures that
bootuf2_SetReasonis only called when needed and prevents potential memory issues by avoiding redundant allocations.
| void bootuf2_SetReason(const char* reason) { | ||
| if (files[FILE_INFO].Content != NULL) { | ||
| free(files[FILE_INFO].Content); | ||
| } | ||
|
|
||
| // modify it to show why we keep in bootloader mode | ||
| // because of file_INFO is a string in .data section, we need to copy it and modify it | ||
| char* const file_INFO_ = (char*)malloc(strlen(file_INFO) + 100); | ||
| memset(file_INFO_, 0, strlen(file_INFO) + 100); | ||
| strcpy(file_INFO_, file_INFO); | ||
| replaceSubstring(file_INFO_, DEFAULT_REASON, reason); | ||
| files[FILE_INFO].Content = file_INFO_; | ||
| files[FILE_INFO].FileSize = strlen(file_INFO_); | ||
| } |
There was a problem hiding this comment.
Risk of memory management issues in bootuf2_SetReason.
The function has several potential issues:
- No check if malloc fails
- Fixed buffer size might be inadequate for long reasons
- Memory leak potential if function is called multiple times
Implement this safer version:
void bootuf2_SetReason(const char* reason) {
if (files[FILE_INFO].Content != NULL) {
free(files[FILE_INFO].Content);
}
// modify it to show why we keep in bootloader mode
// because of file_INFO is a string in .data section, we need to copy it and modify it
char* const file_INFO_ = (char*)malloc(strlen(file_INFO) + 100);
+ if (file_INFO_ == NULL) {
+ // Handle allocation failure - at minimum, set Content to NULL
+ files[FILE_INFO].Content = NULL;
+ files[FILE_INFO].FileSize = 0;
+ return;
+ }
memset(file_INFO_, 0, strlen(file_INFO) + 100);
strcpy(file_INFO_, file_INFO);
replaceSubstring(file_INFO_, DEFAULT_REASON, reason);
files[FILE_INFO].Content = file_INFO_;
files[FILE_INFO].FileSize = strlen(file_INFO_);
}📝 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 bootuf2_SetReason(const char* reason) { | |
| if (files[FILE_INFO].Content != NULL) { | |
| free(files[FILE_INFO].Content); | |
| } | |
| // modify it to show why we keep in bootloader mode | |
| // because of file_INFO is a string in .data section, we need to copy it and modify it | |
| char* const file_INFO_ = (char*)malloc(strlen(file_INFO) + 100); | |
| memset(file_INFO_, 0, strlen(file_INFO) + 100); | |
| strcpy(file_INFO_, file_INFO); | |
| replaceSubstring(file_INFO_, DEFAULT_REASON, reason); | |
| files[FILE_INFO].Content = file_INFO_; | |
| files[FILE_INFO].FileSize = strlen(file_INFO_); | |
| } | |
| void bootuf2_SetReason(const char* reason) { | |
| if (files[FILE_INFO].Content != NULL) { | |
| free(files[FILE_INFO].Content); | |
| } | |
| // modify it to show why we keep in bootloader mode | |
| // because of file_INFO is a string in .data section, we need to copy it and modify it | |
| char* const file_INFO_ = (char*)malloc(strlen(file_INFO) + 100); | |
| if (file_INFO_ == NULL) { | |
| // Handle allocation failure - at minimum, set Content to NULL | |
| files[FILE_INFO].Content = NULL; | |
| files[FILE_INFO].FileSize = 0; | |
| return; | |
| } | |
| memset(file_INFO_, 0, strlen(file_INFO) + 100); | |
| strcpy(file_INFO_, file_INFO); | |
| replaceSubstring(file_INFO_, DEFAULT_REASON, reason); | |
| files[FILE_INFO].Content = file_INFO_; | |
| files[FILE_INFO].FileSize = strlen(file_INFO_); | |
| } |
| static void replaceSubstring(char *str, const char *oldWord, const char *newWord) { | ||
| char *result; // 存放新字符串的缓冲区 | ||
| char *pos, *temp; | ||
| int count = 0; | ||
| int oldLen = strlen(oldWord); | ||
| int newLen = strlen(newWord); | ||
|
|
||
| // 1. 统计 oldWord 在 str 中出现的次数 | ||
| temp = str; | ||
| while ((pos = strstr(temp, oldWord)) != NULL) { | ||
| count++; | ||
| temp = pos + oldLen; | ||
| } | ||
|
|
||
| // 2. 分配足够大小的缓冲区（注意：如果 newLen < oldLen，分配空间会小于原始字符串大小， | ||
| // 但为了代码简单，这里直接分配原始串长度加上扩展部分） | ||
| result = (char *) malloc(strlen(str) + count * (newLen - oldLen) + 1); | ||
| if (result == NULL) { | ||
| // 内存分配失败，直接返回 | ||
| return; | ||
| } | ||
|
|
||
| // 3. 进行字符串替换构造新的结果字符串 | ||
| temp = str; // 临时指针，指向原字符串 | ||
| char *r = result; // 指向新字符串的写入位置 | ||
|
|
||
| while ((pos = strstr(temp, oldWord)) != NULL) { | ||
| // 复制 pos 之前的部分 | ||
| int len = pos - temp; | ||
| memcpy(r, temp, len); | ||
| r += len; | ||
|
|
||
| // 将 newWord 复制到结果中 | ||
| memcpy(r, newWord, newLen); | ||
| r += newLen; | ||
|
|
||
| // 更新 temp 指针，跳过被替换的 oldWord | ||
| temp = pos + oldLen; | ||
| } | ||
| // 复制剩下的部分 | ||
| strcpy(r, temp); | ||
|
|
||
| // 4. 将结果拷贝回原来的字符串中 | ||
| strcpy(str, result); | ||
| free(result); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement safer string handling in replaceSubstring.
The current implementation has several issues:
- The function allocates memory with
mallocbut lacks robust error handling - Large string replacements could lead to buffer overflow
- Comments are in Chinese, inconsistent with the rest of the codebase
Consider implementing a more robust version:
static void replaceSubstring(char *str, const char *oldWord, const char *newWord) {
- char *result; // 存放新字符串的缓冲区
+ char *result; // Buffer for the new string
char *pos, *temp;
int count = 0;
int oldLen = strlen(oldWord);
int newLen = strlen(newWord);
- // 1. 统计 oldWord 在 str 中出现的次数
+ // 1. Count occurrences of oldWord in str
temp = str;
while ((pos = strstr(temp, oldWord)) != NULL) {
count++;
temp = pos + oldLen;
}
- // 2. 分配足够大小的缓冲区（注意：如果 newLen < oldLen，分配空间会小于原始字符串大小，
- // 但为了代码简单，这里直接分配原始串长度加上扩展部分）
+ // 2. Allocate buffer with enough space for the resulting string
result = (char *) malloc(strlen(str) + count * (newLen - oldLen) + 1);
if (result == NULL) {
- // 内存分配失败，直接返回
+ // Memory allocation failed
return;
}
- // 3. 进行字符串替换构造新的结果字符串
- temp = str; // 临时指针，指向原字符串
- char *r = result; // 指向新字符串的写入位置
+ // 3. Perform the string replacement
+ temp = str; // Temporary pointer to the original string
+ char *r = result; // Pointer to the write position in the new string
while ((pos = strstr(temp, oldWord)) != NULL) {
- // 复制 pos 之前的部分
+ // Copy the part before oldWord
int len = pos - temp;
memcpy(r, temp, len);
r += len;
- // 将 newWord 复制到结果中
+ // Copy newWord to the result
memcpy(r, newWord, newLen);
r += newLen;
- // 更新 temp 指针，跳过被替换的 oldWord
+ // Update temp pointer to skip the replaced oldWord
temp = pos + oldLen;
}
- // 复制剩下的部分
+ // Copy the remaining part
strcpy(r, temp);
- // 4. 将结果拷贝回原来的字符串中
+ // 4. Copy the result back to the original string
strcpy(str, result);
free(result);
}📝 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.
| static void replaceSubstring(char *str, const char *oldWord, const char *newWord) { | |
| char *result; // 存放新字符串的缓冲区 | |
| char *pos, *temp; | |
| int count = 0; | |
| int oldLen = strlen(oldWord); | |
| int newLen = strlen(newWord); | |
| // 1. 统计 oldWord 在 str 中出现的次数 | |
| temp = str; | |
| while ((pos = strstr(temp, oldWord)) != NULL) { | |
| count++; | |
| temp = pos + oldLen; | |
| } | |
| // 2. 分配足够大小的缓冲区（注意：如果 newLen < oldLen，分配空间会小于原始字符串大小， | |
| // 但为了代码简单，这里直接分配原始串长度加上扩展部分） | |
| result = (char *) malloc(strlen(str) + count * (newLen - oldLen) + 1); | |
| if (result == NULL) { | |
| // 内存分配失败，直接返回 | |
| return; | |
| } | |
| // 3. 进行字符串替换构造新的结果字符串 | |
| temp = str; // 临时指针，指向原字符串 | |
| char *r = result; // 指向新字符串的写入位置 | |
| while ((pos = strstr(temp, oldWord)) != NULL) { | |
| // 复制 pos 之前的部分 | |
| int len = pos - temp; | |
| memcpy(r, temp, len); | |
| r += len; | |
| // 将 newWord 复制到结果中 | |
| memcpy(r, newWord, newLen); | |
| r += newLen; | |
| // 更新 temp 指针，跳过被替换的 oldWord | |
| temp = pos + oldLen; | |
| } | |
| // 复制剩下的部分 | |
| strcpy(r, temp); | |
| // 4. 将结果拷贝回原来的字符串中 | |
| strcpy(str, result); | |
| free(result); | |
| } | |
| static void replaceSubstring(char *str, const char *oldWord, const char *newWord) { | |
| char *result; // Buffer for the new string | |
| char *pos, *temp; | |
| int count = 0; | |
| int oldLen = strlen(oldWord); | |
| int newLen = strlen(newWord); | |
| // 1. Count occurrences of oldWord in str | |
| temp = str; | |
| while ((pos = strstr(temp, oldWord)) != NULL) { | |
| count++; | |
| temp = pos + oldLen; | |
| } | |
| // 2. Allocate buffer with enough space for the resulting string | |
| result = (char *) malloc(strlen(str) + count * (newLen - oldLen) + 1); | |
| if (result == NULL) { | |
| // Memory allocation failed | |
| return; | |
| } | |
| // 3. Perform the string replacement | |
| temp = str; // Temporary pointer to the original string | |
| char *r = result; // Pointer to the write position in the new string | |
| while ((pos = strstr(temp, oldWord)) != NULL) { | |
| // Copy the part before oldWord | |
| int len = pos - temp; | |
| memcpy(r, temp, len); | |
| r += len; | |
| // Copy newWord to the result | |
| memcpy(r, newWord, newLen); | |
| r += newLen; | |
| // Update temp pointer to skip the replaced oldWord | |
| temp = pos + oldLen; | |
| } | |
| // Copy the remaining part | |
| strcpy(r, temp); | |
| // Copy the result back to the original string | |
| strcpy(str, result); | |
| free(result); | |
| } |
2 participants
Summary by CodeRabbit
New Features
Documentation
Refactor / Chore