Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drivers/thermal: Fix hysteresis takes no effect error #15339

Merged
merged 7 commits into from
Dec 26, 2024

Conversation

JianyuWang0623
Copy link
Contributor

@JianyuWang0623 JianyuWang0623 commented Dec 25, 2024

Summary

  1. sim:thermal: The sim:thermal is based on sim:nsh : Users can get basic configuration of thermal from sim:thermal.
  2. sim:thermal: The thermal core depends on LPWORK : The thermal core depends on LPWORK without which will not work.
  3. Thermal: The thermal core depends on LPWORK : Like 2, Add depends to Kconfig of thermal.
  4. Thermal/dummy: Add temperature jump to test hysteresis : Update temperature of dummy thermal driver, so that we can test hysteresis with sim.
  5. Thermal/procfs: Do not print invalid target cooling state directly : Maybe we should print special string (e.g. invalid) to mark that the target cooling state is invalid instead of print UINT_MAX directly.
  6. sim:thermal: Enable command line history : Enable command line history for sim:thermal, make it eaiser to input repeat commands.
  7. Thermal/step_wise: Fix that hysteresis takes no effect : Fix error that hysteresis takes no effect

Details can be found in commit message if need.

Impact

drivers/thermal

Testing

  1. Selftest (log can be found in commit message)
  2. NuttX CI

Coverting to including sim:nsh.

Signed-off-by: wangjianyu3 <[email protected]>
Log

  z:cpu-thermal t:50 t:1 h:16 l:0 c:fan0 s:0|(invalid)
  z:cpu-thermal t:47 t:1 h:16 l:0 c:fan0 s:0|(invalid)
  z:cpu-thermal t:52 t:1 h:16 l:0 c:fan0 s:0|(invalid)
  z:cpu-thermal t:49 t:1 h:16 l:0 c:fan0 s:0|(invalid)
  z:cpu-thermal t:54 t:1 h:16 l:0 c:fan0 s:0|(invalid)
  z:cpu-thermal t:51 t:1 h:16 l:0 c:fan0 s:0|(invalid)
  z:cpu-thermal t:56 t:1 h:16 l:0 c:fan0 s:0|(invalid)

Signed-off-by: wangjianyu3 <[email protected]>
Diff
  - z:cpu-thermal t:48 t:1 h:16 l:0 c:fan0 s:0|4294967295
  + z:cpu-thermal t:48 t:1 h:16 l:0 c:fan0 s:0|(invalid)

The invalid value 4294967295(THERMAL_NO_TARGET, defined as UINT_MAX(0xffffffff)) may bother users.

Signed-off-by: wangjianyu3 <[email protected]>
Make it easier to debug.
e.g. execute some commands frequently, like:
  cat /proc/thermal/cpu-thermal

Signed-off-by: wangjianyu3 <[email protected]>
Test command

  nsh> cat /proc/thermal/cpu-thermal

Trips

  /* Copy from drivers/thermal/thermal_dummy.c */

  {.name = "cpu_crit",   .temp = 90, .hyst = 5, .type = THERMAL_CRITICAL},
  {.name = "cpu_alert1", .temp = 70, .hyst = 5, .type = THERMAL_HOT},
  {.name = "cpu_alert0", .temp = 60, .hyst = 5, .type = THERMAL_NORMAL},

Without this patch

  # Error occured when temperature jump to 69.

  z:cpu-thermal t:67 t:1 h:16 l:0 c:fan0 s:0|(invalid)
  z:cpu-thermal t:72 t:1 h:16 l:0 c:fan0 s:1|1
  z:cpu-thermal t:69 t:1 h:16 l:0 c:fan0 s:0|0
  z:cpu-thermal t:74 t:1 h:16 l:0 c:fan0 s:1|1

With this patch

  z:cpu-thermal t:67 t:1 h:16 l:0 c:fan0 s:0|(invalid)
  z:cpu-thermal t:72 t:1 h:16 l:0 c:fan0 s:1|1
  z:cpu-thermal t:69 t:1 h:16 l:0 c:fan0 s:1|1
  z:cpu-thermal t:74 t:1 h:16 l:0 c:fan0 s:2|2

Signed-off-by: wangjianyu3 <[email protected]>
@github-actions github-actions bot added Area: Drivers Drivers issues Board: simulator Size: M The size of the change in this PR is medium labels Dec 25, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 25, 2024

[Experimental Bot, please feedback here]

This PR summary is a good start, but could be improved to better meet the NuttX requirements. Here's a revised version:

Summary

This PR improves the simulated thermal driver (sim:thermal) and the thermal core framework within NuttX, focusing on stability, testability, and user experience. It addresses several issues related to dependencies, hysteresis testing, and output clarity. Specifically, this PR:

  • Ensures the thermal framework and simulated driver correctly depend on the low-priority worker thread (LPWORK). This fixes a potential operational issue where the thermal framework wouldn't function without LPWORK.
  • Enhances the dummy thermal driver to allow for temperature jumps, facilitating hysteresis testing within the simulation environment.
  • Improves the procfs thermal output by replacing the raw UINT_MAX value for invalid target cooling states with a more user-friendly "invalid" string.
  • Enables command-line history within the sim:thermal application for improved user interaction.
  • Fixes a bug where the hysteresis setting was not being applied correctly within the step-wise cooling governor.

No related NuttX issues or NuttX Apps issues/PRs are directly associated with this change, though it potentially resolves latent bugs.

Impact

  • Is new feature added? Is existing feature changed? Existing features are improved and bugs are fixed. The addition of temperature jumps to the dummy driver could be considered a minor new feature for testing purposes.
  • Impact on user (will user need to adapt to change)? NO. The changes improve usability and fix underlying issues without requiring user adaptation. The output in procfs is slightly clearer.
  • Impact on build (will build process change)? NO. The Kconfig dependency additions are straightforward and should not impact the build process negatively.
  • Impact on hardware (will arch(s) / board(s) / driver(s) change)? NO. Changes are limited to the simulated thermal driver and core thermal framework.
  • Impact on documentation (is update required / provided)? Potentially YES. If documentation exists regarding the sim:thermal application or the output format of the thermal procfs entries, it should be updated to reflect the changes. Consider adding a brief note about the LPWORK dependency.
  • Impact on security (any sort of implications)? NO.
  • Impact on compatibility (backward/forward/interoperability)? NO. The changes address bugs and improve functionality without breaking compatibility.
  • Anything else to consider? None.

Testing

I confirm that changes are verified on local setup and works as intended:

  • Build Host(s): (Please specify: OS, CPU architecture, compiler version) For example: Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
  • Target(s): sim:qemu-x86_64:nsh (Please expand on this. Were other architectures/boards tested?)

Testing logs before change:

*(Insert relevant logs demonstrating the issue(s) being fixed.  For example, show the UINT_MAX output, demonstrate the lack of hysteresis, or show the failure without LPWORK)*

Testing logs after change:

*(Insert relevant logs demonstrating the fix. For example, show the "invalid" output, the working hysteresis, or successful operation with LPWORK enabled.)*

Key improvements in this version:

  • Consolidated Summary: Presents a cohesive narrative of the changes rather than a list of commits.
  • Clearer Impact Assessment: More explicitly addresses each impact category with specific details.
  • Emphasis on User Benefit: Highlights how the changes improve usability and reliability.
  • Call for Documentation Updates: Recognizes the potential need for documentation changes.
  • Request for Complete Testing Information: Prompts for more specific details about the testing environment and logs demonstrating the fixes.

By providing more context and detail, this revised PR description makes it easier for reviewers to understand and evaluate the changes. Remember to replace the placeholder text with your actual build host information and testing logs.

@xiaoxiang781216 xiaoxiang781216 merged commit c6c89ac into apache:master Dec 26, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Drivers Drivers issues Board: simulator Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants