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

Add a sleep timeout setting for hall sensor #1969

Merged
merged 6 commits into from
Aug 21, 2024

Conversation

aveao
Copy link
Contributor

@aveao aveao commented Aug 20, 2024

  • Please check if the PR fulfills these requirements
  • The changes have been tested locally - Works on my iron™
  • There are no breaking changes

^ I'm not sure, I changed the order of the settings in Settings.h and Settings.c, I'm not sure if this breaks anything. I did get my settings reset after the flash (with a Certain settings changed! message). Please let me know if I should just add it to the end of the list or something, which would then hopefully make it non-breaking.

  • What kind of change does this PR introduce?

It adds an option to sleep when the soldering iron is placed on a stand, with the amount to sleep configurable. Default is 1 second, which was the behavior before. It can be set up to 60 seconds, with 5 second increments.

I had opened a feature request here: #1968

(Implements #1968)

  • What is the current behavior?
    With a hall effect sensor configured, the iron goes to sleep after 1 second after going above threshold.

  • What is the new behavior (if this is a feature change)?
    This amount can be configured.

  • Other information:
    Fitting the text is hard, I had to put it without spaces in English. Good luck to translators.

IMG_20240820_185405

@aveao
Copy link
Contributor Author

aveao commented Aug 20, 2024

Will try and fix issues in CI.

ProfilePhase5Temp = 50, // Temperature to target for the end of phase 5
ProfilePhase5Duration = 51, // Target duration for phase 5
ProfileCooldownSpeed = 52, // Maximum allowed cooldown speed in degrees per second
HallEffectSleepTime = 29, // Seconds timeout to sleep when hall effect over threshold
Copy link
Owner

Choose a reason for hiding this comment

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

Yeah this has to go at the end, these index's are used for setting migration between reflash so new settings need to be appended :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in fdefb5b + fb904a5

Copy link
Owner

@Ralim Ralim left a comment

Choose a reason for hiding this comment

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

Looks fantastic, please append the setting to the end of the list to ensure that settings migrate for users but otherwise looks great 👍🏼

@aveao
Copy link
Contributor Author

aveao commented Aug 21, 2024

I see that CI failure is due to all but one language files missing the new line, should I add the english text in all languages?

@Ralim
Copy link
Owner

Ralim commented Aug 21, 2024

I see that CI failure is due to all but one language files missing the new line, should I add the english text in all languages?

Yeah you have to add something to all files, I normally add English as the filler by default (I only know one language :( )

@aveao
Copy link
Contributor Author

aveao commented Aug 21, 2024

I see that CI failure is due to all but one language files missing the new line, should I add the english text in all languages?

Yeah you have to add something to all files, I normally add English as the filler by default (I only know one language :( )

Done :)

Copy link
Owner

@Ralim Ralim left a comment

Choose a reason for hiding this comment

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

This seems good to me. Have you tested it ?

@aveao
Copy link
Contributor Author

aveao commented Aug 21, 2024

This seems good to me. Have you tested it ?

Yes, also measured times which seemed accurate.
(I only tested before the settings reorder but I do not expect that to break the functionality.)

@Ralim
Copy link
Owner

Ralim commented Aug 21, 2024

Fantastic, just wanted to check as I dont have my unit next to me.

@Ralim Ralim enabled auto-merge (squash) August 21, 2024 03:50
auto-merge was automatically disabled August 21, 2024 04:02

Head branch was pushed to by a user without write access

@aveao
Copy link
Contributor Author

aveao commented Aug 21, 2024

/build/ironos #  make  clean  check-style
make  -C source/  clean
make[1]: Entering directory '/build/ironos/source'
Building for Pine64 Pinecilv1
rm -Rf Core/Gen
rm -Rf Objects
rm -Rf Hexfile/*
rm -Rf ../Translations/__pycache__
make[1]: Leaving directory '/build/ironos/source'
make  -C source/  check-style
make[1]: Entering directory '/build/ironos/source'
Building for Pine64 Pinecilv1


Style check: PASS


make[1]: Leaving directory '/build/ironos/source'
/build/ironos #

Would you be able to approve CI run again? Sorry for not checking this earlier, didn't know what tests were relevant (maybe I should send a PR to the development page with info about that also?).

@Ralim
Copy link
Owner

Ralim commented Aug 21, 2024

Yeah I'll let it run whenever I get the notification; this trap only happens for first PR to the repo.

Documentation updates are always loved 🤣

@aveao
Copy link
Contributor Author

aveao commented Aug 21, 2024

Neato, everything seems to pass now. Thank you very much for the speedy review btw :)

@Ralim Ralim merged commit 02b21a6 into Ralim:dev Aug 21, 2024
18 checks passed
@Ralim
Copy link
Owner

Ralim commented Aug 21, 2024

Thank you for getting this into a PR so soon from filing the issue 🙇🏼 Reviewing and merging PR's is always easier than doing the work.

@aveao aveao deleted the hall-effect-sleep-timeout branch August 21, 2024 08:36
@fredericuslaurentii
Copy link
Contributor

What is the point after the unit of measurement for?
I think we can gain a letter from its removal

@aveao
Copy link
Contributor Author

aveao commented Sep 10, 2024

What is the point after the unit of measurement for? I think we can gain a letter from its removal

Do you mean the s (seconds) or do you mean the .? the . has already been there and is a scroll progress bar in the list.
I'd be very much for keeping seconds (alb maybe making it smaller).

@fredericuslaurentii
Copy link
Contributor

Whops, yes, you're right. It's just the scroll bar 😅.
I managed to fit the translation in Italian although I had to adopt an abbreviation.

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