sched/wdog: merge wdog_periods_s into wdog_s

[anchao]

Summary

sched/wdog: merge wdog_periods_s into wdog_s

Make wdog support period semantics by default, avoid exposing two structures to enhance consistency

Signed-off-by: chao an [email protected]

Impact

N/A

Testing

ostest

[nuttxpr]

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary of the change, it lacks crucial details.

Here's a breakdown of what's missing:

• Insufficient Summary: While the summary describes what is being changed, it fails to adequately explain why. What problem does merging these structures solve? What are the benefits (e.g., reduced memory usage, simplified API, improved performance)? How does it "enhance consistency"?
• Missing Impact Assessment: "N/A" is not acceptable. Even seemingly small changes can have unexpected impacts. The author needs to carefully consider and document the impact on each area (user, build, hardware, documentation, security, compatibility). For example, does this change affect any existing drivers or applications that use the old structure? Does it change the size of the watchdog structure, potentially impacting memory usage on resource-constrained devices?
• Inadequate Testing: "ostest" is too vague. What specific tests within ostest were run? What were the expected results? The provided log sections are empty, making it impossible to verify the change's effectiveness. The author should include relevant log snippets demonstrating the watchdog functionality before and after the change, highlighting the improvement or fix. Additionally, the build host and target details are missing.

In short, the PR needs more detail and evidence to demonstrate that the change is beneficial, safe, and well-tested.

[xiaoxiang781216]

but the chagne will increase wdog_s size even the caller doesn't require the period callback.

[anchao]

how about add a kconfig setting to make period wdog feature configurable?

[xiaoxiang781216]

doesn't resolve the problem, since not all user in the same image use the periodic watchdog.

[cederom]

Thanks @anchao :-) Could you please write some more details why this change is necessary and what it solves / fixes / improves?

You often introduce changes in critical parts of the RTOS but with not much explanation why. Single sentence on what you change is not enough, we can see that in the code, but we also need to know WHY. It is important to explain the motivation behind the change to other people and that would help in review.

You say "Make wdog support period semantics by default, avoid exposing two structures to enhance consistency". Consistency with what? Implementations on other OS/RTOS? Removes code that is not always necessary / used to decrease firmware size?

Current split design looks on purpose? Have you checked what targets will be affected by this change? If so where is the check presented?

What if some people use that code as it is now.. it will be broken for them.. and it would be nice to know WHY.

[anchao]

Thanks @anchao :-) Could you please write some more details why this change is necessary and what it solves / fixes / improves?

Just merge wdog_periods_s into wdog_s , individual developers don't need to learn whether to use wdog_periods_s or wdog_s

You often introduce changes in critical parts of the RTOS but with not much explanation why. Single sentence on what you change is not enough, we can see that in the code, but we also need to know WHY. It is important to explain the motivation behind the change to other people and that would help in review.

original author of this feature is not me, I carried out some refined tweaks of the code.

You say "Make wdog support period semantics by default, avoid exposing two structures to enhance consistency". Consistency with what? Implementations on other OS/RTOS? Removes code that is not always necessary / used to decrease firmware size?

you should raise the question to #15937

Current split design looks on purpose? Have you checked what targets will be affected by this change? If so where is the check presented?

ditto

What if some people use that code as it is now.. it will be broken for them.. and it would be nice to know WHY.

this feature was merged just 2 days ago, why don't you raise these questions before #15937 merged?

#15937

[cederom]

Aaah, fresh stuff, thanks @anchao, one more simple sentence more in the description would make things clear from start :-) Well I sit usually ~16h in front of the computer but not always sorry :D

Build time selectable option seems reasonable but @xiaoxiang781216 knows the details so I will remain silent :-P Good luck guys :-)

[anchao]

Aaah, fresh stuff, thanks @anchao, one more simple sentence more in the description would make things clear from start :-) Well I sit usually ~16h in front of the computer but not always sorry :D

Build time selectable option seems reasonable but @xiaoxiang781216 knows the details so I will remain silent :-P Good luck guys :-)

I think you should do more exercise and not sit in front of the computer for a long time, it's good for your health

[acassis]

@anchao please update the log message to include: why this feature is needed and its advantages

[anchao]

@anchao please update the log message to include: why this feature is needed and its advantages

@acassis

new API is not compatible with the old structure. The following is a comparison before and after the PR change:

1. feature in sched/wdog: Support for periodic wdog. #15937

struct wdog_period_s wdog;

ret = wd_start(&wdog, ...);        <-- failure, compiler warning
ret = wd_start_period(&wdog, ...); 

struct wdog_s wdog;

ret = wd_start(&wdog, ...);
ret = wd_start_period(&wdog, ...); <-- failure, compiler warning and access out of bounds

2. after this PR, we merge struct wdog_period_s to struct wdog_s, so that developers do not need to pay special attention to whether to use wdog_period_s or wdog_s, But the cost is that all wdog_s need to add 8-16（64bits system or time64）bytes

struct wdog_s wdog;

ret = wd_start(&wdog, ...);        <-- success
ret = wd_start_period(&wdog, ...); <-- success

Here is a decision for yours. If prefer proposal 1, I will close this PR. otherwise please approve this PR.

[xiaoxiang781216]

@anchao the initial design is using the unified wdog_s, but it increase 8 bytes for each wdog_s even the caller doesn't use period timer, so the final decision is to used the seperated structure.

[anchao]

@anchao the initial design is using the unified wdog_s, but it increase 8 bytes for each wdog_s even the caller doesn't use period timer, so the final decision is to used the seperated structure.

8 bytes can enhance consistency and avoid software bugs. I think the benefits are greater.

[xiaoxiang781216]

@anchao the initial design is using the unified wdog_s, but it increase 8 bytes for each wdog_s even the caller doesn't use period timer, so the final decision is to used the seperated structure.

8 bytes can enhance consistency and

Sorry, I don't think the new function use new struct isn't consistency.

< avoid software bugs. I think the benefits are greater.

what's software bug?

[anchao]

Sorry, I don't think the new function use new struct isn't consistency.

If I only have one wdog_s instance, and I want to support both periodic and non-periodic scenarios, how should I implement this? Using 1 wdog_s and 1 wdog_periods_s instances?

< avoid software bugs. I think the benefits are greater.

what's software bug?

see issue 1

#15959 (comment)

[anchao]

@xiaoxiang781216 So you expect developers to write code like this?

struct wdog_periods_s pwdog;
struct wdog_s wdog;
int ret;

if (...)
  ret = wd_start(&wdog, ...);
else
  ret = wd_start_period(&pwdog, ...);

[xiaoxiang781216]

@xiaoxiang781216 So you expect developers to write code like this?

struct wdog_periods_s pwdog;
struct wdog_s wdog;
int ret;

if (...)
  ret = wd_start(&wdog, ...);
else
  ret = wd_start_period(&pwdog, ...);

in this case, you can always use wdog_periods_s with period == 0 for no period case

[anchao]

@xiaoxiang781216 So you expect developers to write code like this?

struct wdog_periods_s pwdog;

struct wdog_s wdog;

int ret;

if (...)

ret = wd_start(&wdog, ...);

else

ret = wd_start_period(&pwdog, ...);

in this case, you can always use wdog_periods_s with period == 0 for no period case

So, to avoid similar problems, we must use wdog_period_s, right? could we remove wdog_s and only keep wdog_period_s? I don't think wdog_s needs to exist anymore

[anchao]

To be honest, interfaces and data types are extremely important concepts. They interact directly with developers. If you want to reduce the size, you should optimize the internal implementation instead of exposing these APIs that are prone to problems during the development process.