fix:RNDIS DHCP dependency,and delay implementation #376
Conversation
Rbb666
changed the title
WalkthroughTwo files modified to introduce conditional compilation logic: DHCP initialization in RNDIS CDC template now branches between DHCPD server mode and netdev-based DHCP, and USB Kinetis delay function conditionally uses RT-Thread sleep or busy-wait depending on thread support availability. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![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)
port/kinetis/usb_glue_mcx.c (1)
83-83: Parameter type limits maximum delay to 255ms.The
uint8_tparameter restricts the delay to a maximum of 255 milliseconds. If longer delays might be needed in the future, consider usinguint32_tfor the parameter type.demo/cdc_rndis_template.c (1)
192-199: Make netdev.h include conditional to match usage.The
netdev.hheader is only used whenLWIP_USING_DHCPDis not defined (lines 256-259), but it's included unconditionally. Consider making it conditional to avoid unnecessary dependencies.Apply this diff:
#ifdef LWIP_USING_DHCPD #include <dhcp_server.h> +#else +#include <netdev.h> #endif #include <rtthread.h> #include <rtdevice.h> #include <netif/ethernetif.h> -#include <netdev.h>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
demo/cdc_rndis_template.c(2 hunks)port/kinetis/usb_glue_mcx.c(1 hunks)
🔇 Additional comments (1)
demo/cdc_rndis_template.c (1)
253-260: Consider adding error handling and logging for DHCP initialization failures.Both DHCP initialization paths lack error handling:
dhcpd_start("u0")(line 254) return value is not checked- When
netdev_get_by_namereturns NULL (lines 256-259), the failure is silentSilent failures could lead to connectivity issues that are difficult to debug in production.
Consider adding error logging:
#ifdef LWIP_USING_DHCPD - dhcpd_start("u0"); + if (dhcpd_start("u0") != 0) { + /* Log error or handle failure */ + } #else struct netdev *netdev = netdev_get_by_name("u0"); if (netdev) { netdev_dhcp_enabled(netdev, RT_TRUE); + } else { + /* Log error: network device not found */ } #endifWould you like me to generate a script to verify if error handling patterns exist elsewhere in the codebase?
| #ifdef __RTTHREAD__ | ||
| rt_thread_mdelay(ms); | ||
| #else | ||
| for (uint32_t i = 0; i < ms; i++) | ||
| { | ||
| for (volatile uint32_t j = 0; j < 10000; j++); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Magic constant makes delay inaccurate across different MCU models.
The busy-wait calibration constant 10000 is CPU frequency dependent and will produce inaccurate delays across the supported Kinetis MCU models (MCXC444, MCXA153, MCXN947, MCXA156), which have different maximum clock frequencies (e.g., MCXN947 at 150MHz vs MCXA153 at 96MHz).
Consider using SystemCoreClock to calculate the iteration count dynamically:
#ifdef __RTTHREAD__
rt_thread_mdelay(ms);
#else
+ /* Approximate delay based on core clock frequency */
+ uint32_t cycles_per_ms = SystemCoreClock / 1000;
+ uint32_t iterations = cycles_per_ms / 10; /* Rough estimate accounting for loop overhead */
for (uint32_t i = 0; i < ms; i++)
{
- for (volatile uint32_t j = 0; j < 10000; j++);
+ for (volatile uint32_t j = 0; j < iterations; j++);
}
#endifAlternatively, if precise timing is not critical, document the expected CPU frequency for the current calibration value.
📝 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.
| #ifdef __RTTHREAD__ | |
| rt_thread_mdelay(ms); | |
| #else | |
| for (uint32_t i = 0; i < ms; i++) | |
| { | |
| for (volatile uint32_t j = 0; j < 10000; j++); | |
| } | |
| #endif | |
| #ifdef __RTTHREAD__ | |
| rt_thread_mdelay(ms); | |
| #else | |
| /* Approximate delay based on core clock frequency */ | |
| uint32_t cycles_per_ms = SystemCoreClock / 1000; | |
| uint32_t iterations = cycles_per_ms / 10; /* Rough estimate accounting for loop overhead */ | |
| for (uint32_t i = 0; i < ms; i++) | |
| { | |
| for (volatile uint32_t j = 0; j < iterations; j++); | |
| } | |
| #endif |
🤖 Prompt for AI Agents
In port/kinetis/usb_glue_mcx.c around lines 85-92 the busy-wait uses a magic
constant (10000) which is CPU-frequency dependent; replace the hardcoded
inner-loop count with a calculation derived from SystemCoreClock so the loop
produces ~1 ms per outer iteration (e.g., compute iterations = (SystemCoreClock
/ 1000) / estimated_cycles_per_loop and use that value instead of 10000), or
better yet call a platform delay API (rt_thread_mdelay or HAL_Delay / SysTick)
if available; if SystemCoreClock is not present, add a clear comment documenting
the CPU frequency assumption and provide a #define fallback calibration constant
and TODO to calibrate per-MCU.
2 participants
Sync:RT-Thread/rt-thread#10844
Summary by CodeRabbit