[soft_rtc]fix timespec/timeval error, optimize soft_rtc implementation #10874
Conversation
|
👋 感谢您对 RT-Thread 的贡献！Thank you for your contribution to RT-Thread! 为确保代码符合 RT-Thread 的编码规范，请在你的仓库中执行以下步骤运行代码格式化工作流（如果格式化CI运行失败）。 🛠 操作步骤 | Steps
完成后，提交将自动更新至 如有问题欢迎联系我们，再次感谢您的贡献！💐 |
|
|
📌 Code Review Assignment🏷️ Tag: componentsReviewers: @Maihuanyi Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2025-11-16 10:32 CST)
📝 Review Instructions
|
dongly
changed the title
|
@Maihuanyi 请审核 |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes timespec/timeval handling errors in the soft RTC implementation and optimizes the overall code structure. The main issue addressed is that RT_DEVICE_CTRL_RTC_GET_TIMEVAL and RT_DEVICE_CTRL_RTC_GET_TIMESPEC commands were producing incorrect results when soft RTC was enabled.
Key Changes:
- Unified time storage using a single
init_ts(timespec) baseline instead of separateinit_time,init_tv, and conditionalinit_tsvariables - Added new
get_rtc_time()function and reimplementedset_rtc_time()to work with timespec structures consistently - Simplified
soft_rtc_control()by removing platform-specific#ifdef RT_USING_KTIMEconditions and using the unified get/set functions - Added comprehensive documentation comments following doxygen format for all functions
- Introduced spinlock protection for thread-safe access to shared time variables
| ts->tv_sec = init_ktime_ts.tv_sec + (current_ts.tv_sec - init_ktime_ts.tv_sec); | ||
| ts->tv_nsec = init_ktime_ts.tv_nsec + (current_ts.tv_nsec - init_ktime_ts.tv_nsec); |
There was a problem hiding this comment.
Bug / 错误: Incorrect time calculation logic in get_rtc_time()
English: The calculation on line 147 is incorrect. It adds the difference (current_ts.tv_sec - init_ktime_ts.tv_sec) to init_ktime_ts.tv_sec, which effectively just results in current_ts.tv_sec. The correct calculation should add the elapsed time to init_ts.tv_sec (the RTC baseline time), not to init_ktime_ts.tv_sec (the system time when baseline was set).
中文: 第147行的计算逻辑有误。它将差值 (current_ts.tv_sec - init_ktime_ts.tv_sec) 加到 init_ktime_ts.tv_sec 上，这实际上只是得到 current_ts.tv_sec。正确的计算应该是将经过的时间加到 init_ts.tv_sec（RTC基准时间）上，而不是加到 init_ktime_ts.tv_sec（设置基准时的系统时间）上。
Correct implementation / 正确实现:
ts->tv_sec = init_ts.tv_sec + (current_ts.tv_sec - init_ktime_ts.tv_sec);
ts->tv_nsec = init_ts.tv_nsec + (current_ts.tv_nsec - init_ktime_ts.tv_nsec);| ts->tv_sec = init_ktime_ts.tv_sec + (current_ts.tv_sec - init_ktime_ts.tv_sec); | |
| ts->tv_nsec = init_ktime_ts.tv_nsec + (current_ts.tv_nsec - init_ktime_ts.tv_nsec); | |
| ts->tv_sec = init_ts.tv_sec + (current_ts.tv_sec - init_ktime_ts.tv_sec); | |
| ts->tv_nsec = init_ts.tv_nsec + (current_ts.tv_nsec - init_ktime_ts.tv_nsec); |
| init_tick = rt_tick_get(); | ||
| init_time = timegm(&time_new); | ||
| #endif | ||
| init_ts.tv_sec = timegm(&time_new); |
There was a problem hiding this comment.
Bug / 错误: Missing initialization of init_ts.tv_nsec
English: When initializing the RTC device, only init_ts.tv_sec is set on line 322, but init_ts.tv_nsec is never initialized. This could lead to unpredictable nanosecond values. The nanosecond field should be explicitly set to 0.
中文: 在初始化 RTC 设备时，第322行只设置了 init_ts.tv_sec，但从未初始化 init_ts.tv_nsec。这可能导致纳秒值不可预测。应该明确将纳秒字段设置为0。
Suggestion / 建议:
init_ts.tv_sec = timegm(&time_new);
init_ts.tv_nsec = 0;| init_ts.tv_sec = timegm(&time_new); | |
| init_ts.tv_sec = timegm(&time_new); | |
| init_ts.tv_nsec = 0; |
| #else | ||
| rt_tick_t tick = rt_tick_get_delta(init_tick); | ||
| ts->tv_sec = init_ts.tv_sec + tick / RT_TICK_PER_SECOND; | ||
| ts->tv_nsec = init_ts.tv_nsec + ((tick % RT_TICK_PER_SECOND) * (1000000000UL / RT_TICK_PER_SECOND)); |
There was a problem hiding this comment.
Bug / 错误: Potential overflow in nanosecond calculation
English: On line 152, the calculation (tick % RT_TICK_PER_SECOND) * (1000000000UL / RT_TICK_PER_SECOND) may lose precision due to integer division. For example, if RT_TICK_PER_SECOND is 1000, the expression 1000000000UL / RT_TICK_PER_SECOND equals 1000000, but if RT_TICK_PER_SECOND is not a divisor of 1000000000, precision will be lost. A more accurate approach would be to use ((tick % RT_TICK_PER_SECOND) * 1000000000UL) / RT_TICK_PER_SECOND.
中文: 第152行的计算 (tick % RT_TICK_PER_SECOND) * (1000000000UL / RT_TICK_PER_SECOND) 可能由于整数除法而损失精度。例如，如果 RT_TICK_PER_SECOND 是1000，表达式 1000000000UL / RT_TICK_PER_SECOND 等于1000000，但如果 RT_TICK_PER_SECOND 不是1000000000的除数，精度将会损失。更准确的方法是使用 ((tick % RT_TICK_PER_SECOND) * 1000000000UL) / RT_TICK_PER_SECOND。
Suggestion / 建议:
ts->tv_nsec = init_ts.tv_nsec + (((tick % RT_TICK_PER_SECOND) * 1000000000UL) / RT_TICK_PER_SECOND);| ts->tv_nsec = init_ts.tv_nsec + ((tick % RT_TICK_PER_SECOND) * (1000000000UL / RT_TICK_PER_SECOND)); | |
| ts->tv_nsec = init_ts.tv_nsec + (((tick % RT_TICK_PER_SECOND) * 1000000000UL) / RT_TICK_PER_SECOND); |
| /** | ||
| * @brief Alarm timeout callback function | ||
| * @param param Pointer to RTC device | ||
| * @return None | ||
| * | ||
| * This function is called when the alarm timer expires and updates | ||
| * the alarm system. | ||
| */ |
There was a problem hiding this comment.
[nitpick] Documentation / 文档问题: Incorrect parameter name in function documentation
English: The parameter documentation on line 60 says @param param Pointer to RTC device, but the actual parameter name in the function signature on line 67 is param. However, the description "Pointer to RTC device" is misleading because looking at the function call context, param is actually a void* that's passed to rt_alarm_update(), not necessarily a pointer to an RTC device. The documentation should be more accurate about what this parameter represents.
中文: 第60行的参数文档说 @param param Pointer to RTC device，虽然第67行的函数签名中参数名确实是 param，但描述"Pointer to RTC device"具有误导性，因为从函数调用上下文来看，param 实际上是传递给 rt_alarm_update() 的 void*，不一定是指向 RTC 设备的指针。文档应该更准确地描述此参数的含义。
Suggestion / 建议:
/**
* @brief Alarm timeout callback function
* @param param User data passed to alarm update function
* @return None
*
* This function is called when the alarm timer expires and updates
* the alarm system.
*/|
|
||
| rt_device_control(&soft_rtc_dev, RT_DEVICE_CTRL_RTC_GET_TIME, &time); | ||
| set_rtc_time(time); | ||
| struct timespec ts = { time, 0 }; |
There was a problem hiding this comment.
[nitpick] Maintainability / 可维护性: Redundant synchronization in rt_soft_rtc_sync()
English: The function rt_soft_rtc_sync() gets the current time and then immediately sets it back as the baseline. This effectively resets the soft RTC to the current time, which loses any accumulated drift. This seems counterintuitive for a "sync" function. The purpose appears to be updating alarm status (called in set_rtc_time()), but the logic could be clearer. Consider adding a comment explaining why getting and immediately setting the same time is the intended behavior.
中文: 函数 rt_soft_rtc_sync() 获取当前时间，然后立即将其设置回基准时间。这实际上将软 RTC 重置为当前时间，这会丢失任何累积的漂移。对于"同步"函数来说，这似乎违反直觉。其目的似乎是更新闹钟状态（在 set_rtc_time() 中调用），但逻辑可以更清晰。建议添加注释解释为什么获取并立即设置相同时间是预期行为。
Suggestion / 建议:
Add a comment explaining the purpose of this operation, or reconsider if this is the correct synchronization logic.
| struct timespec ts = { time, 0 }; | |
| struct timespec ts = { time, 0 }; | |
| /* | |
| * Intentionally reset the soft RTC baseline to the current time. | |
| * This operation updates alarm status and synchronizes the soft RTC | |
| * with the external time source, discarding any accumulated drift. | |
| * This is the intended behavior for synchronization in this context. | |
| */ |
|
@xqyjlj 一起帮忙review下呗 |
3 participants
拉取/合并请求描述：(PR description)
[
为什么提交这份PR (why to submit this PR)
启用 soft rtc 后, RT_DEVICE_CTRL_RTC_GET_TIMEVAL/RT_DEVICE_CTRL_RTC_GET_TIMESPEC 会出错
你的解决方案是什么 (what is your solution)
请提供验证的bsp和config (provide the config and bsp)
RT_USING_SOFT_RTC]
当前拉取/合并请求的状态 Intent for your PR
必须选择一项 Choose one (Mandatory):
代码质量 Code Quality：
我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:
#if 0代码，不包含已经被注释了的代码 All redundant code is removed and cleaned up