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

MHP30 Soldering/Reflow Temperature Profile #1672

Merged
merged 19 commits into from Apr 25, 2023
Merged

Conversation

codingcatgirl
Copy link
Contributor

@codingcatgirl codingcatgirl commented Apr 23, 2023

This PR adds a feature to configure a soldering Profile through the settings, which fulfils feature requests #1201 and #1619.

This feature has the following… uh… features:

  • Set preheat temperature and preheat speed (in degrees per second) for the start of the profile. The preheat speed is a maximum speed of course, if the MHP30 can't keep up, it will happen more slowly.
  • Configure up to 5 phases, each consisting of a temperature and a duration, the temperature will be gradually moved to the target temperature during this duration. Again, if the MHP30 can't keep up, it will happen more slowly.
  • Set a cooldown speed (in degrees per second) for the cooldown phase at the end of the profile.
  • The device beeps when the cooldown has been completed.

The feature is only included when compiling for the MHP30. The reflow profile mode is accessible through a long press on the soldering mode button and, on the MHP30, replaces the previous long press function that was a shortcut to the temperature control.

As a side effect this PR fixes a bug where the the LED would flash wildly when the device turns off the screen.

There's some other bugs i've found while coding on this that i have not fixed, which i have reported in #1670 and #1671.

I have tested it and from what i can tell everything seems to work.

However, there are a few open questions:

  • Add translations for languages other than English and German. – Is there an easy way to fill the translation files with defaults or do i have to edit every file manually?
  • Compilation size – This feature adds quite a few more strings. For the MHP30 the size of the compiled hex file now surpasses 128KB. Seems to not be an issue for the MHP30, but it looks to me like all strings, even for disabled features, are included in every firmware and i wonder if some firmware builds are now too big and if this needs to be adressed.
  • Documentation – On one hand, i can't run gen_menu_docs.py cause it fails with load_json() missing 1 required positional argument: 'skip_first_line'. The other question however is if this feature should be documented in some way beyond that.

Looking forward to feedback :)

This PR closes #1201 and also fixes #1570!

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.

Hia,
Thank you so much for getting this going.

A few smaller requests for changes that should hopefully be super simple.
(Please feel free to comment back if you disagree; as well all opinions I welcome discussion)

Can you please ensure that even with this profile build flag turned on, by default the device still acts in the old way where it just has a single setpoint with no timeout; and then profile based modes can be turned on if desired for the use case.

❤️ Thank you so much for this though

source/Core/BSP/BSP.h Outdated Show resolved Hide resolved
source/Core/Inc/Settings.h Outdated Show resolved Hide resolved
source/Core/Threads/OperatingModes/Soldering.cpp Outdated Show resolved Hide resolved
@Ralim
Copy link
Owner

Ralim commented Apr 23, 2023

Is there an easy way to fill the translation files with defaults or do i have to edit every file manually?

Not really a nice way at the moment, I generally just use the VSCodium replace tools with a regex to insert the english placeholder.

This feature adds quite a few more strings. For the MHP30 the size of the compiled hex file now surpasses 128KB. Seems to not be an issue for the MHP30, but it looks to me like all strings, even for disabled features, are included in every firmware and i wonder if some firmware builds are now too big and if this needs to be adressed.

If we are out of space, yeah we will need to address it; any chance you have any ideas of how best to mask out what strings are actually required per device? Definitely open to ideas here, just hasnt (yet) been touched.

On one hand, i can't run gen_menu_docs.py cause it fails with load_json() missing 1 required positional argument: 'skip_first_line'. The other question however is if this feature should be documented in some way beyond that.

Ack I'll fixup the gen docs this week.

It would be ideal if you could add a new markdown file to the Documentation folder about the profile mode (note in the start of the page its only for MHP30 atm). And then link to it from the index.md.

@codingcatgirl
Copy link
Contributor Author

Not really a nice way at the moment, I generally just use the VSCodium replace tools with a regex to insert the english placeholder.

Alright, so i will probably also write a python script and add it.

If we are out of space, yeah we will need to address it; any chance you have any ideas of how best to mask out what strings are actually required per device? Definitely open to ideas here, just hasnt (yet) been touched.

The question mostly is… are we out of space? How much space does each device have? The TS100 Firmware in English still compiles at 128k at least.

An idea that i had would have been to run the make_translation.py multiple times, before each build or at least before each device. If some strings in the translation definition file are marked as only needed for certain devices, the string could be set to an empty string there which should be enough? Alternatively, features could be defined per device in a format that is readable for both python and the makefile and instead of defining which features to compile in via the configuration.h it could be added using a compiler flag. (So there are no deviations between what the compilers thinks and what the translation script thinks.)

It would be ideal if you could add a new markdown file to the Documentation folder about the profile mode (note in the start of the page its only for MHP30 atm). And then link to it from the index.md.

I'lll take a look.

@Ralim
Copy link
Owner

Ralim commented Apr 23, 2023

The question mostly is… are we out of space? How much space does each device have? The TS100 Firmware in English still compiles at 128k at least.

Keep in mind that some TS100's actually only have 64k of flash, so we need at the least TS100 to stay in its 64k box.

The bigger issue is the TS80P which is desperately out of space. While so far they have all used legit ST micros (when generally have always had the full 128k fitted, even if not validated). I dont trust Miniware to keep that going forever since they have swapped what device-implementing-a-clone-of-the-ST-api they have used in the TS100 a bunch already.

An idea that i had would have been to run the make_translation.py multiple times, before each build or at least before each device. If some strings in the translation definition file are marked as only needed for certain devices, the string could be set to an empty string there which should be enough? Alternatively, features could be defined per device in a format that is readable for both python and the makefile and instead of defining which features to compile in via the configuration.h it could be added using a compiler flag. (So there are no deviations between what the compilers thinks and what the translation script thinks.)

I'm leaning towards the latter option of some sort of feature matrix file per device, that python can read and use when generating strings etc. Keeping in mind that as the python generates most of the translation data already, its not a jump for it to also create the header files etc to match the build. Would take a bunch of time to mark strings in the defs as for what features and update the python to generate trimmed files though.

@codingcatgirl codingcatgirl force-pushed the profiles branch 2 times, most recently from 6c6a9d1 to df0fbee Compare April 24, 2023 00:37
@codingcatgirl
Copy link
Contributor Author

Well, refactoring done and it looks like everything compiles, with the exception of the T80P for a few languages. :/ I can't spontaneously think of a way to save space there other than by implementing the translation strings differently.

@codingcatgirl
Copy link
Contributor Author

Okay, i've build a relatively simple system that filters out translations. Translations can specify macros that, if they exist, will include/exclude that string. I'm simply calling GCC to export all preprocessor macros for that build and then search through them with python. Works fine!

The fun result is that with this merge requests, some firmwares now actually get smaller :D

Compared to the last official release:

  • MHP30_EN.bin: 44000 → 45304 (+1304 bytes) (ok, to be expected with a new feature)
  • Pinecil_EN.bin: 53640 → 53456 (-184 bytes)
  • Pinecilv2_EN.bin: 189960 → 190044 (+84 bytes) (not exactly sure why this one got bigger, maybe you have an idea?
  • TS100_EN.bin: 37956 → 37916 (-40 bytes)
  • TS80_EN.bin: 38288 → 38232 (-56 bytes)
  • TS80P_EN.bin: 43220 → 43176 (-44 bytes)

I'm gonna take a look at documentation now and then assume that this PR is done?

@codingcatgirl
Copy link
Contributor Author

codingcatgirl commented Apr 24, 2023

I've added documentation for the soldering profile mode. All that's needed as far as i can tell is to run the gen_menu_docs.py script if you tell me how :)

I will also double check that the firmwares compiled with the current state of this PR work as expected on both my MHP30 and my TS100.

@codingcatgirl
Copy link
Contributor Author

Had to fix a mistake, now i can confirm it runs as expected even outside of profile mode on both TS100 and MHP30.

@discip discip requested a review from Ralim April 24, 2023 15:43
@discip
Copy link
Collaborator

discip commented Apr 24, 2023

First thing I thought reading the discussion about changing file size for devices that don't have features like this:
Isn't there a way to limit the code change and therefore the file size to only the affected devices?

And there you are providing just that!

GREAT, FINALLY! 🚀

Since this only applies to the function you're implementing, I kindly ask you to do the same for functions like Hallsensor, BLE and PD as well. To possibly further reduce the firmware size for memory-critical-devices like the TS80P? 😊

@codingcatgirl
Copy link
Contributor Author

codingcatgirl commented Apr 24, 2023

Since this only applies to the function you're implementing, I kindly ask you to do the same for functions like Hallsensor, BLE and PD as well. To possibly further reduce the firmware size for memory-critical-devices like the TS80P?

silently passes you a note that you should just check out the diff, because i already did that

i mean, why else would i brag about all firmwares getting smaller :P

@codingcatgirl
Copy link
Contributor Author

Just made a change so that the the black python checker stops complaining too.

@discip discip enabled auto-merge April 24, 2023 20:17
@codingcatgirl
Copy link
Contributor Author

codingcatgirl commented Apr 24, 2023

Hey, there's still the call of the menu docs script missing, too early for auto-merge :D

Luckily, auto-merge doesn't seem to be working cause the workflow is paused for some reason

@discip
Copy link
Collaborator

discip commented Apr 24, 2023

Hey, there's still the call of the menu docs script missing, too early for auto-merge :D

Luckily, auto-merge doesn't seem ot be working cause the workflow is paused for some reason

As long as @Ralim does not approve the review, nothing will be merged yet.

I'm gonna take a look at documentation now and then assume that this PR is done?

I thought this would be good to go? 😊

If this isn't ready yet, though, you still have the option to convert it into a draft.

@codingcatgirl
Copy link
Contributor Author

In the end there is only one command left to run. Unfortunately, i can't run it, cause the documentation is wrong :D Then it should be ready, yes.

CMakeLists.txt Outdated Show resolved Hide resolved
@Ralim
Copy link
Owner

Ralim commented Apr 25, 2023

For this PR dont worry about gen_menu_docs, Ill fix that post-merge.
If you can just drop that cmake file from this PR It looks good to go

@codingcatgirl
Copy link
Contributor Author

codingcatgirl commented Apr 25, 2023

CMakeFile is dropped, i also did a bit of a rebase and squash so there's less commits, Could do more if desired, but i'm not sure if that will break the discussions in this PR and i don't know if you will squash this before merging anyway.

Well, looks like we're done here? :)

Documentation/GettingStarted.md Outdated Show resolved Hide resolved
Translations/translation_YUE_HK.json Outdated Show resolved Hide resolved
Translations/translation_YUE_HK.json Outdated Show resolved Hide resolved
Translations/translation_YUE_HK.json Outdated Show resolved Hide resolved
Translations/translation_ZH_CN.json Outdated Show resolved Hide resolved
Translations/translation_ZH_CN.json Outdated Show resolved Hide resolved
Translations/translation_ZH_TW.json Outdated Show resolved Hide resolved
Documentation/GettingStarted.md Outdated Show resolved Hide resolved
Documentation/GettingStarted.md Outdated Show resolved Hide resolved
Documentation/Menu.md Outdated Show resolved Hide resolved
@discip
Copy link
Collaborator

discip commented Apr 25, 2023

@codingcatgirl
Please check this one more time.

codingcatgirl and others added 2 commits April 25, 2023 19:09
Co-authored-by: discip <53649486+discip@users.noreply.github.com>
Co-authored-by: discip <53649486+discip@users.noreply.github.com>
@codingcatgirl
Copy link
Contributor Author

Done. Anything else left for me to do?

Co-authored-by: discip <53649486+discip@users.noreply.github.com>
@discip discip enabled auto-merge April 25, 2023 18:16
@discip discip merged commit 4574071 into Ralim:dev Apr 25, 2023
11 checks passed
@Ralim
Copy link
Owner

Ralim commented Apr 25, 2023

Thank you for this, ☺️ it looks great to me :)

@codingcatgirl codingcatgirl deleted the profiles branch April 25, 2023 20:49
@codingcatgirl
Copy link
Contributor Author

Thanks for merging :)

@p0ojisan
Copy link

p0ojisan commented Jul 2, 2023

Is this feature available on latest dev branch?

I downloaded artifact from latest dev branch and applied my mhp30. I see profile preferences in setting menu, but cannot enter to profile mode when press & holding A (left) button.

@codingcatgirl
Copy link
Contributor Author

codingcatgirl commented Jul 2, 2023

It was definitely working when the Pull Request was merged.

Unfortunately, it looks like @Ralim's Commit for S60 support has removed the button handling to enter the soldering profile mode again.

@Ralim
Copy link
Owner

Ralim commented Jul 2, 2023

Ah ffs.

Git hated me with that merge I swear.

On it

Give me a day or so.

Ralim added a commit that referenced this pull request Jul 9, 2023
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.

MHP30 - Idle bug Feature request on MHP30
4 participants