Replies: 2 comments
-
Please marry me. Joke aside...
Thanks a lot, you made me happy. Overall, they're good ideas. Some of them I've already had, and some others require thought and discussion. I'm currently working on my homelab to deploy a new Docker application (no link with this repository) and will get back to you later tonight or this week. Thanks for your work. |
Beta Was this translation helpful? Give feedback.
-
Update on ImplementationI've completed the first three points of the proposed changes. Here's a summary of what has been implemented: 1. Handler for Applying Fan SpeedI’ve added a handler function for applying user-defined fan speed. This function supports both static and dynamic fan control and can handle decimal or hexadecimal inputs for fan speed. Below is the function implementation: # Apply user-defined fan control settings
#
# This function applies user-defined fan control settings based on the specified mode and fan speed.
# It handles both decimal and hexadecimal fan speed inputs, converting between them as needed.
# The function then applies the fan control and updates the current fan control profile.
#
# Parameters:
# $1 (MODE): The fan control mode.
# 1 for static fan speed, 2 for dynamic (interpolated) fan control.
# $2 (LOCAL_FAN_SPEED): The desired fan speed. Can be in decimal (0-100) or hexadecimal (0x00-0x64) format.
#
function apply_user_fan_control() {
local MODE=$1
local LOCAL_FAN_SPEED=$2 2. Interpolation Welcome ChartA table with calculated parameters based on the set values is displayed on startup: The calculation of line interpolation has been moved to functions.sh and can be used by: calculate_interpolated_fan_speed "$temp" "$lower_threshold" "$upper_threshold" "$min_speed" "$max_speed" This method returns the calculated interpolated fan speed as a decimal percentage (0-100). 3. Unit Tests for Interpolation CalculationUnit tests for the interpolation calculation have been added using the bats-core library. (I didn’t create additional files required by their license.) To run tests, use: ./tests.sh 4. Next StepsI think this part should be completed after merging the current changes and setting up CI/CD. |
Beta Was this translation helpful? Give feedback.
-
This disscussion is connected to #60.
@tigerblue77 I’ve locally merged the code with the current state of the master branch, incorporating all changes from my pull request. I really like the improvements you’ve made—they align with many of my ideas, and they enhance the overall solution. Great work! 😃
Here are some additional suggestions that I think could further improve the code and its functionality:
1.
functions.sh
This would make the fan speed-setting function essentially “untouchable” in the future, while the handler remains open for extensions, such as adding a PID control option.
2.
Dell_iDRAC_fan_controller.sh
functions.sh
or a newcalculations.sh
file. This could help clean up the main loop, making it easier to read and maintain. I’ll discuss main loop cleanup further below.3.
tests.sh
(Unit Tests)4. Other Proposals
While not directly related to line interpolation, here are some additional ideas that could enhance the project:
Main Loop Refactor: Break the main loop into smaller, dedicated functions, such as:
apply_overheat
,apply_fan_speed
,apply_fan_interpolated_speed
,print_temperatures
, etc.This would improve code readability and modularity.
Optimize Fan Speed Changes: Only overwrite the fan speed if the value changes. Although I haven’t measured the exact impact, it’s likely that establishing a connection and processing data via iDRAC could be slightly costly.
Enhanced Console Output: Add color and styling to the console output using ANSI codes. This could improve visibility and make the output more user-friendly. However, keep in mind that this might require additional maintenance, and the printing-related code could become slightly harder to read.
What are your thoughts on these changes? I’m happy to discuss further and collaborate on implementing them!
Beta Was this translation helpful? Give feedback.
All reactions