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

Usermod Updated: Internal Temperature V2 #4033

Conversation

adamsthws
Copy link
Contributor

@adamsthws adamsthws commented Jun 26, 2024

Hello!

Please may I request to add the following improvement to the usermod: 'Internal Temperature V2'...

Added high temperature indicator/action

  • A configurable preset is activated when the internal temperature raises above a configurable threshold temperature.

  • When the internal temperature falls back below the threshold, the previously active playlist/preset/state is re-activated.

  • If the preset to trigger is set to 0, no preset is activated when temperature rises above the threshold.

  • A small temperature buffer prevents frequent toggling between states when the temperature is close to the threshold.
    (The reset threshold is automatically calculated to be two degrees lower than whatever the activation threshold is set to).

  • To prevent the user setting the loop interval too low, a minimum allowable interval has been added.

To do

  • In a later phase I plan to add isAboveThreshold to MQTT.

Thankyou!
Adam Matthews

# Added high temperature indicator/action...

- A configurable preset is activated when the internal temperature raises above a configurable threshold temperature.

- When the internal temperature falls back below the threshold, the previously active preset is re-activated.

- To prevent frequent toggling between states when the temperature is close to the threshold, the reset threshold is slightly lower than the activation threshold to provide a small buffer.

- Reset threshold is automatically calculated to be two degrees lower than whatever the activation threshold is set to.

- To prevent the user setting the loop interval too low, a minimum allowable interval has been added.
Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some things that need to be changed.

When you want to temporarily store current preset, you are better of by saving currentPreset variable. If that is 0 then it is safe to save current state using saveTemporaryPreset().

I'd also like to invite @lost-hope to do a peer review.

Applied various fixes as advised by @blazoncek

Thankyou for the advice!

- Updated float: 95.0 > 95.0f

- Updated type: const > constexpr

- Comments clarified

- Preset setting: `-1` > `0`
Github flavoured markdown didn't work as expected.
@blazoncek
Copy link
Collaborator

There ar still a few <br> in readme. You missed some float constants and preset id could be uint8_t to save a bit of RAM (not mandatory).

- Updated all doubles to floating-point literals by adding explicit `f` suffix

- Removed all remaining html from readme markdown documentation.
@adamsthws
Copy link
Contributor Author

There ar still a few <br> in readme. You missed some float constants and preset id could be uint8_t to save a bit of RAM (not mandatory).

Sorry I missed these... Thanyou for being so thorough with your review!

Simplified the code by removing an unnecessary function definition and instead using direct assignment in the place where the function was previously called.
@adamsthws
Copy link
Contributor Author

There are some things that need to be changed.

When you want to temporarily store current preset, you are better of by saving currentPreset variable. If that is 0 then it is safe to save current state using saveTemporaryPreset().

I'd also like to invite @lost-hope to do a peer review.

In addition to storing current preset, if a playlist is active, is it also possible to store the current playlist (and position in playlist?) to resume from the same place later?

@blazoncek
Copy link
Collaborator

In addition to storing current preset, if a playlist is active, is it also possible to store the current playlist (and position in playlist?) to resume from the same place later?

Yes, use currentPlaylist which is set if non-zero.

Enabled the storing the currently active preset or playlist for it to be restored later
@adamsthws
Copy link
Contributor Author

In addition to storing current preset, if a playlist is active, is it also possible to store the current playlist (and position in playlist?) to resume from the same place later?

Yes, use currentPlaylist which is set if non-zero.

This is now implimented, thankyou for your guidance and patience Blaz!

@blazoncek
Copy link
Collaborator

@lost-hope is this ok with you?

@lost-hope
Copy link
Collaborator

@lost-hope is this ok with you?

Yep looks fine to me.

@blazoncek blazoncek merged commit a812fc0 into Aircoookie:0_15 Jul 3, 2024
18 checks passed
@adamsthws adamsthws deleted the 0_15_internal_temp_usermod_improvement_new branch July 3, 2024 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants