Skip to content
New issue

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

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

Already on GitHub? # to your account

Add CONFigure:FANx:HYSteresis command (#2) #104

Merged
merged 3 commits into from
Aug 13, 2024
Merged

Add CONFigure:FANx:HYSteresis command (#2) #104

merged 3 commits into from
Aug 13, 2024

Conversation

Zitt
Copy link
Contributor

@Zitt Zitt commented Aug 10, 2024

Allows user to change the hysteresis of a given fan to limit the amount of INFO lines sent to LOG and/or SYSLOG.

Zitt and others added 3 commits August 10, 2024 18:49
Allows user to change the hysteresis of a given fan to limit the
amount of INFO lines sent to LOG and/or SYSLOG.
* Update CodeQL (tjko#103)

* Fix codql finding: Multiplication result converted to larger type
* update workflow

* Add CONFigure:FANx:HYSteresis command
Allows user to change the hysteresis of a given fan to limit the
amount of INFO lines sent to LOG and/or SYSLOG.

* Fix to memory memory leak on failed MQTT connections.

* minor cleanup

* Changes CONFigure:FANx:HYSteresis to CONFigure:FANx:HYS_Tacho
Add CONFigure:FANx:HYS_Pwm and CONFigure:FANx:HYSTeresis?
so that hysteresis can be applied to both the tacho and pwm signals.

---------

Co-authored-by: Timo Kokkonen <tjko@iki.fi>
@tjko tjko changed the base branch from main to hysteresis August 13, 2024 02:37
@tjko
Copy link
Owner

tjko commented Aug 13, 2024

Thanks, looks good. I'll just make couple tweaks to keep commands consistent with existing commands (and to behave like other SCPI commands).

@tjko tjko merged commit f3ec074 into tjko:hysteresis Aug 13, 2024
9 checks passed
@Zitt
Copy link
Contributor Author

Zitt commented Aug 13, 2024

@tjko I made even more changes in my fork:
135afb4
but I wasn't completely happy with the implementation.

I wanted there to be a HYST sub command (IE see line 2751 in command.c).
but after spending hours trying to make it work; I realized that you can't have CONF:FANx:HYS:PWM?
because the code isn't structured to "save" the fan index in
run_cmd()
function. The code is only able to "look back" and previous cmd and not prev_cmd-1 for the index.
I couldn't make it work.

As a result; I just flattened the PWM and TACHO hysteresis commands into
CONFigure:FANx:HYS_Tacho
CONFigure:FANx:HYS_Pwm

IF you'd like me to try a pull request with my changes; please let me know.

@tjko
Copy link
Owner

tjko commented Aug 13, 2024

@Zitt, I noticed same when looking the pull request :)

Current "prev_cmd" pointer was a "temporary" hack, that I never got around to fix it seems.
I'll see if I can make change to expose the full command "stack" to the command functions in case they're interested other than the previous command in the chain...

@Zitt
Copy link
Contributor Author

Zitt commented Aug 14, 2024

Looks like your current commit #108 has the updated changes from my commit.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants