Merge hid config into dap_main #63
Conversation
Signed-off-by: sakumisu <[email protected]>
Signed-off-by: sakumisu <[email protected]>
Signed-off-by: sakumisu <[email protected]>
WalkthroughThis update introduces a new project for the HPM5301EVK Lite board, providing a CMSIS-DAP debug probe implementation with both SPI and GPIO-based SWD/JTAG support. It adds all core source files, USB and UART bridge logic, and project configuration. The build system, board-specific configuration, and USB stack settings are included, with detailed handling for DMA, interrupts, and USB descriptors. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Board
participant DAP
participant USB
participant UART
User->>Board: Power on / Reset
Board->>DAP: Initialize DAP subsystem
Board->>USB: Initialize USB stack and descriptors
Board->>UART: Pre-initialize UART, configure DMA
loop Main Loop
DAP->>DAP: Handle debug requests
USB->>UART: Forward USB CDC data to UART (DMA)
UART->>USB: Forward UART RX data to USB (CDC)
end
Poem
✨ Finishing Touches
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. 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: 1
🧹 Nitpick comments (2)
projects/HSLink-Pro/src/HID_COMM/hid_comm.cpp (1)
47-64: Consider using debug logging instead of printfThe function correctly handles USB events, but the
printfon line 53 should use the USB logging macros for consistency with the rest of the codebase.- printf("hid config\r\n"); + USB_LOG_DBG("hid config\r\n");dap_main.c (1)
247-247: Consider a more secure default serial number.The default serial number contains predictable patterns. Consider using a more unique default or generating it from device-specific information.
Consider initializing this with device-specific information like MAC address or chip ID during
chry_dap_init()if not already set by the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
DAP/Source/DAP.c(1 hunks)dap_main.c(15 hunks)dap_main.h(3 hunks)projects/HSLink-Pro/src/CMakeLists.txt(2 hunks)projects/HSLink-Pro/src/DAP_config.h(0 hunks)projects/HSLink-Pro/src/HID_COMM/hid_comm.cpp(3 hunks)projects/HSLink-Pro/src/HID_COMM/hid_comm.h(0 hunks)projects/HSLink-Pro/src/JTAG_DP/JTAG_DP_IO.c(1 hunks)projects/HSLink-Pro/src/JTAG_DP/JTAG_DP_SPI.c(0 hunks)projects/HSLink-Pro/src/dp_common.c(2 hunks)projects/HSLink-Pro/src/main.cpp(3 hunks)projects/HSLink-Pro/src/usb_config.h(5 hunks)projects/HSLink-Pro/src/usb_configuration.c(0 hunks)projects/HSLink-Pro/src/usb_configuration.h(0 hunks)
💤 Files with no reviewable changes (5)
- projects/HSLink-Pro/src/JTAG_DP/JTAG_DP_SPI.c
- projects/HSLink-Pro/src/DAP_config.h
- projects/HSLink-Pro/src/usb_configuration.h
- projects/HSLink-Pro/src/usb_configuration.c
- projects/HSLink-Pro/src/HID_COMM/hid_comm.h
🧰 Additional context used
🧬 Code Graph Analysis (2)
DAP/Source/DAP.c (2)
projects/HSLink-Pro/src/dp_common.c (1)
Set_Clock_Delay(123-131)DAP/Include/DAP.h (1)
Set_Clock_Delay(327-327)
projects/HSLink-Pro/src/JTAG_DP/JTAG_DP_IO.c (3)
projects/HSLink-Pro/src/main.cpp (1)
void(29-36)projects/HSLink-Pro/src/DAP_config.h (15)
void(361-368)void(383-434)void(452-456)void(461-465)void(484-489)void(494-499)void(515-524)void(530-537)void(543-551)void(569-578)void(611-619)void(638-656)void(679-685)void(692-699)void(745-748)projects/HSLink-Pro/src/JTAG_DP/JTAG_DP.h (1)
IO_PORT_JTAG_SETUP(8-8)
🔇 Additional comments (18)
projects/HSLink-Pro/src/JTAG_DP/JTAG_DP_IO.c (1)
265-265: LGTM! Function declaration formatting corrected.The indentation fix properly aligns the function declaration with standard C formatting conventions and matches the prototype declaration in
JTAG_DP.hline 8.projects/HSLink-Pro/src/main.cpp (3)
25-26: LGTM! Serial number handling updated for dynamic allocation.The change correctly updates the serial number formatting to use the new
serial_number_dynamicbuffer, aligning with the refactored USB device initialization approach.
75-75: LGTM! USB initialization refactored to use new modular approach.The change from
USB_Configuration()tochry_dap_init(0, HPM_USB0_BASE)aligns with the broader refactoring that moves USB device initialization from the legacyusb_configuration.cto the new modulardap_main.cimplementation.
91-91: LGTM! HID configuration macro updated for consistency.The change from
CONFIG_USE_HID_CONFIGtoCONFIG_CHERRYDAP_USE_CUSTOM_HIDmaintains consistency with the new configuration naming scheme introduced in the refactoring.DAP/Source/DAP.c (1)
61-61: LGTM! Enables platform-specific clock delay implementations.Adding the
__attribute__((weak))attribute allows platform-specific implementations to override this default clock delay function. This aligns with the modular refactoring whereprojects/HSLink-Pro/src/dp_common.cprovides a specialized implementation that selects between SPI and I/O port clock delay settings.projects/HSLink-Pro/src/CMakeLists.txt (3)
35-48: LGTM! Enhanced version handling with component parsing.The improved version logic correctly parses the version string into major, minor, and patch components, providing better build-time version information. The fallback to debug values when no version file exists is a good practice.
75-78: LGTM! Added support for new modular components.The addition of
swd_hostandUSB2UARTsource and include directories supports the modular refactoring approach, properly organizing the new functionality.
99-99: LGTM! Updated HID configuration macro for consistency.The rename from
CONFIG_USE_HID_CONFIGtoCONFIG_CHERRYDAP_USE_CUSTOM_HIDmaintains consistency with the new configuration naming scheme across the codebase.projects/HSLink-Pro/src/dp_common.c (3)
19-21: LGTM! Macro definition supports local clock delay calculations.The
MAX_SWJ_CLOCKmacro definition is consistent with the clock delay calculation pattern and supports the local implementation of clock delay functions.
22-41: LGTM! Standard I/O clock delay implementation.The
IO_Set_Clock_Delayfunction provides the standard clock delay calculation logic, properly handling both fast and slow clock scenarios with appropriate delay calculations.
123-131: LGTM! Intelligent clock delay dispatching based on port mode.The
Set_Clock_Delayfunction provides excellent modular design by conditionally selecting between SPI and I/O clock delay implementations based on the current debug port and port mode settings. This centralizes clock delay logic while supporting different hardware interfaces.projects/HSLink-Pro/src/HID_COMM/hid_comm.cpp (1)
135-135: LGTM! Dynamic serial number integration.The change to use
serial_number_dynamicis consistent with the refactoring to support dynamic serial numbers.dap_main.h (2)
76-91: LGTM! Well-structured callback declarations.The extern declarations properly indicate that these callbacks should be implemented by the user, following a clean separation of concerns.
22-24: HID endpoint definitions verified – no conflicts with other endpoints.The HID_IN_EP (0x88) and HID_OUT_EP (0x09) are distinct from the existing DAP (0x81/0x02), CDC (0x83/0x04/0x85), and MSC (0x86/0x07) endpoints. No overlaps detected—changes approved.
dap_main.c (1)
499-531: LGTM! Well-structured USB initialization.The
chry_dap_initfunction properly:
- Initializes ring buffers
- Registers descriptors
- Conditionally adds interfaces based on configuration
- Sets up endpoints with appropriate callbacks
The modular approach is a significant improvement over the previous monolithic initialization.
projects/HSLink-Pro/src/usb_config.h (3)
23-23: Debug level changed from ERROR to INFO.The default debug level has been raised from
USB_DBG_ERRORtoUSB_DBG_INFO, which will increase log verbosity. Ensure this is intentional and won't flood the console in production builds.Consider making this configurable via build flags or keeping it at ERROR level for release builds.
36-42: LGTM! Proper cache-aware alignment configuration.The alignment configuration correctly uses cache line size when data cache is enabled, falling back to 4-byte alignment otherwise. This ensures proper DMA operation on cache-enabled systems.
302-307: LGTM! Platform-specific address conversion macros.The address conversion macros properly use board and core-specific functions for physical to RAM address translation, which is essential for proper DMA operation in multi-core systems.
| #define HID_DESC() \ | ||
| /************** Descriptor of Custom interface *****************/ \ | ||
| 0x09, /* bLength: Interface Descriptor size */ \ | ||
| USB_DESCRIPTOR_TYPE_INTERFACE, /* bDescriptorType: Interface descriptor type */ \ | ||
| 0X03, /* bInterfaceNumber: Number of Interface */ \ | ||
| 0x00, /* bAlternateSetting: Alternate setting */ \ | ||
| 0x02, /* bNumEndpoints */ \ | ||
| 0x03, /* bInterfaceClass: HID */ \ | ||
| 0x01, /* bInterfaceSubClass : 1=BOOT, 0=no boot */ \ | ||
| 0x00, /* nInterfaceProtocol : 0=none, 1=keyboard, 2=mouse */ \ | ||
| 0, /* iInterface: Index of string descriptor */ /******************** Descriptor of Custom HID ********************/ \ | ||
| 0x09, /* bLength: HID Descriptor size */ \ | ||
| HID_DESCRIPTOR_TYPE_HID, /* bDescriptorType: HID */ \ | ||
| 0x11, /* bcdHID: HID Class Spec release number */ \ | ||
| 0x01, \ | ||
| 0x00, /* bCountryCode: Hardware target country */ \ | ||
| 0x01, /* bNumDescriptors: Number of HID class descriptors to follow */ \ | ||
| 0x22, /* bDescriptorType */ \ | ||
| HID_CUSTOM_REPORT_DESC_SIZE, /* wItemLength: Total length of Report descriptor */ \ | ||
| 0x00, /******************** Descriptor of Custom in endpoint ********************/ \ | ||
| 0x07, /* bLength: Endpoint Descriptor size */ \ | ||
| USB_DESCRIPTOR_TYPE_ENDPOINT, /* bDescriptorType: */ \ | ||
| HID_IN_EP, /* bEndpointAddress: Endpoint Address (IN) */ \ | ||
| 0x03, /* bmAttributes: Interrupt endpoint */ \ | ||
| WBVAL(HID_PACKET_SIZE), /* wMaxPacketSize: 4 Byte max */ \ | ||
| HIDRAW_INTERVAL, /* bInterval: Polling Interval */ /******************** Descriptor of Custom out endpoint ********************/ \ | ||
| 0x07, /* bLength: Endpoint Descriptor size */ \ | ||
| USB_DESCRIPTOR_TYPE_ENDPOINT, /* bDescriptorType: */ \ | ||
| HID_OUT_EP, /* bEndpointAddress: Endpoint Address (IN) */ \ | ||
| 0x03, /* bmAttributes: Interrupt endpoint */ \ | ||
| WBVAL(HID_PACKET_SIZE), /* wMaxPacketSize: 4 Byte max */ \ | ||
| HIDRAW_INTERVAL /* bInterval: Polling Interval */ | ||
| // clang-format on |
There was a problem hiding this comment.
Interface number should be dynamically calculated.
The HID interface number is hardcoded as 0x03 on line 145, but this might not be correct if MSC is disabled. Consider calculating it dynamically based on enabled interfaces.
- 0X03, /* bInterfaceNumber: Number of Interface */ \
+ (1 + 2), /* bInterfaceNumber: Number of Interface (WinUSB + CDC ACM) */ \📝 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.
| #define HID_DESC() \ | |
| /************** Descriptor of Custom interface *****************/ \ | |
| 0x09, /* bLength: Interface Descriptor size */ \ | |
| USB_DESCRIPTOR_TYPE_INTERFACE, /* bDescriptorType: Interface descriptor type */ \ | |
| 0X03, /* bInterfaceNumber: Number of Interface */ \ | |
| 0x00, /* bAlternateSetting: Alternate setting */ \ | |
| 0x02, /* bNumEndpoints */ \ | |
| 0x03, /* bInterfaceClass: HID */ \ | |
| 0x01, /* bInterfaceSubClass : 1=BOOT, 0=no boot */ \ | |
| 0x00, /* nInterfaceProtocol : 0=none, 1=keyboard, 2=mouse */ \ | |
| 0, /* iInterface: Index of string descriptor */ /******************** Descriptor of Custom HID ********************/ \ | |
| 0x09, /* bLength: HID Descriptor size */ \ | |
| HID_DESCRIPTOR_TYPE_HID, /* bDescriptorType: HID */ \ | |
| 0x11, /* bcdHID: HID Class Spec release number */ \ | |
| 0x01, \ | |
| 0x00, /* bCountryCode: Hardware target country */ \ | |
| 0x01, /* bNumDescriptors: Number of HID class descriptors to follow */ \ | |
| 0x22, /* bDescriptorType */ \ | |
| HID_CUSTOM_REPORT_DESC_SIZE, /* wItemLength: Total length of Report descriptor */ \ | |
| 0x00, /******************** Descriptor of Custom in endpoint ********************/ \ | |
| 0x07, /* bLength: Endpoint Descriptor size */ \ | |
| USB_DESCRIPTOR_TYPE_ENDPOINT, /* bDescriptorType: */ \ | |
| HID_IN_EP, /* bEndpointAddress: Endpoint Address (IN) */ \ | |
| 0x03, /* bmAttributes: Interrupt endpoint */ \ | |
| WBVAL(HID_PACKET_SIZE), /* wMaxPacketSize: 4 Byte max */ \ | |
| HIDRAW_INTERVAL, /* bInterval: Polling Interval */ /******************** Descriptor of Custom out endpoint ********************/ \ | |
| 0x07, /* bLength: Endpoint Descriptor size */ \ | |
| USB_DESCRIPTOR_TYPE_ENDPOINT, /* bDescriptorType: */ \ | |
| HID_OUT_EP, /* bEndpointAddress: Endpoint Address (IN) */ \ | |
| 0x03, /* bmAttributes: Interrupt endpoint */ \ | |
| WBVAL(HID_PACKET_SIZE), /* wMaxPacketSize: 4 Byte max */ \ | |
| HIDRAW_INTERVAL /* bInterval: Polling Interval */ | |
| // clang-format on | |
| #define HID_DESC() \ | |
| /************** Descriptor of Custom interface *****************/ \ | |
| 0x09, /* bLength: Interface Descriptor size */ \ | |
| USB_DESCRIPTOR_TYPE_INTERFACE, /* bDescriptorType: Interface descriptor type */ \ | |
| - 0X03, /* bInterfaceNumber: Number of Interface */ \ | |
| + (1 + 2), /* bInterfaceNumber: Number of Interface (WinUSB + CDC ACM) */ \ | |
| 0x00, /* bAlternateSetting: Alternate setting */ \ | |
| 0x02, /* bNumEndpoints */ \ | |
| 0x03, /* bInterfaceClass: HID */ \ | |
| 0x01, /* bInterfaceSubClass : 1=BOOT, 0=no boot */ \ | |
| 0x00, /* nInterfaceProtocol : 0=none, 1=keyboard, 2=mouse */ \ | |
| 0, /* iInterface: Index of string descriptor */ /******************** Descriptor of Custom HID ********************/ \ | |
| 0x09, /* bLength: HID Descriptor size */ \ | |
| HID_DESCRIPTOR_TYPE_HID, /* bDescriptorType: HID */ \ | |
| 0x11, /* bcdHID: HID Class Spec release number */ \ | |
| 0x01, \ | |
| 0x00, /* bCountryCode: Hardware target country */ \ | |
| 0x01, /* bNumDescriptors: Number of HID class descriptors to follow */ \ | |
| 0x22, /* bDescriptorType */ \ | |
| HID_CUSTOM_REPORT_DESC_SIZE, /* wItemLength: Total length of Report descriptor */ \ | |
| 0x00, /******************** Descriptor of Custom in endpoint ********************/ \ | |
| 0x07, /* bLength: Endpoint Descriptor size */ \ | |
| USB_DESCRIPTOR_TYPE_ENDPOINT, /* bDescriptorType: */ \ | |
| HID_IN_EP, /* bEndpointAddress: Endpoint Address (IN) */ \ | |
| 0x03, /* bmAttributes: Interrupt endpoint */ \ | |
| WBVAL(HID_PACKET_SIZE), /* wMaxPacketSize: 4 Byte max */ \ | |
| HIDRAW_INTERVAL, /* bInterval: Polling Interval */ \ | |
| /******************** Descriptor of Custom out endpoint ********************/ \ | |
| 0x07, /* bLength: Endpoint Descriptor size */ \ | |
| USB_DESCRIPTOR_TYPE_ENDPOINT, /* bDescriptorType: */ \ | |
| HID_OUT_EP, /* bEndpointAddress: Endpoint Address (IN) */ \ | |
| 0x03, /* bmAttributes: Interrupt endpoint */ \ | |
| WBVAL(HID_PACKET_SIZE), /* wMaxPacketSize: 4 Byte max */ \ | |
| HIDRAW_INTERVAL /* bInterval: Polling Interval */ \ | |
| // clang-format on |
🤖 Prompt for AI Agents
In dap_main.c around lines 141 to 173, the HID interface number is hardcoded as
0x03, which can cause issues if the MSC interface is disabled. Modify the code
to calculate the HID interface number dynamically based on which interfaces are
enabled, ensuring the interface number reflects the actual configuration rather
than a fixed value.
Signed-off-by: sakumisu <[email protected]>
Deploying cherrydap with
|
| Latest commit: |
c0b440e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://061385f9.cherrydap.pages.dev |
| Branch Preview URL: | https://refactor.cherrydap.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
dap_main.c (1)
141-173: Interface number should be dynamically calculated.The HID interface number is still hardcoded as
0x03on line 145. This issue was previously identified and should be fixed.
🧹 Nitpick comments (9)
dap_main.c (2)
41-45: Consider dynamic interface numbering.The MSC interface number is conditionally set but assumes a fixed ordering. This could break if interfaces are reordered.
Consider calculating interface numbers dynamically based on which interfaces are actually enabled, rather than hardcoding values.
689-718: Add comment about test implementation.This MSC implementation uses RAM-based storage limited to 10 blocks. Consider adding a comment to clarify this is for testing purposes only.
#if CONFIG_CHERRYDAP_USE_MSC +// Simple RAM-based MSC implementation for testing purposes only #define BLOCK_SIZE 512 #define BLOCK_COUNT 10projects/HSLink-OB/main.c (1)
27-39: Consider adding error handling for OTP reads.The UUID reading from OTP should handle potential read failures gracefully.
static void serial_number_init(void) { #define OTP_CHIP_UUID_IDX_START (88U) #define OTP_CHIP_UUID_IDX_END (91U) uint32_t uuid_words[4]; uint32_t word_idx = 0; for (uint32_t i = OTP_CHIP_UUID_IDX_START; i <= OTP_CHIP_UUID_IDX_END; i++) { - uuid_words[word_idx++] = ROM_API_TABLE_ROOT->otp_driver_if->read_from_shadow(i); + uint32_t value = ROM_API_TABLE_ROOT->otp_driver_if->read_from_shadow(i); + if (value == 0xFFFFFFFF) { + // Handle potential read error or unprogrammed OTP + printf("Warning: OTP index %u may be unprogrammed\n", i); + } + uuid_words[word_idx++] = value; } sprintf(serial_number_dynamic, "%08X%08X%08X%08X", uuid_words[0], uuid_words[1], uuid_words[2], uuid_words[3]); printf("Serial number: %s\n", serial_number_dynamic); }projects/HSLink-OB/board/hslinkob/pinmux.c (2)
37-40: Remove empty else blockThe empty else block serves no purpose and can be removed for cleaner code.
} else if (ptr == HPM_UART3) { HPM_IOC->PAD[IOC_PAD_PB15].FUNC_CTL = IOC_PB15_FUNC_CTL_UART3_TXD; HPM_IOC->PAD[IOC_PAD_PB14].FUNC_CTL = IOC_PB14_FUNC_CTL_UART3_RXD; - } else { - ; } }
122-124: Remove or explain commented codePlease either remove the commented-out code or add a comment explaining why it needs to be kept for future reference.
} else if (ptr == HPM_GPTMR1) { -// HPM_IOC->PAD[IOC_PAD_PA02].FUNC_CTL = IOC_PA02_FUNC_CTL_GPTMR1_COMP_1; + /* PA02 can be configured as GPTMR1_COMP_1 if needed */ }projects/HSLink-OB/board/hslinkob/board.h (1)
44-49: Translate Chinese comment to English for consistencyThe comment is in Chinese while the rest of the file uses English. Please translate for consistency.
/** - * @brief 比较硬件版本号是否相同，如果输入的是UINT8_MAX，则不比较这一位以及后面的版本号 - * @return 如果硬件版本号相同，返回true，否则返回false + * @brief Compare hardware version numbers. If input is UINT8_MAX, skip comparing this and subsequent version numbers + * @return Returns true if hardware versions match, false otherwise */ bool CheckHardwareVersion(uint8_t major, uint8_t minor, uint8_t patch);projects/HSLink-OB/board/hslinkob/board.c (3)
206-206: Add comment explaining the ramp-up time calculationThe magic number calculation would benefit from an explanation of how the 9ms ramp-up time is calculated.
- /* Configure the External OSC ramp-up time: ~9ms */ - pllctlv2_xtal_set_rampup_time(HPM_PLLCTLV2, 32UL * 1000UL * 9U); + /* Configure the External OSC ramp-up time: ~9ms + * Formula: 32MHz * 1000 * 9ms = 288000 cycles */ + pllctlv2_xtal_set_rampup_time(HPM_PLLCTLV2, 32UL * 1000UL * 9U);
237-241: Consider using named constants for PLL configurationThe PLL post-divider values and frequencies could be defined as named constants for better maintainability.
+ /* PLL0 configuration constants */ + #define PLL0_CLK0_FREQ_MHZ 720 + #define PLL0_CLK0_POSTDIV 0 + #define PLL0_CLK1_POSTDIV 3 + #define PLL0_CLK2_POSTDIV 7 + /* Configure PLL0 Post Divider */ - pllctlv2_set_postdiv(HPM_PLLCTLV2, 0, 0, 0); /* PLL0CLK0: 720MHz */ - pllctlv2_set_postdiv(HPM_PLLCTLV2, 0, 1, 3); /* PLL0CLK1: 450MHz */ - pllctlv2_set_postdiv(HPM_PLLCTLV2, 0, 2, 7); /* PLL0CLK2: 300MHz */ + pllctlv2_set_postdiv(HPM_PLLCTLV2, 0, 0, PLL0_CLK0_POSTDIV); /* PLL0CLK0: 720MHz */ + pllctlv2_set_postdiv(HPM_PLLCTLV2, 0, 1, PLL0_CLK1_POSTDIV); /* PLL0CLK1: 450MHz */ + pllctlv2_set_postdiv(HPM_PLLCTLV2, 0, 2, PLL0_CLK2_POSTDIV); /* PLL0CLK2: 300MHz */ /* Configure PLL0 Frequency to 720MHz */ - pllctlv2_init_pll_with_freq(HPM_PLLCTLV2, 0, 720000000); + pllctlv2_init_pll_with_freq(HPM_PLLCTLV2, 0, PLL0_CLK0_FREQ_MHZ * 1000000);
388-396: Add TODO comments for unimplemented RGB LED functionsThe RGB LED control functions are empty stubs. Consider adding TODO comments if implementation is planned.
void board_disable_output_rgb_led(uint8_t color) { + /* TODO: Implement RGB LED control when hardware is available */ (void)color; } void board_enable_output_rgb_led(uint8_t color) { + /* TODO: Implement RGB LED control when hardware is available */ (void)color; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
projects/HSLink-OB/board/hslinkob/doc/hpm5301evklite.pngis excluded by!**/*.png
📒 Files selected for processing (19)
dap_main.c(14 hunks)dap_main.h(3 hunks)projects/HSLink-OB/CMakeLists.txt(1 hunks)projects/HSLink-OB/board/hslinkob/CMakeLists.txt(1 hunks)projects/HSLink-OB/board/hslinkob/README_en.md(1 hunks)projects/HSLink-OB/board/hslinkob/README_zh.md(1 hunks)projects/HSLink-OB/board/hslinkob/board.c(1 hunks)projects/HSLink-OB/board/hslinkob/board.h(1 hunks)projects/HSLink-OB/board/hslinkob/hslinkob.yaml(1 hunks)projects/HSLink-OB/board/hslinkob/pinmux.c(1 hunks)projects/HSLink-OB/board/hslinkob/pinmux.h(1 hunks)projects/HSLink-OB/dp_common.c(2 hunks)projects/HSLink-OB/main.c(3 hunks)projects/HSLink-Pro/src/CMakeLists.txt(2 hunks)projects/HSLink-Pro/src/HID_COMM/hid_comm.cpp(3 hunks)projects/HSLink-Pro/src/HID_COMM/hid_comm.h(0 hunks)projects/HSLink-Pro/src/main.cpp(3 hunks)projects/HSLink-Pro/src/usb_configuration.c(0 hunks)projects/HSLink-Pro/src/usb_configuration.h(0 hunks)
💤 Files with no reviewable changes (3)
- projects/HSLink-Pro/src/usb_configuration.c
- projects/HSLink-Pro/src/HID_COMM/hid_comm.h
- projects/HSLink-Pro/src/usb_configuration.h
✅ Files skipped from review due to trivial changes (3)
- projects/HSLink-OB/CMakeLists.txt
- projects/HSLink-OB/board/hslinkob/CMakeLists.txt
- projects/HSLink-OB/board/hslinkob/hslinkob.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- projects/HSLink-Pro/src/main.cpp
- projects/HSLink-Pro/src/CMakeLists.txt
- projects/HSLink-Pro/src/HID_COMM/hid_comm.cpp
🧰 Additional context used
🧬 Code Graph Analysis (1)
projects/HSLink-OB/board/hslinkob/pinmux.c (1)
projects/HSLink-OB/board/hslinkob/pinmux.h (16)
init_py_pins_as_pgpio(14-14)init_uart_pins(15-15)init_uart_pin_as_gpio(16-16)init_i2c_pins(17-17)init_gpio_pins(18-18)init_spi_pins(19-19)init_spi_pins_with_gpio_as_cs(20-20)init_gptmr_pins(21-21)init_butn_pins(22-22)init_acmp_pins(23-23)init_adc_pins(24-24)init_adc_bldc_pins(25-25)init_usb_pins(26-26)init_led_pins_as_gpio(27-27)init_uart_break_signal_pin(28-28)init_gptmr_channel_pin(29-29)
🪛 markdownlint-cli2 (0.17.2)
projects/HSLink-OB/board/hslinkob/README_en.md
49-49: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
50-50: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
51-51: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
52-52: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
99-99: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
100-100: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
projects/HSLink-OB/board/hslinkob/README_zh.md
49-49: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
50-50: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
51-51: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
52-52: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
98-98: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
99-99: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
100-100: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (20)
projects/HSLink-OB/dp_common.c (5)
18-20: LGTM!The macro correctly calculates the maximum SWJ clock frequency based on CPU clock and delay cycles.
21-40: LGTM!The clock delay calculation logic is well-implemented with proper handling of fast/slow modes and appropriate rounding.
122-130: LGTM!The wrapper function correctly selects the appropriate clock delay implementation based on the debug port mode.
18-19: LGTM! Clean macro definition.The
MAX_SWJ_CLOCKmacro correctly calculates the maximum SWJ clock frequency based on CPU clock and I/O timing constraints. The formula is mathematically sound and well-documented.
122-130: Excellent abstraction design!The wrapper function cleanly abstracts the choice between SPI and I/O clock delay mechanisms based on port mode and debug port type. This modular approach improves maintainability and aligns well with the refactoring objectives.
dap_main.h (5)
7-7: LGTM!The HID header include and endpoint definitions follow the established pattern correctly.
Also applies to: 22-24
40-44: LGTM!The HID packet size definitions correctly match USB specification limits for interrupt endpoints in HS and FS modes.
49-56: LGTM!The configuration macros provide clean conditional compilation with sensible defaults.
62-62: LGTM!The changes properly separate interface declarations from implementation details.
Also applies to: 69-69, 75-80
84-91: LGTM!The HID callback declarations provide a clean interface for user-implemented HID event handling.
dap_main.c (6)
3-40: LGTM!The descriptor size macros are well-structured and support modular configuration.
47-138: LGTM!The WinUSB and BOS descriptors are properly formatted with clear constant definitions.
214-245: LGTM!The HID report descriptor correctly defines vendor-specific reports with appropriate sizes.
247-247: LGTM!The dynamic serial number implementation allows runtime configuration, which is useful for unique device identification.
Also applies to: 288-290
453-463: LGTM!The HID endpoint structures are properly defined with correct conditional compilation.
497-529: LGTM!The initialization function provides a clean, modular setup sequence with proper conditional interface registration.
projects/HSLink-OB/main.c (2)
11-12: LGTM!The JTAG configuration fields provide necessary control over JTAG operation modes.
58-58: LGTM!The initialization sequence correctly sets up the serial number and uses the new modular USB initialization.
Also applies to: 65-65
projects/HSLink-OB/board/hslinkob/README_en.md (1)
1-42: LGTM!The documentation provides comprehensive hardware information and clear instructions for board usage.
projects/HSLink-OB/board/hslinkob/pinmux.h (1)
1-35: Well-structured header fileThe pinmux header follows best practices with proper include guards and C++ compatibility.
| MSC_DESCRIPTOR_INIT(MSC_INTF_NUM, MSC_OUT_EP, MSC_IN_EP, DAP_PACKET_SIZE, 0x00), | ||
| #endif |
There was a problem hiding this comment.
Fix interface number inconsistency.
The MSC interface number uses MSC_INTF_NUM in the main config descriptor but hardcoded 0x04 in the other speed config descriptor. Both should use the same approach.
- MSC_DESCRIPTOR_INIT(0x04, MSC_OUT_EP, MSC_IN_EP, DAP_PACKET_SIZE, 0x00),
+ MSC_DESCRIPTOR_INIT(MSC_INTF_NUM, MSC_OUT_EP, MSC_IN_EP, DAP_PACKET_SIZE, 0x00),Also applies to: 209-209
🤖 Prompt for AI Agents
In dap_main.c at lines 192-193 and line 209, replace the hardcoded interface
number 0x04 in the other speed configuration descriptor with the macro
MSC_INTF_NUM to ensure consistency with the main configuration descriptor. This
means using MSC_INTF_NUM instead of the literal 0x04 in both places.
| | Function | Position | Remark | | ||
| | --------- | -------- | ------ | | ||
| | UART3.TXD | J3[8] | | ||
| | UART3.RXD | J3[10] | | ||
| | UART0.TXD | J3[36] | | ||
| | UART0.RXD | J3[38] | | ||
| | UART3.break | J3[24] | generate uart break signal | | ||
|
|
There was a problem hiding this comment.
Fix table formatting - missing column data.
The UART pin table has inconsistent column count. Add empty remarks for consistency.
| Function | Position | Remark |
| --------- | -------- | ------ |
-| UART3.TXD | J3[8] |
-| UART3.RXD | J3[10] |
-| UART0.TXD | J3[36] |
-| UART0.RXD | J3[38] |
+| UART3.TXD | J3[8] | |
+| UART3.RXD | J3[10] | |
+| UART0.TXD | J3[36] | |
+| UART0.RXD | J3[38] | |
| UART3.break | J3[24] | generate uart break signal |📝 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.
| | Function | Position | Remark | | |
| | --------- | -------- | ------ | | |
| | UART3.TXD | J3[8] | | |
| | UART3.RXD | J3[10] | | |
| | UART0.TXD | J3[36] | | |
| | UART0.RXD | J3[38] | | |
| | UART3.break | J3[24] | generate uart break signal | | |
| | Function | Position | Remark | | |
| | ------------- | -------- | ---------------------------- | | |
| | UART3.TXD | J3[8] | | | |
| | UART3.RXD | J3[10] | | | |
| | UART0.TXD | J3[36] | | | |
| | UART0.RXD | J3[38] | | | |
| | UART3.break | J3[24] | generate uart break signal | |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
49-49: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
50-50: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
51-51: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
52-52: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
🤖 Prompt for AI Agents
In projects/HSLink-OB/board/hslinkob/README_en.md between lines 47 and 54, the
UART pin table has inconsistent column counts because some rows lack data in the
Remark column. Add empty placeholders in the Remark column for rows missing this
data to ensure all rows have the same number of columns and maintain proper
table formatting.
|
|
||
| | Function | Position | Remark | | ||
| | ------------- | ----- | ------ | | ||
| | GPTMR0.CAPT_1 | J3[3] | | ||
| | GPTMR0.COMP_1 | J3[5] | | ||
| | GPTMR0.COMP_3 | J3[8] | BCLK of i2s emulation | | ||
| | GPTMR0.COMP_2 | J3[26] | LRCK of i2s emulation | | ||
| | GPTMR1.COMP_1 | J3[7] | MLCK of i2s emulation | | ||
|
|
There was a problem hiding this comment.
Fix table formatting - missing column data.
The GPTMR pin table has inconsistent column count. Add empty remarks for consistency.
| Function | Position | Remark |
| ------------- | ----- | ------ |
-| GPTMR0.CAPT_1 | J3[3] |
-| GPTMR0.COMP_1 | J3[5] |
+| GPTMR0.CAPT_1 | J3[3] | |
+| GPTMR0.COMP_1 | J3[5] | |
| GPTMR0.COMP_3 | J3[8] | BCLK of i2s emulation |
| GPTMR0.COMP_2 | J3[26] | LRCK of i2s emulation |
| GPTMR1.COMP_1 | J3[7] | MLCK of i2s emulation |📝 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.
| | Function | Position | Remark | | |
| | ------------- | ----- | ------ | | |
| | GPTMR0.CAPT_1 | J3[3] | | |
| | GPTMR0.COMP_1 | J3[5] | | |
| | GPTMR0.COMP_3 | J3[8] | BCLK of i2s emulation | | |
| | GPTMR0.COMP_2 | J3[26] | LRCK of i2s emulation | | |
| | GPTMR1.COMP_1 | J3[7] | MLCK of i2s emulation | | |
| | Function | Position | Remark | | |
| | ------------- | ----- | ------ | | |
| | GPTMR0.CAPT_1 | J3[3] | | | |
| | GPTMR0.COMP_1 | J3[5] | | | |
| | GPTMR0.COMP_3 | J3[8] | BCLK of i2s emulation | | |
| | GPTMR0.COMP_2 | J3[26] | LRCK of i2s emulation | | |
| | GPTMR1.COMP_1 | J3[7] | MLCK of i2s emulation | |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
99-99: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
100-100: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
🤖 Prompt for AI Agents
In projects/HSLink-OB/board/hslinkob/README_en.md lines 96 to 104, the GPTMR pin
table has inconsistent column counts due to missing remarks in some rows. Add
empty remark cells (e.g., a blank space or dash) for rows without remarks to
ensure all rows have the same number of columns and maintain proper table
formatting.
| | 功能 | 位置 | 备注 | | ||
| | --------- | ------ | ------ | | ||
| | UART3.TXD | J3[8] | | ||
| | UART3.RXD | J3[10] | | ||
| | UART0.TXD | J3[36] | | ||
| | UART0.RXD | J3[38] | | ||
| | UART3.break | J3[24] | 产生uart break信号| | ||
|
|
There was a problem hiding this comment.
Fix table column count mismatch
The table declares 3 columns in the header but some rows only have 2 columns. Add the missing column values to maintain consistency.
| 功能 | 位置 | 备注 |
| --------- | ------ | ------ |
-| UART3.TXD | J3[8] |
-| UART3.RXD | J3[10] |
-| UART0.TXD | J3[36] |
-| UART0.RXD | J3[38] |
+| UART3.TXD | J3[8] | |
+| UART3.RXD | J3[10] | |
+| UART0.TXD | J3[36] | |
+| UART0.RXD | J3[38] | |
| UART3.break | J3[24] | 产生uart break信号|📝 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.
| | 功能 | 位置 | 备注 | | |
| | --------- | ------ | ------ | | |
| | UART3.TXD | J3[8] | | |
| | UART3.RXD | J3[10] | | |
| | UART0.TXD | J3[36] | | |
| | UART0.RXD | J3[38] | | |
| | UART3.break | J3[24] | 产生uart break信号| | |
| | 功能 | 位置 | 备注 | | |
| | --------- | ------ | ------ | | |
| | UART3.TXD | J3[8] | | | |
| | UART3.RXD | J3[10] | | | |
| | UART0.TXD | J3[36] | | | |
| | UART0.RXD | J3[38] | | | |
| | UART3.break | J3[24] | 产生uart break信号| |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
49-49: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
50-50: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
51-51: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
52-52: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
🤖 Prompt for AI Agents
In projects/HSLink-OB/board/hslinkob/README_zh.md around lines 47 to 54, the
markdown table header declares three columns but some rows only have two
columns, causing a column count mismatch. Add the missing third column values
(such as empty placeholders or appropriate remarks) to all rows to ensure each
row has exactly three columns, maintaining table consistency.
| | 功能 | 位置 | 备注 | | ||
| | ------------- | ----- | ------ | ||
| | GPTMR0.CAPT_1 | J3[3] | | ||
| | GPTMR0.COMP_1 | J3[5] | | ||
| | GPTMR0.COMP_3 | J3[8] | SPI模拟I2S的BLCK | | ||
| | GPTMR0.COMP_2 | J3[26] | SPI模拟I2S的LRCK | | ||
| | GPTMR1.COMP_1 | J3[7] | SPI模拟I2S的MCLK | | ||
|
|
There was a problem hiding this comment.
Fix table formatting issues
The GPTMR pins table has formatting inconsistencies: missing trailing pipes and column count mismatches.
| 功能 | 位置 | 备注 |
-| ------------- | ----- | ------
-| GPTMR0.CAPT_1 | J3[3] |
-| GPTMR0.COMP_1 | J3[5] |
+| ------------- | ----- | ------ |
+| GPTMR0.CAPT_1 | J3[3] | |
+| GPTMR0.COMP_1 | J3[5] | |
| GPTMR0.COMP_3 | J3[8] | SPI模拟I2S的BLCK |
| GPTMR0.COMP_2 | J3[26] | SPI模拟I2S的LRCK |
| GPTMR1.COMP_1 | J3[7] | SPI模拟I2S的MCLK |📝 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.
| | 功能 | 位置 | 备注 | | |
| | ------------- | ----- | ------ | |
| | GPTMR0.CAPT_1 | J3[3] | | |
| | GPTMR0.COMP_1 | J3[5] | | |
| | GPTMR0.COMP_3 | J3[8] | SPI模拟I2S的BLCK | | |
| | GPTMR0.COMP_2 | J3[26] | SPI模拟I2S的LRCK | | |
| | GPTMR1.COMP_1 | J3[7] | SPI模拟I2S的MCLK | | |
| | 功能 | 位置 | 备注 | | |
| | ------------- | ----- | ------ | | |
| | GPTMR0.CAPT_1 | J3[3] | | | |
| | GPTMR0.COMP_1 | J3[5] | | | |
| | GPTMR0.COMP_3 | J3[8] | SPI模拟I2S的BLCK | | |
| | GPTMR0.COMP_2 | J3[26] | SPI模拟I2S的LRCK | | |
| | GPTMR1.COMP_1 | J3[7] | SPI模拟I2S的MCLK | |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
98-98: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
99-99: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
100-100: Table column count
Expected: 3; Actual: 2; Too few cells, row will be missing data
(MD056, table-column-count)
🤖 Prompt for AI Agents
In projects/HSLink-OB/board/hslinkob/README_zh.md around lines 97 to 104, the
GPTMR pins table has formatting issues such as missing trailing pipes and
inconsistent column counts. Fix the table by ensuring each row ends with a pipe
character and all rows have the same number of columns to maintain proper
markdown table structure.
projects/HSLink-OB/dp_common.c
Outdated
| void IO_Set_Clock_Delay(uint32_t clock) { | ||
| uint32_t delay; | ||
|
|
||
| if (clock >= MAX_SWJ_CLOCK(DELAY_FAST_CYCLES)) { | ||
| DAP_Data.fast_clock = 1U; | ||
| DAP_Data.clock_delay = 1U; | ||
| } else { | ||
| DAP_Data.fast_clock = 0U; | ||
|
|
||
| delay = ((CPU_CLOCK/2U) + (clock - 1U)) / clock; | ||
| if (delay > IO_PORT_WRITE_CYCLES) { | ||
| delay -= IO_PORT_WRITE_CYCLES; | ||
| delay = (delay + (DELAY_SLOW_CYCLES - 1U)) / DELAY_SLOW_CYCLES; | ||
| } else { | ||
| delay = 1U; | ||
| } | ||
|
|
||
| DAP_Data.clock_delay = delay; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add input validation to prevent division by zero.
The function logic is correct for calculating clock delays, but it's missing input validation for the clock parameter.
Apply this diff to add input validation:
void IO_Set_Clock_Delay(uint32_t clock) {
uint32_t delay;
+
+ if (clock == 0U) {
+ DAP_Data.fast_clock = 0U;
+ DAP_Data.clock_delay = 1U;
+ return;
+ }
if (clock >= MAX_SWJ_CLOCK(DELAY_FAST_CYCLES)) {📝 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 IO_Set_Clock_Delay(uint32_t clock) { | |
| uint32_t delay; | |
| if (clock >= MAX_SWJ_CLOCK(DELAY_FAST_CYCLES)) { | |
| DAP_Data.fast_clock = 1U; | |
| DAP_Data.clock_delay = 1U; | |
| } else { | |
| DAP_Data.fast_clock = 0U; | |
| delay = ((CPU_CLOCK/2U) + (clock - 1U)) / clock; | |
| if (delay > IO_PORT_WRITE_CYCLES) { | |
| delay -= IO_PORT_WRITE_CYCLES; | |
| delay = (delay + (DELAY_SLOW_CYCLES - 1U)) / DELAY_SLOW_CYCLES; | |
| } else { | |
| delay = 1U; | |
| } | |
| DAP_Data.clock_delay = delay; | |
| } | |
| } | |
| void IO_Set_Clock_Delay(uint32_t clock) { | |
| uint32_t delay; | |
| if (clock == 0U) { | |
| DAP_Data.fast_clock = 0U; | |
| DAP_Data.clock_delay = 1U; | |
| return; | |
| } | |
| if (clock >= MAX_SWJ_CLOCK(DELAY_FAST_CYCLES)) { | |
| DAP_Data.fast_clock = 1U; | |
| DAP_Data.clock_delay = 1U; | |
| } else { | |
| DAP_Data.fast_clock = 0U; | |
| delay = ((CPU_CLOCK/2U) + (clock - 1U)) / clock; | |
| if (delay > IO_PORT_WRITE_CYCLES) { | |
| delay -= IO_PORT_WRITE_CYCLES; | |
| delay = (delay + (DELAY_SLOW_CYCLES - 1U)) / DELAY_SLOW_CYCLES; | |
| } else { | |
| delay = 1U; | |
| } | |
| DAP_Data.clock_delay = delay; | |
| } | |
| } |
🤖 Prompt for AI Agents
In projects/HSLink-OB/dp_common.c around lines 21 to 40, the function
IO_Set_Clock_Delay lacks validation for the clock parameter, which can lead to
division by zero errors. Add a check at the start of the function to return
early or handle the case when clock is zero, preventing any division operations
with zero. This ensures safe execution and avoids runtime errors.
| # HPM5301EVKLITE开发板 | ||
|
|
||
| ## 概述 | ||
|
|
||
| HPM5301EVKLite是基于先楫的入门级高性能MCU HPM5301的开发板。HPM5301EVKLite提供了一个USB Type-C接口实现高速的USB-OTG功能，板载的按键和LED方便用户交互，同时提供了一个适配了树莓派的扩展接口和一个标准的JTAG调试接口。 | ||
|
|
||
|  | ||
|
|
||
| ## console 信息打印 | ||
|
|
||
| 默认使用 UART0 进行console打印，通过USB转串口工具外接UART0.TXD（J3.36）和UART0.RXD（J3.38）。 | ||
|
|
||
| ## BOOT切换 |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
dap_main.c (2)
141-173: Interface number calculation still needs to be dynamic.The HID interface number is still hardcoded as
0x03on line 145. Based on the conditional compilation logic, this should be calculated dynamically to account for when MSC is disabled.Apply this fix to calculate the interface number dynamically:
- 0X03, /* bInterfaceNumber: Number of Interface */ \ + (1 + 2), /* bInterfaceNumber: Number of Interface (WinUSB + CDC ACM) */ \
209-209: Fix inconsistent interface number usage.The MSC interface uses a hardcoded
0x04here butMSC_INTF_NUMelsewhere. This should be consistent.- MSC_DESCRIPTOR_INIT(0x04, MSC_OUT_EP, MSC_IN_EP, DAP_PACKET_SIZE, 0x00), + MSC_DESCRIPTOR_INIT(MSC_INTF_NUM, MSC_OUT_EP, MSC_IN_EP, DAP_PACKET_SIZE, 0x00),projects/HSLink-OB/board/hslinkob/README_en.md (2)
47-54: Fix table formatting - missing column data.The UART pin table has inconsistent column count. Add empty remarks for consistency.
| Function | Position | Remark | | --------- | -------- | ------ | -| UART3.TXD | J3[8] | -| UART3.RXD | J3[10] | -| UART0.TXD | J3[36] | -| UART0.RXD | J3[38] | +| UART3.TXD | J3[8] | | +| UART3.RXD | J3[10] | | +| UART0.TXD | J3[36] | | +| UART0.RXD | J3[38] | | | UART3.break | J3[24] | generate uart break signal |
96-104: Fix table formatting - missing column data.The GPTMR pin table has inconsistent column count. Add empty remarks for consistency.
| Function | Position | Remark | | ------------- | ----- | ------ | -| GPTMR0.CAPT_1 | J3[3] | -| GPTMR0.COMP_1 | J3[5] | +| GPTMR0.CAPT_1 | J3[3] | | +| GPTMR0.COMP_1 | J3[5] | | | GPTMR0.COMP_3 | J3[8] | BCLK of i2s emulation | | GPTMR0.COMP_2 | J3[26] | LRCK of i2s emulation | | GPTMR1.COMP_1 | J3[7] | MLCK of i2s emulation |projects/HSLink-OB/board/hslinkob/README_zh.md (2)
47-54: Fix table column count mismatch.The table declares 3 columns in the header but some rows only have 2 columns. Add the missing column values to maintain consistency.
| 功能 | 位置 | 备注 | | --------- | ------ | ------ | -| UART3.TXD | J3[8] | -| UART3.RXD | J3[10] | -| UART0.TXD | J3[36] | -| UART0.RXD | J3[38] | +| UART3.TXD | J3[8] | | +| UART3.RXD | J3[10] | | +| UART0.TXD | J3[36] | | +| UART0.RXD | J3[38] | | | UART3.break | J3[24] | 产生uart break信号|
97-104: Fix table formatting issues.The GPTMR pins table has formatting inconsistencies: missing trailing pipes and column count mismatches.
| 功能 | 位置 | 备注 | -| ------------- | ----- | ------ -| GPTMR0.CAPT_1 | J3[3] | -| GPTMR0.COMP_1 | J3[5] | +| ------------- | ----- | ------ | +| GPTMR0.CAPT_1 | J3[3] | | +| GPTMR0.COMP_1 | J3[5] | | | GPTMR0.COMP_3 | J3[8] | SPI模拟I2S的BLCK | | GPTMR0.COMP_2 | J3[26] | SPI模拟I2S的LRCK | | GPTMR1.COMP_1 | J3[7] | SPI模拟I2S的MCLK |
🧹 Nitpick comments (8)
dap_main.c (1)
247-247: Consider making the serial number buffer size configurable.The 36-byte serial number buffer is hardcoded. Consider defining this as a macro for maintainability.
+#define SERIAL_NUMBER_MAX_LEN 36 -char serial_number_dynamic[36] = "00000000000000000123456789ABCDEF"; // Dynamic serial number +char serial_number_dynamic[SERIAL_NUMBER_MAX_LEN] = "00000000000000000123456789ABCDEF"; // Dynamic serial numberprojects/HSLink-OB/board/hslinkob/README_en.md (1)
88-89: Fix grammatical errors in TinyUF2 section.The verb forms don't agree with the subjects. Use proper subject-verb agreement.
- - 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； + - Connect PA9 to GND and press reset, the board enters DFU mode. Then connect PA9 to 3.3V, drag the app to the U disk, which will download the app and enter it directly if successful; + - Connect PA9 to 3.3V and press reset, the board enters bootloader mode. If flash has a valid app, it will directly enter the app;projects/HSLink-OB/board/hslinkob/README_zh.md (1)
1-13: Consider improving the README description as suggested.A user has commented that the README description should be improved. Consider adding more specific technical details about the board's capabilities, supported protocols, or unique features that distinguish it from other development boards.
Would you like me to suggest specific improvements to the board description or help draft additional technical content?
projects/HSLink-OB/board/hslinkob/pinmux.c (1)
123-124: Clarify commented GPTMR1 configuration.Line 123 has a commented-out GPTMR1 configuration. Consider either removing this commented code or adding a comment explaining why it's disabled.
} else if (ptr == HPM_GPTMR1) { -// HPM_IOC->PAD[IOC_PAD_PA02].FUNC_CTL = IOC_PA02_FUNC_CTL_GPTMR1_COMP_1; + // GPTMR1 COMP_1 configuration disabled - add reason here if neededprojects/HSLink-OB/board/hslinkob/board.h (1)
309-309: Consider adding parameter documentation for callback typedef.The
board_timer_cbtypedef could benefit from brief documentation about the callback context and constraints.+/* Timer callback function type - called from interrupt context */ typedef void (*board_timer_cb)(void);projects/HSLink-OB/board/hslinkob/board.c (3)
200-251: Replace magic numbers with named constants.The clock configuration uses several magic numbers that would benefit from named constants for better maintainability.
Consider defining constants like:
+#define XTAL_RAMPUP_TIME_MS 9U +#define CLOCK_PRESET_HIGH_PERF 2 +#define DCDC_VOLTAGE_MV 1175 +#define CPU_FREQ_MHZ 360 +#define PLL0_FREQ_HZ 720000000Then use these constants in the implementation for better code clarity.
416-432: Consider timeout handling for stuck SCL condition.The infinite loop when SCL is stuck low could hang the system indefinitely.
Consider adding a timeout or allowing the system to proceed with degraded functionality:
if (i2c_get_line_scl_status(ptr) == false) { printf("CLK is low, please power cycle the board\n"); - while (1) { - } + /* Could return error code or implement timeout-based recovery */ + return; /* Allow system to continue with I2C disabled */ }
388-396: Document stub implementations.These RGB LED functions are currently stubs - consider adding comments to clarify if this is intentional for this board variant.
void board_disable_output_rgb_led(uint8_t color) { + /* RGB LED not implemented on this board variant */ (void)color; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
projects/HSLink-OB/board/hslinkob/doc/hpm5301evklite.pngis excluded by!**/*.png
📒 Files selected for processing (12)
dap_main.c(14 hunks)projects/HSLink-OB/CMakeLists.txt(1 hunks)projects/HSLink-OB/board/hslinkob/CMakeLists.txt(1 hunks)projects/HSLink-OB/board/hslinkob/README_en.md(1 hunks)projects/HSLink-OB/board/hslinkob/README_zh.md(1 hunks)projects/HSLink-OB/board/hslinkob/board.c(1 hunks)projects/HSLink-OB/board/hslinkob/board.h(1 hunks)projects/HSLink-OB/board/hslinkob/hslinkob.yaml(1 hunks)projects/HSLink-OB/board/hslinkob/pinmux.c(1 hunks)projects/HSLink-OB/board/hslinkob/pinmux.h(1 hunks)projects/HSLink-OB/dp_common.c(2 hunks)projects/HSLink-OB/main.c(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- projects/HSLink-OB/board/hslinkob/hslinkob.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- projects/HSLink-OB/CMakeLists.txt
- projects/HSLink-OB/board/hslinkob/CMakeLists.txt
- projects/HSLink-OB/main.c
- projects/HSLink-OB/dp_common.c
- projects/HSLink-OB/board/hslinkob/pinmux.h
🧰 Additional context used
🧬 Code Graph Analysis (1)
projects/HSLink-OB/board/hslinkob/pinmux.c (1)
projects/HSLink-OB/board/hslinkob/pinmux.h (16)
init_py_pins_as_pgpio(14-14)init_uart_pins(15-15)init_uart_pin_as_gpio(16-16)init_i2c_pins(17-17)init_gpio_pins(18-18)init_spi_pins(19-19)init_spi_pins_with_gpio_as_cs(20-20)init_gptmr_pins(21-21)init_butn_pins(22-22)init_acmp_pins(23-23)init_adc_pins(24-24)init_adc_bldc_pins(25-25)init_usb_pins(26-26)init_led_pins_as_gpio(27-27)init_uart_break_signal_pin(28-28)init_gptmr_channel_pin(29-29)
🪛 LanguageTool
projects/HSLink-OB/board/hslinkob/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)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (12)
dap_main.c (1)
497-529: Excellent modular initialization function.The
chry_dap_init()function provides a clean, consolidated approach to USB device setup with proper conditional compilation for optional interfaces. This addresses the PR objective of merging HID config into dap_main effectively.projects/HSLink-OB/board/hslinkob/pinmux.c (3)
18-27: Good implementation of PY pins initialization.The function correctly initializes PY00-PY05 pins as PGPIO using PIOC, which aligns with the note about special handling for PY/PZ IOs.
56-69: Excellent I2C pin configuration.The I2C pin setup correctly configures open-drain, pull-up, and loopback settings, which are essential for proper I2C operation.
164-182: Proper USB pin configuration with PIOC handling.The USB pin configuration correctly handles both the analog pin setup and the special PIOC configuration required for PY port IOs. This demonstrates good understanding of the hardware requirements.
projects/HSLink-OB/board/hslinkob/board.h (3)
30-48: Good version checking design.The hardware version structure and comparison function provide a flexible way to handle different board revisions. The use of UINT8_MAX for wildcard matching is a clever approach.
311-367: Comprehensive board API design.The function declarations provide a well-structured board abstraction layer covering all major peripherals and initialization needs. The separation of concerns between clock, pin, and peripheral initialization is good design.
141-153: File “projects/HSLink-OB/board/hslinkob/board.h”: verify GPTMR0 channel assignmentsMultiple functions are mapped to the same GPTMR0 channels, which will conflict if they ever run at the same time:
- Channel 1: BOARD_GPTMR & BOARD_GPTMR_PWM
- Channel 2: BOARD_GPTMR_PWM_SYNC & BOARD_GPTMR_I2S_LRCK
- Channel 0: BOARD_GPTMR_I2S_FINSH (no overlap)
- Channel 3: BOARD_GPTMR_I2S_BCLK (no overlap)
Please review these mappings and either reassign channels or enforce exclusive usage when these features are active.
projects/HSLink-OB/board/hslinkob/board.c (5)
22-77: Well-documented flash configuration section.The extensive documentation and clear bit field explanations make this configuration section maintainable and understandable.
457-474: Good error handling with appropriate failure response.The function properly checks initialization status and provides clear error messages. The infinite loop on failure is appropriate for critical hardware initialization.
86-115: Excellent console initialization with proper sequencing.The initialization properly handles pin configuration before clock enabling, with clear comments explaining the rationale. Error handling is appropriate for critical console setup.
117-133: Good branding and version information display.The banner and version information provide clear board identification and software version tracking.
292-479: Consistent and well-implemented peripheral initialization functions.The peripheral initialization functions follow good patterns with proper pin configuration, clock management, and parameter validation.
| const uint8_t hid_custom_report_desc[HID_CUSTOM_REPORT_DESC_SIZE] = { | ||
| /* USER CODE BEGIN 0 */ | ||
| 0x06, 0x00, 0xff, /* USAGE_PAGE (Vendor Defined Page 1) */ | ||
| 0x09, 0x01, /* USAGE (Vendor Usage 1) */ | ||
| 0xa1, 0x01, /* COLLECTION (Application) */ | ||
| 0x85, 0x02, /* REPORT ID (0x02) */ | ||
| 0x09, 0x02, /* USAGE (Vendor Usage 1) */ | ||
| 0x15, 0x00, /* LOGICAL_MINIMUM (0) */ | ||
| 0x25, 0xff, /*LOGICAL_MAXIMUM (255) */ | ||
| 0x75, 0x08, /* REPORT_SIZE (8) */ | ||
| 0x96, 0xff, 0x03, /* REPORT_COUNT (1023) */ | ||
| 0x81, 0x02, /* INPUT (Data,Var,Abs) */ | ||
| /* <___________________________________________________> */ | ||
| 0x85, 0x01, /* REPORT ID (0x01) */ | ||
| 0x09, 0x03, /* USAGE (Vendor Usage 1) */ | ||
| 0x15, 0x00, /* LOGICAL_MINIMUM (0) */ | ||
| 0x25, 0xff, /* LOGICAL_MAXIMUM (255) */ | ||
| 0x75, 0x08, /* REPORT_SIZE (8) */ | ||
| 0x96, 0xff, 0x03, /* REPORT_COUNT (1023) */ | ||
| 0x91, 0x02, /* OUTPUT (Data,Var,Abs) */ | ||
|
|
||
| /* <___________________________________________________> */ | ||
| 0x85, 0x03, /* REPORT ID (0x03) */ | ||
| 0x09, 0x04, /* USAGE (Vendor Usage 1) */ | ||
| 0x15, 0x00, /* LOGICAL_MINIMUM (0) */ | ||
| 0x25, 0xff, /* LOGICAL_MAXIMUM (255) */ | ||
| 0x75, 0x08, /* REPORT_SIZE (8) */ | ||
| 0x96, 0xff, 0x03, /* REPORT_COUNT (1023) */ | ||
| 0xb1, 0x02, /* FEATURE (Data,Var,Abs) */ | ||
| /* USER CODE END 0 */ | ||
| 0xC0 /* END_COLLECTION */ | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify HID report descriptor validity.
The custom HID report descriptor specifies very large report counts (1023 bytes) for input, output, and feature reports. Ensure this aligns with the actual buffer sizes and USB transfer capabilities of the target hardware.
The large report counts might cause issues with USB hosts or exceed hardware buffer limitations. Please verify:
🏁 Script executed:
#!/bin/bash
# Search for HID packet size definitions to verify consistency
rg -A 3 "HID.*PACKET.*SIZE" Length of output: 4335
🏁 Script executed:
#!/bin/bash
# Show context around HID_PACKET_SIZE definition to understand when it’s 1024 vs 64
rg -C7 "#define HID_PACKET_SIZE" -n dap_main.h
# Locate the custom report descriptor size macro
rg -C3 "HID_CUSTOM_REPORT_DESC_SIZE" -n .Length of output: 2580
Align HID report counts with the configured packet size
The descriptor’s fixed REPORT_COUNT of 1023 bytes (0x03FF) only matches a 1024-byte packet (HS mode) and will overflow a 64-byte packet (FS mode). Update the descriptor so REPORT_COUNT is derived from HID_PACKET_SIZE:
• In dap_main.h (around lines 40–44) you have:
#ifdef CONFIG_USB_HS
# define HID_PACKET_SIZE 1024
#else
# define HID_PACKET_SIZE 64
#endif• In dap_main.c (lines 214–245) change each
0x96, 0xFF, 0x03
to something like:
/* bytes for REPORT_COUNT = HID_PACKET_SIZE – 1 */
#define HID_REPORT_COUNT_L ((HID_PACKET_SIZE - 1) & 0xFF)
#define HID_REPORT_COUNT_H (((HID_PACKET_SIZE - 1) >> 8) & 0xFF)
…
0x75, 0x08, /* REPORT_SIZE (8) */
0x96, HID_REPORT_COUNT_L, HID_REPORT_COUNT_H, /* REPORT_COUNT */
…This ensures the report descriptor always matches your IN/OUT endpoint’s wMaxPacketSize.
• Verify the host can still issue GET_FEATURE/SET_FEATURE requests of that size on the control endpoint.
🤖 Prompt for AI Agents
In dap_main.c around lines 214 to 245, the fixed REPORT_COUNT value of 1023
bytes (0x03FF) in the HID report descriptor does not adapt to different USB
packet sizes, causing overflow in Full Speed mode. Replace each occurrence of
the bytes 0x96, 0xFF, 0x03 with macros that calculate REPORT_COUNT based on
HID_PACKET_SIZE defined in dap_main.h. Define HID_REPORT_COUNT_L and
HID_REPORT_COUNT_H as the low and high bytes of (HID_PACKET_SIZE - 1), then use
these macros in the descriptor to set REPORT_COUNT dynamically, ensuring it
matches the endpoint's wMaxPacketSize.
| void board_timer_create(uint32_t ms, board_timer_cb cb) | ||
| { | ||
| uint32_t gptmr_freq; | ||
| gptmr_channel_config_t config; | ||
|
|
||
| timer_cb = cb; | ||
| gptmr_channel_get_default_config(BOARD_CALLBACK_TIMER, &config); | ||
|
|
||
| clock_add_to_group(BOARD_CALLBACK_TIMER_CLK_NAME, 0); | ||
| gptmr_freq = clock_get_frequency(BOARD_CALLBACK_TIMER_CLK_NAME); | ||
|
|
||
| config.reload = gptmr_freq / 1000 * ms; | ||
| gptmr_channel_config(BOARD_CALLBACK_TIMER, BOARD_CALLBACK_TIMER_CH, &config, false); | ||
| gptmr_enable_irq(BOARD_CALLBACK_TIMER, GPTMR_CH_RLD_IRQ_MASK(BOARD_CALLBACK_TIMER_CH)); | ||
| intc_m_enable_irq_with_priority(BOARD_CALLBACK_TIMER_IRQ, 1); | ||
|
|
||
| gptmr_start_counter(BOARD_CALLBACK_TIMER, BOARD_CALLBACK_TIMER_CH); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add parameter validation for timer creation.
The function should validate the callback parameter to prevent null pointer assignment.
Add validation:
void board_timer_create(uint32_t ms, board_timer_cb cb)
{
+ if (cb == NULL) {
+ return; /* or handle error appropriately */
+ }
+
uint32_t gptmr_freq;
gptmr_channel_config_t config;📝 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 board_timer_create(uint32_t ms, board_timer_cb cb) | |
| { | |
| uint32_t gptmr_freq; | |
| gptmr_channel_config_t config; | |
| timer_cb = cb; | |
| gptmr_channel_get_default_config(BOARD_CALLBACK_TIMER, &config); | |
| clock_add_to_group(BOARD_CALLBACK_TIMER_CLK_NAME, 0); | |
| gptmr_freq = clock_get_frequency(BOARD_CALLBACK_TIMER_CLK_NAME); | |
| config.reload = gptmr_freq / 1000 * ms; | |
| gptmr_channel_config(BOARD_CALLBACK_TIMER, BOARD_CALLBACK_TIMER_CH, &config, false); | |
| gptmr_enable_irq(BOARD_CALLBACK_TIMER, GPTMR_CH_RLD_IRQ_MASK(BOARD_CALLBACK_TIMER_CH)); | |
| intc_m_enable_irq_with_priority(BOARD_CALLBACK_TIMER_IRQ, 1); | |
| gptmr_start_counter(BOARD_CALLBACK_TIMER, BOARD_CALLBACK_TIMER_CH); | |
| } | |
| void board_timer_create(uint32_t ms, board_timer_cb cb) | |
| { | |
| if (cb == NULL) { | |
| return; /* or handle error appropriately */ | |
| } | |
| uint32_t gptmr_freq; | |
| gptmr_channel_config_t config; | |
| timer_cb = cb; | |
| gptmr_channel_get_default_config(BOARD_CALLBACK_TIMER, &config); | |
| clock_add_to_group(BOARD_CALLBACK_TIMER_CLK_NAME, 0); | |
| gptmr_freq = clock_get_frequency(BOARD_CALLBACK_TIMER_CLK_NAME); | |
| config.reload = gptmr_freq / 1000 * ms; | |
| gptmr_channel_config(BOARD_CALLBACK_TIMER, BOARD_CALLBACK_TIMER_CH, &config, false); | |
| gptmr_enable_irq(BOARD_CALLBACK_TIMER, GPTMR_CH_RLD_IRQ_MASK(BOARD_CALLBACK_TIMER_CH)); | |
| intc_m_enable_irq_with_priority(BOARD_CALLBACK_TIMER_IRQ, 1); | |
| gptmr_start_counter(BOARD_CALLBACK_TIMER, BOARD_CALLBACK_TIMER_CH); | |
| } |
🤖 Prompt for AI Agents
In projects/HSLink-OB/board/hslinkob/board.c around lines 273 to 290, the
function board_timer_create does not validate the callback parameter, which can
lead to null pointer assignment. Add a check at the start of the function to
verify that the cb parameter is not NULL; if it is NULL, return early or handle
the error appropriately to prevent assigning a null callback.
| void board_timer_isr(void) | ||
| { | ||
| if (gptmr_check_status(BOARD_CALLBACK_TIMER, GPTMR_CH_RLD_STAT_MASK(BOARD_CALLBACK_TIMER_CH))) { | ||
| gptmr_clear_status(BOARD_CALLBACK_TIMER, GPTMR_CH_RLD_STAT_MASK(BOARD_CALLBACK_TIMER_CH)); | ||
| timer_cb(); | ||
| } |
There was a problem hiding this comment.
Add null pointer check for timer callback.
The ISR calls the timer callback without checking if it's null, which could cause a crash.
Add safety check:
void board_timer_isr(void)
{
if (gptmr_check_status(BOARD_CALLBACK_TIMER, GPTMR_CH_RLD_STAT_MASK(BOARD_CALLBACK_TIMER_CH))) {
gptmr_clear_status(BOARD_CALLBACK_TIMER, GPTMR_CH_RLD_STAT_MASK(BOARD_CALLBACK_TIMER_CH));
- timer_cb();
+ if (timer_cb != NULL) {
+ timer_cb();
+ }
}
}🤖 Prompt for AI Agents
In projects/HSLink-OB/board/hslinkob/board.c around lines 265 to 270, the
board_timer_isr function calls timer_cb() without verifying if the timer
callback pointer is null, risking a crash. Add a null pointer check before
calling timer_cb() to ensure it is not null, and only invoke it if the check
passes.
| void board_init_usb_dp_dm_pins(void) | ||
| { | ||
| /* Disconnect usb dp/dm pins pull down 45ohm resistance */ | ||
|
|
||
| while (sysctl_resource_any_is_busy(HPM_SYSCTL)) { | ||
| ; | ||
| } | ||
| if (pllctlv2_xtal_is_stable(HPM_PLLCTLV2) && pllctlv2_xtal_is_enabled(HPM_PLLCTLV2)) { | ||
| if (clock_check_in_group(clock_usb0, 0)) { | ||
| usb_phy_disable_dp_dm_pulldown(HPM_USB0); | ||
| } else { | ||
| clock_add_to_group(clock_usb0, 0); | ||
| usb_phy_disable_dp_dm_pulldown(HPM_USB0); | ||
| clock_remove_from_group(clock_usb0, 0); | ||
| } | ||
| } else { | ||
| uint8_t tmp; | ||
| tmp = sysctl_resource_target_get_mode(HPM_SYSCTL, sysctl_resource_xtal); | ||
| sysctl_resource_target_set_mode(HPM_SYSCTL, sysctl_resource_xtal, 0x03); /* NOLINT */ | ||
| clock_add_to_group(clock_usb0, 0); | ||
| usb_phy_disable_dp_dm_pulldown(HPM_USB0); | ||
| clock_remove_from_group(clock_usb0, 0); | ||
| while (sysctl_resource_target_is_busy(HPM_SYSCTL, sysctl_resource_usb0)) { | ||
| ; | ||
| } | ||
| sysctl_resource_target_set_mode(HPM_SYSCTL, sysctl_resource_xtal, tmp); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout protection to busy-wait loops.
The busy-wait loops could potentially hang the system if hardware doesn't respond as expected.
Consider adding timeout protection:
while (sysctl_resource_any_is_busy(HPM_SYSCTL)) {
- ;
+ /* Add timeout counter to prevent infinite loop */
+ static uint32_t timeout = 10000;
+ if (--timeout == 0) break;
}Apply similar timeout protection to the second busy-wait loop at lines 193-195.
📝 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 board_init_usb_dp_dm_pins(void) | |
| { | |
| /* Disconnect usb dp/dm pins pull down 45ohm resistance */ | |
| while (sysctl_resource_any_is_busy(HPM_SYSCTL)) { | |
| ; | |
| } | |
| if (pllctlv2_xtal_is_stable(HPM_PLLCTLV2) && pllctlv2_xtal_is_enabled(HPM_PLLCTLV2)) { | |
| if (clock_check_in_group(clock_usb0, 0)) { | |
| usb_phy_disable_dp_dm_pulldown(HPM_USB0); | |
| } else { | |
| clock_add_to_group(clock_usb0, 0); | |
| usb_phy_disable_dp_dm_pulldown(HPM_USB0); | |
| clock_remove_from_group(clock_usb0, 0); | |
| } | |
| } else { | |
| uint8_t tmp; | |
| tmp = sysctl_resource_target_get_mode(HPM_SYSCTL, sysctl_resource_xtal); | |
| sysctl_resource_target_set_mode(HPM_SYSCTL, sysctl_resource_xtal, 0x03); /* NOLINT */ | |
| clock_add_to_group(clock_usb0, 0); | |
| usb_phy_disable_dp_dm_pulldown(HPM_USB0); | |
| clock_remove_from_group(clock_usb0, 0); | |
| while (sysctl_resource_target_is_busy(HPM_SYSCTL, sysctl_resource_usb0)) { | |
| ; | |
| } | |
| sysctl_resource_target_set_mode(HPM_SYSCTL, sysctl_resource_xtal, tmp); | |
| } | |
| } | |
| void board_init_usb_dp_dm_pins(void) | |
| { | |
| /* Disconnect usb dp/dm pins pull down 45ohm resistance */ | |
| while (sysctl_resource_any_is_busy(HPM_SYSCTL)) { | |
| /* Add timeout counter to prevent infinite loop */ | |
| static uint32_t timeout = 10000; | |
| if (--timeout == 0) { | |
| break; | |
| } | |
| } | |
| if (pllctlv2_xtal_is_stable(HPM_PLLCTLV2) && pllctlv2_xtal_is_enabled(HPM_PLLCTLV2)) { | |
| if (clock_check_in_group(clock_usb0, 0)) { | |
| usb_phy_disable_dp_dm_pulldown(HPM_USB0); | |
| } else { | |
| clock_add_to_group(clock_usb0, 0); | |
| usb_phy_disable_dp_dm_pulldown(HPM_USB0); | |
| clock_remove_from_group(clock_usb0, 0); | |
| } | |
| } else { | |
| uint8_t tmp; | |
| tmp = sysctl_resource_target_get_mode(HPM_SYSCTL, sysctl_resource_xtal); | |
| sysctl_resource_target_set_mode(HPM_SYSCTL, sysctl_resource_xtal, 0x03); /* NOLINT */ | |
| clock_add_to_group(clock_usb0, 0); | |
| usb_phy_disable_dp_dm_pulldown(HPM_USB0); | |
| clock_remove_from_group(clock_usb0, 0); | |
| while (sysctl_resource_target_is_busy(HPM_SYSCTL, sysctl_resource_usb0)) { | |
| /* Add timeout counter to prevent infinite loop */ | |
| static uint32_t timeout = 10000; | |
| if (--timeout == 0) { | |
| break; | |
| } | |
| } | |
| sysctl_resource_target_set_mode(HPM_SYSCTL, sysctl_resource_xtal, tmp); | |
| } | |
| } |
🤖 Prompt for AI Agents
In projects/HSLink-OB/board/hslinkob/board.c around lines 171 to 198, the
busy-wait loops lack timeout protection, risking indefinite hangs if hardware is
unresponsive. Add a timeout counter to each while loop that checks
sysctl_resource_any_is_busy and sysctl_resource_target_is_busy, breaking the
loop and handling the timeout if the counter exceeds a safe threshold. This
ensures the system won't hang indefinitely waiting for hardware status changes.
| uint64_t millis() | ||
| { | ||
| uint64_t mchtmr_count = mchtmr_get_count(HPM_MCHTMR); | ||
| return (uint64_t)(mchtmr_count * 1000 / MCHTMR_CLK_FREQ); | ||
| } |
There was a problem hiding this comment.
Potential integer overflow and division by zero risks.
The calculation mchtmr_count * 1000 / MCHTMR_CLK_FREQ could overflow for large timer counts, and there's no check for zero frequency.
Consider this safer implementation:
uint64_t millis()
{
+ if (MCHTMR_CLK_FREQ == 0) {
+ return 0;
+ }
uint64_t mchtmr_count = mchtmr_get_count(HPM_MCHTMR);
- return (uint64_t)(mchtmr_count * 1000 / MCHTMR_CLK_FREQ);
+ return mchtmr_count / (MCHTMR_CLK_FREQ / 1000);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In projects/HSLink-OB/board/hslinkob/board.c around lines 165 to 169, the
function millis() risks integer overflow when multiplying mchtmr_count by 1000
and does not check if MCHTMR_CLK_FREQ is zero, which could cause division by
zero. To fix this, first add a check to return 0 or an error if MCHTMR_CLK_FREQ
is zero. Then, to avoid overflow, perform the division before multiplication if
possible or use a wider integer type for intermediate calculations to safely
compute the elapsed milliseconds.
This reverts commit 672f854.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
projects/hpm5301evklite/dp_common.c (2)
33-33: Replace Chinese comments with English for consistency.The comments "判断是否需要切换模式" should be in English to maintain consistency with the rest of the codebase.
Apply this diff to translate the comments:
- // 判断是否需要切换模式 + // Check if mode switching is needed- // 判断是否需要切换模式 + // Check if mode switching is neededAlso applies to: 48-48
63-78: Simplify the clock divider calculation logic.The div variable is set to 10 multiple times (lines 65, 67, 71) which appears redundant. The logic could be simplified.
Consider refactoring to eliminate redundant assignments:
sclk_div = ((SPI_MAX_SRC_CLOCK / sclk_freq_in_hz) / 2) - 1; /* SCLK = SPI_SRC_CLOK / ((SCLK_DIV + 1) * 2)*/ + div = 10; if (sclk_div <= 0xFE) { - div = 10; + // Use default div and clock source } else { - div = 10; src_clock = clk_src_pll0_clk1; /* 600M */ sclk_div = ((SPI_MID_SRC_CLOCK / sclk_freq_in_hz) / 2) - 1; /* SCLK = SPI_SRC_CLOK / ((SCLK_DIV + 1) * 2)*/ if (sclk_div >= 0xFE) { - div = 10; src_clock = clk_src_pll1_clk2; /* 500M */ sclk_div = ((SPI_MIN_SRC_CLOCK / sclk_freq_in_hz) / 2) - 1; if (sclk_div >= 0xFE) { sclk_div = 0xFE; /* The minimum sclk clock allowed is 98KHz */ } } }projects/hpm5301evklite/SW_DP_SPI.c (1)
55-55: Remove commented debug statements.Consider removing the commented
printfstatements to keep the code clean.- //printf("SWJ_Sequence %d\n", count); - //printf("SWD_Sequence\n");Also applies to: 98-98
projects/hpm5301evklite/JTAG_DP_SPI.c (2)
42-42: Consider making USE_GPIO_1_BIT configurable via build system.The
USE_GPIO_1_BITmacro is hardcoded to 0. Consider making it configurable through CMake or build definitions to allow runtime or build-time selection of GPIO vs SPI mode.-#define USE_GPIO_1_BIT 0 +#ifndef USE_GPIO_1_BIT +#define USE_GPIO_1_BIT 0 +#endifAlso applies to: 89-98
392-392: Add spaces after comment markers for consistency.- /* Capture-IR and Shift-IR */ + /* Capture-IR and Shift-IR */ - /* Capture-DR & Shift-DR */ + /* Capture-DR & Shift-DR */ - /* Get D31 */ + /* Get D31 */ - /* Write Transfer */ + /* Write Transfer */ - /* Update-DR */ + /* Update-DR */ - /* Idle cycles */ + /* Idle cycles */ - /* Capture-DR & Shift-DR */ + /* Capture-DR & Shift-DR */ - /* Update-DR */ + /* Update-DR */ - /* Capture-DR & Shift-DR */ + /* Capture-DR & Shift-DR */ - /* Bypass after data */ + /* Bypass after data */ - /* Update-DR */ + /* Update-DR */Also applies to: 439-439, 492-492, 519-519, 549-549, 562-562, 577-577, 613-613, 632-632, 661-661, 680-680
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
projects/hpm5301evklite/bin/hpm5301_daplink.binis excluded by!**/*.binprojects/hpm5301evklite/bin/spi_over_swd.binis excluded by!**/*.bin
📒 Files selected for processing (31)
projects/HSLink-OB/.version(0 hunks)projects/HSLink-OB/CMakeLists.txt(0 hunks)projects/HSLink-OB/JTAG_DP/JTAG_DP.c(0 hunks)projects/HSLink-OB/JTAG_DP/JTAG_DP.h(0 hunks)projects/HSLink-OB/JTAG_DP/JTAG_DP_IO.c(0 hunks)projects/HSLink-OB/SW_DP/SW_DP.c(0 hunks)projects/HSLink-OB/SW_DP/SW_DP.h(0 hunks)projects/HSLink-OB/SW_DP/SW_DP_IO.c(0 hunks)projects/HSLink-OB/USB2UART/usb2uart.c(0 hunks)projects/HSLink-OB/USB2UART/usb2uart.h(0 hunks)projects/HSLink-OB/dp_common.c(0 hunks)projects/HSLink-OB/dp_common.h(0 hunks)projects/HSLink-OB/flash_uf2.ld(0 hunks)projects/HSLink-OB/main.c(0 hunks)projects/HSLink-OB/scripts/uf2conv.py(0 hunks)projects/HSLink-OB/setting.h(0 hunks)projects/HSLink-OB/swd_host/debug_cm.h(0 hunks)projects/HSLink-OB/swd_host/swd_host.c(0 hunks)projects/HSLink-OB/swd_host/swd_host.h(0 hunks)projects/HSLink-OB/usb_config.h(0 hunks)projects/hpm5301evklite/.gitignore(1 hunks)projects/hpm5301evklite/CMakeLists.txt(1 hunks)projects/hpm5301evklite/DAP_config.h(17 hunks)projects/hpm5301evklite/JTAG_DP_SPI.c(19 hunks)projects/hpm5301evklite/SW_DP_SPI.c(8 hunks)projects/hpm5301evklite/app.yaml(1 hunks)projects/hpm5301evklite/bin/请看说明.txt(1 hunks)projects/hpm5301evklite/dp_common.c(1 hunks)projects/hpm5301evklite/main.c(1 hunks)projects/hpm5301evklite/usb2uart.c(1 hunks)projects/hpm5301evklite/usb_config.h(1 hunks)
💤 Files with no reviewable changes (20)
- projects/HSLink-OB/.version
- projects/HSLink-OB/USB2UART/usb2uart.h
- projects/HSLink-OB/dp_common.h
- projects/HSLink-OB/JTAG_DP/JTAG_DP.c
- projects/HSLink-OB/main.c
- projects/HSLink-OB/dp_common.c
- projects/HSLink-OB/SW_DP/SW_DP.c
- projects/HSLink-OB/JTAG_DP/JTAG_DP.h
- projects/HSLink-OB/usb_config.h
- projects/HSLink-OB/scripts/uf2conv.py
- projects/HSLink-OB/SW_DP/SW_DP.h
- projects/HSLink-OB/setting.h
- projects/HSLink-OB/swd_host/debug_cm.h
- projects/HSLink-OB/flash_uf2.ld
- projects/HSLink-OB/JTAG_DP/JTAG_DP_IO.c
- projects/HSLink-OB/CMakeLists.txt
- projects/HSLink-OB/USB2UART/usb2uart.c
- projects/HSLink-OB/swd_host/swd_host.c
- projects/HSLink-OB/swd_host/swd_host.h
- projects/HSLink-OB/SW_DP/SW_DP_IO.c
✅ Files skipped from review due to trivial changes (3)
- projects/hpm5301evklite/app.yaml
- projects/hpm5301evklite/bin/请看说明.txt
- projects/hpm5301evklite/.gitignore
🧰 Additional context used
🧬 Code Graph Analysis (5)
projects/hpm5301evklite/main.c (1)
projects/hpm5301evklite/usb2uart.c (1)
uartx_preinit(82-93)
projects/hpm5301evklite/dp_common.c (5)
projects/hpm5301evklite/SW_DP_SPI.c (1)
PORT_SWD_SETUP(37-43)projects/hpm5301evklite/DAP_config.h (4)
PORT_SWD_SETUP(391-391)PORT_SWD_SETUP(416-416)PORT_JTAG_SETUP(349-349)PORT_JTAG_SETUP(382-382)projects/hpm5301evklite/JTAG_DP_SPI.c (1)
PORT_JTAG_SETUP(73-100)projects/HSLink-Pro/src/dp_common.c (1)
Set_Clock_Delay(123-131)DAP/Include/DAP.h (1)
Set_Clock_Delay(327-327)
projects/hpm5301evklite/usb2uart.c (1)
dap_main.c (1)
chry_dap_usb2uart_uart_send_complete(670-682)
projects/hpm5301evklite/JTAG_DP_SPI.c (2)
projects/hpm5301evklite/DAP_config.h (3)
PORT_JTAG_SETUP(349-349)PORT_JTAG_SETUP(382-382)gpiom_configure_pin_control_setting(341-341)DAP/Include/DAP.h (5)
JTAG_Sequence(280-280)JTAG_ReadIDCode(282-282)JTAG_WriteAbort(283-283)JTAG_IR(281-281)JTAG_Transfer(284-284)
projects/hpm5301evklite/SW_DP_SPI.c (3)
projects/HSLink-Pro/src/DAP_config.h (3)
PORT_SWD_SETUP(377-377)TIMESTAMP_GET(719-723)TIMESTAMP_GET(719-719)projects/HSLink-Pro/src/SW_DP/SW_DP.c (4)
PORT_SWD_SETUP(5-12)SWJ_Sequence(14-42)SWD_Sequence(44-51)SWD_Transfer(53-60)DAP/Include/DAP.h (3)
SWJ_Sequence(278-278)SWD_Sequence(279-279)SWD_Transfer(285-285)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (10)
projects/hpm5301evklite/usb_config.h (1)
1-166: USB configuration looks comprehensive and well-structured.The configuration parameters for both USB device and host stacks are reasonable, with appropriate buffer sizes, timeout values, and class-specific settings. The non-cacheable RAM attribute for DMA operations is correctly defined.
projects/hpm5301evklite/main.c (1)
16-19: Consider the impact of the tight loop on CPU usage.The main loop continuously polls without any delay or yield mechanism. While this ensures low latency, it may result in high CPU usage. Consider if this is the intended behavior for your use case.
projects/hpm5301evklite/CMakeLists.txt (1)
1-83: CMake configuration is well-organized and comprehensive.The build configuration properly handles conditional compilation for GPIO vs SPI modes, includes all necessary source files, and appropriately uses O3 optimization for embedded performance.
projects/hpm5301evklite/SW_DP_SPI.c (3)
37-43: Function renaming aligns with DAP interface.The renaming from
SPI_PORT_SWD_SETUPtoPORT_SWD_SETUPis consistent with the DAP interface declarations and allows for proper integration with the wrapper functions.
90-145: SWD sequence implementation looks correct.The function properly handles both read and write operations with appropriate SPI transfer modes and data bit configurations.
186-186: ACK width configuration is correct for the SPI‐based SWD driver.The 4-bit and 5-bit reads account for the required turnaround cycles plus the 3-bit ACK field:
- Read transfers use 1 turnaround bit + 3 ACK bits = 4 bits
- Write transfers use 1 turnaround + 3 ACK bits + 1 turnaround = 5 bits
The subsequent
ack >>= 1; ack &= 0x07;shifts out the first turnaround and masks to the 3-bit ACK. This matches the pattern used in other SWD SPI drivers (e.g.SW_DP/SW_DP_SPI.c).projects/hpm5301evklite/JTAG_DP_SPI.c (3)
107-181: JTAG interface functions properly implemented.The renamed functions correctly match the DAP interface declarations and appropriately delegate to internal implementation functions.
556-559: Timestamp capture is commented out.The timestamp capture using
TIMESTAMP_GET()is commented out, unlike the SWD implementation. Is this intentional or should it be enabled for JTAG transfers as well?
299-301: Add break statement to default case.The empty default case could cause unintended fall-through behavior.
default: + break; - break;Likely an incorrect or invalid review comment.
projects/hpm5301evklite/DAP_config.h (1)
168-189: CMSIS-DAP string functions are disabled – consider plug-and-play detection.Although omitting the vendor, product, and serial strings won’t break the core CMSIS-DAP protocol, debugger tools like Keil MDK or OpenOCD rely on those USB descriptors for automatic device recognition. Without them, users must manually configure the debugger to match your VID/PID.
• If you want seamless host detection, implement these functions in projects/hpm5301evklite/DAP_config.h (lines 168–189 and 263–266):
- DAP_GetVendorString
- DAP_GetProductString
- DAP_GetSerNumString
• Otherwise, document the need for manual debugger setup or confirm that manual configuration is acceptable in your use case.
| { | ||
| uint32_t buf_addr; | ||
| dma_resource_t *tx_resource = &dma_resource_pools[UART_TX_DMA_RESOURCE_INDEX]; | ||
| if (len <= 0) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary comparison with zero for unsigned type.
The parameter len is of type uint16_t (unsigned), so it cannot be negative. The comparison should only check for zero.
Apply this diff:
- if (len <= 0) {
+ if (len == 0) {📝 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.
| if (len <= 0) { | |
| if (len == 0) { |
🤖 Prompt for AI Agents
In projects/hpm5301evklite/usb2uart.c at line 122, the condition checks if len
is less than or equal to zero, but len is an unsigned uint16_t and cannot be
negative. Modify the condition to check only if len is equal to zero to remove
the unnecessary comparison.
| for (i = 0; i < rx_desc_size; i++) { | ||
| if (link_addr == core_local_mem_to_sys_address(BOARD_RUNNING_CORE, (uint32_t)&rx_descriptors[i])) { | ||
| chry_ringbuffer_write(&g_uartrx, uart_rx_buf[i - 1], UART_RX_DMA_BUFFER_SIZE); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix potential buffer underflow in DMA callback.
When i == 0, accessing uart_rx_buf[i - 1] will cause a buffer underflow.
Apply this diff to fix the array access:
for (i = 0; i < rx_desc_size; i++) {
if (link_addr == core_local_mem_to_sys_address(BOARD_RUNNING_CORE, (uint32_t)&rx_descriptors[i])) {
- chry_ringbuffer_write(&g_uartrx, uart_rx_buf[i - 1], UART_RX_DMA_BUFFER_SIZE);
+ chry_ringbuffer_write(&g_uartrx, uart_rx_buf[i], UART_RX_DMA_BUFFER_SIZE);
}
}📝 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.
| for (i = 0; i < rx_desc_size; i++) { | |
| if (link_addr == core_local_mem_to_sys_address(BOARD_RUNNING_CORE, (uint32_t)&rx_descriptors[i])) { | |
| chry_ringbuffer_write(&g_uartrx, uart_rx_buf[i - 1], UART_RX_DMA_BUFFER_SIZE); | |
| } | |
| } | |
| for (i = 0; i < rx_desc_size; i++) { | |
| if (link_addr == core_local_mem_to_sys_address(BOARD_RUNNING_CORE, (uint32_t)&rx_descriptors[i])) { | |
| chry_ringbuffer_write(&g_uartrx, uart_rx_buf[i], UART_RX_DMA_BUFFER_SIZE); | |
| } | |
| } |
🤖 Prompt for AI Agents
In projects/hpm5301evklite/usb2uart.c around lines 45 to 49, the code accesses
uart_rx_buf[i - 1] inside a loop starting at i = 0, which causes a buffer
underflow when i is zero. To fix this, adjust the index to avoid subtracting 1
from i, ensuring the array access stays within valid bounds, for example by
using uart_rx_buf[i] instead of uart_rx_buf[i - 1].
| printf("linker_addr:0x%08x %d\n", (uint32_t)&rx_descriptors[i], i); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or conditionally compile debug print statements.
Debug printf statements should not be in production code as they can impact performance and clutter the output.
Apply this diff to conditionally compile the debug prints:
}
+#ifdef DEBUG
rx_descriptors[i].ctrl &= ~DMA_MGR_INTERRUPT_MASK_TC;
printf("linker_addr:0x%08x %d\n", (uint32_t)&rx_descriptors[i], i);
+#endif
}📝 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.
| printf("linker_addr:0x%08x %d\n", (uint32_t)&rx_descriptors[i], i); | |
| } | |
| } | |
| #ifdef DEBUG | |
| rx_descriptors[i].ctrl &= ~DMA_MGR_INTERRUPT_MASK_TC; | |
| printf("linker_addr:0x%08x %d\n", (uint32_t)&rx_descriptors[i], i); | |
| #endif | |
| } |
🤖 Prompt for AI Agents
In projects/hpm5301evklite/usb2uart.c around lines 178 to 179, the debug printf
statement should be removed or wrapped in a conditional compilation block to
prevent it from running in production. Use preprocessor directives like #ifdef
DEBUG and #endif around the printf to ensure it only compiles when debugging is
enabled, improving performance and keeping output clean in production builds.
| HPM_IOC->PAD[PIN_JTAG_TRST].FUNC_CTL = IOC_PAD_FUNC_CTL_ALT_SELECT_SET(0); | ||
| HPM_IOC->PAD[PIN_SRST].FUNC_CTL = IOC_PAD_FUNC_CTL_ALT_SELECT_SET(0); | ||
| HPM_IOC->PAD[PIN_LED_RUNNING].FUNC_CTL = IOC_PAD_FUNC_CTL_ALT_SELECT_SET(0); |
There was a problem hiding this comment.
Undefined pin references in PORT_JTAG_SETUP.
The inline PORT_JTAG_SETUP function references undefined pins: PIN_JTAG_TRST, PIN_SRST, PIN_LED_RUNNING, and PIN_LED_CONNECTED. These pins are not defined in this file.
Either define these pins or remove the references if they're not used in this configuration.
🤖 Prompt for AI Agents
In projects/hpm5301evklite/DAP_config.h around lines 354 to 356, the function
PORT_JTAG_SETUP references undefined pins PIN_JTAG_TRST, PIN_SRST,
PIN_LED_RUNNING, and PIN_LED_CONNECTED. To fix this, either add proper
definitions for these pins in the file or remove their references from the
PORT_JTAG_SETUP function if they are not needed in this configuration.
| } | ||
| //LED_SetConnectMode(bit & 0x01); | ||
| else { | ||
| gpio_write_pin(PIN_GPIO, GPIO_GET_PORT_INDEX(PIN_LED_CONNECTED), GPIO_GET_PIN_INDEX(PIN_LED_RUNNING), false); |
There was a problem hiding this comment.
Bug: Incorrect pin index in LED_CONNECTED_OUT (OFF case).
Same issue in the OFF case - uses wrong pin index.
- gpio_write_pin(PIN_GPIO, GPIO_GET_PORT_INDEX(PIN_LED_CONNECTED), GPIO_GET_PIN_INDEX(PIN_LED_RUNNING), false);
+ gpio_write_pin(PIN_GPIO, GPIO_GET_PORT_INDEX(PIN_LED_CONNECTED), GPIO_GET_PIN_INDEX(PIN_LED_CONNECTED), false);📝 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.
| gpio_write_pin(PIN_GPIO, GPIO_GET_PORT_INDEX(PIN_LED_CONNECTED), GPIO_GET_PIN_INDEX(PIN_LED_RUNNING), false); | |
| gpio_write_pin(PIN_GPIO, GPIO_GET_PORT_INDEX(PIN_LED_CONNECTED), GPIO_GET_PIN_INDEX(PIN_LED_CONNECTED), false); |
🤖 Prompt for AI Agents
In projects/hpm5301evklite/DAP_config.h at line 684, the gpio_write_pin call for
turning off the LED_CONNECTED_OUT uses the wrong pin index. Correct this by
replacing GPIO_GET_PIN_INDEX(PIN_LED_RUNNING) with
GPIO_GET_PIN_INDEX(PIN_LED_CONNECTED) to ensure the correct pin is targeted when
turning off the LED.
| return; | ||
| __STATIC_INLINE void LED_CONNECTED_OUT (uint32_t bit) { | ||
| if(bit & 0x01) { | ||
| gpio_write_pin(PIN_GPIO, GPIO_GET_PORT_INDEX(PIN_LED_CONNECTED), GPIO_GET_PIN_INDEX(PIN_LED_RUNNING), true); |
There was a problem hiding this comment.
Bug: Incorrect pin index in LED_CONNECTED_OUT.
The function uses PIN_LED_RUNNING index with PIN_LED_CONNECTED port, which is incorrect.
- gpio_write_pin(PIN_GPIO, GPIO_GET_PORT_INDEX(PIN_LED_CONNECTED), GPIO_GET_PIN_INDEX(PIN_LED_RUNNING), true);
+ gpio_write_pin(PIN_GPIO, GPIO_GET_PORT_INDEX(PIN_LED_CONNECTED), GPIO_GET_PIN_INDEX(PIN_LED_CONNECTED), true);📝 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.
| gpio_write_pin(PIN_GPIO, GPIO_GET_PORT_INDEX(PIN_LED_CONNECTED), GPIO_GET_PIN_INDEX(PIN_LED_RUNNING), true); | |
| gpio_write_pin(PIN_GPIO, GPIO_GET_PORT_INDEX(PIN_LED_CONNECTED), GPIO_GET_PIN_INDEX(PIN_LED_CONNECTED), true); |
🤖 Prompt for AI Agents
In projects/hpm5301evklite/DAP_config.h at line 681, the gpio_write_pin function
incorrectly uses the pin index of PIN_LED_RUNNING with the port of
PIN_LED_CONNECTED. To fix this, replace GPIO_GET_PIN_INDEX(PIN_LED_RUNNING) with
GPIO_GET_PIN_INDEX(PIN_LED_CONNECTED) so that both the port and pin index
correspond to PIN_LED_CONNECTED.
Summary by CodeRabbit
New Features
Refactor
Chores
Bug Fixes