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 macro to enable tip types for supported hardware only #2031

Merged
merged 4 commits into from
Dec 24, 2024
Merged

Conversation

ia
Copy link
Collaborator

@ia ia commented Dec 23, 2024

  • Please check if the PR fulfills these requirements
  • The changes have been tested locally
  • There are no breaking changes
  • What kind of change does this PR introduce?
    Enable Soldering Tip Type routines for supported hardware only.

  • What is the current behavior?
    There are parts of code which are not relevant for all supported models, but yet the code is compiled & takes space on flash.

  • What is the new behavior (if this is a feature change)?

  • add TIP_TYPE_SUPPORT macro;
  • enable related functions using the macro;
  • enable TIP_TYPE_SUPPORT only for TS101 and Pinecil v2 as main targets for this feature;
  • put Soldering Tip Type down the list in the settings, since it's optional.
  • Other information:

Hello. I understand that this is raw & experimental feature. And I'm sorry I wasn't around when it was implemented, because probably even back then I would suggest the same what I propose now. When I was testing some translations on TS80P, I did end up with this:

soldering tip type

Apparently, Soldering Tip Type is enabled for TS80P but none of available options (even Auto) are picked from the related switch/case block. But instead of fixing the symptom (i.e., just enable Auto for TS80P & others), I took some liberty to look around the related parts of code scattered in the project and came out with this suggestion. Further I will add a couple of comments as code review to clarify the decisions.

And just as an illustration - the results which I did get for TS80P/EN:

  • build from the current dev:
Memory region         Used Size  Region Size  %age Used
             ROM:       41488 B        46 KB     88.08%
  • build with this patch:
Memory region         Used Size  Region Size  %age Used
             ROM:       41420 B        46 KB     87.93%

I know that ~0.15% is just laughable, but to keep beefing up code base with unused code for some of the targets is just not quite reasonable approach, while the same space can be used for really useful features & code in the future.

Please, let me know what you think.

@ia ia added the Bug Serious issue or problem. label Dec 23, 2024
@ia ia requested review from Ralim and discip December 23, 2024 20:09
@ia ia self-assigned this Dec 23, 2024
@@ -218,6 +218,7 @@
#define TEMP_NTC 1
#define ACCEL_I2CBB1 1
#define POW_EPR 1
#define TIP_TYPE_SUPPORT 1 // Support for tips of different types, i.e. resistance
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Basically, I try to make the similar approach as for PROFILE_SUPPORT: we just enable everything related under the macro.

typedef enum {
TIP_TYPE_AUTO = 0, // value for the default case
TIP_TYPE_MAX = 0, // marker for settings when not supported
} tipType_t;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Implement duplicated tipType_t stub for two reasons:

  • TIP_TYPE_MAX = 0 for settings struct to avoid compile-time error and set 0/auto as default right there for non-supported models;
  • TIP_TYPE_AUTO = 0 for backward compatibility with the stub function (see below).

@@ -370,3 +374,6 @@ uint8_t getUserSelectedTipResistance() {
break;
}
}
#else
uint8_t getUserSelectedTipResistance() { return tipType_t::TIP_TYPE_AUTO; }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Instead of twisting my head about how to do it inside of the existed instance of getUserSelectedTipResistance(), I thought that the most simple & straight-forward approach is just to make the duplicated stub with the single return.

Comment on lines +303 to +306
#ifdef TIP_TYPE_SUPPORT
/* Tip Type */
{SETTINGS_DESC(SettingsItemIndex::SolderingTipType), nullptr, displaySolderingTipType, showSolderingTipType, SettingsOptions::SolderingTipType, SettingsItemIndex::SolderingTipType, 5},
#endif /* TIP_TYPE_SUPPORT */
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I took liberty to move the setting down inside of the soldering menu since in the UX design & usability usually they put variable/non-constant options closer to "the end" of "the list". So while temperature settings are applied to nearly every target (hence, "first in a row"), targets with support of different tips are limited. Just for the same reason we don't put "[thermo] profiles" submenu in the top of the list.

@ia ia enabled auto-merge December 23, 2024 20:27
@ia
Copy link
Collaborator Author

ia commented Dec 23, 2024

And the original problem with displaying tip type setting on TS80P takes root probably here because none of these macros inside lookupTipName() enabled, except TIPTYPE_TS80 but there is a tiny mistype in the macro usage. But even if we will fix the typo, the code probably won't compile because the tipType_t::TS80_4_5_OHM is commented & disabled.

UPD. Yeap (this is from dev for TS80P just with fixed typo):

Core/Src/Settings.cpp: In function 'const char* lookupTipName()':
Core/Src/Settings.cpp:323:19: error: 'TS80_4_5_OHM' is not a member of 'tipType_t'
  323 |   case tipType_t::TS80_4_5_OHM:
      |                   ^~~~~~~~~~~~

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.

I agree this is a cleaner way to do this

@ia ia merged commit 989a2cf into Ralim:dev Dec 24, 2024
18 checks passed
@Ralim
Copy link
Owner

Ralim commented Dec 24, 2024

Thank you for this, definitely glad you cleaned it up, and my bad for not doing this from the start 😁

@ia
Copy link
Collaborator Author

ia commented Dec 24, 2024

Thank you for this, definitely glad you cleaned it up, and my bad for not doing this from the start 😁

Please, don't worry. As I say I understand this is very raw & experimental feature. And it's better to separate this early while it have only prototypes, than some time later for larger code base. You're always welcome. ;)

As of for the feature itself, I really think it will be super great if, let's say, Pine short tips can be used not only with Pinecil V2 but with other T12-compatible irons.

@Ralim
Copy link
Owner

Ralim commented Dec 24, 2024

As of for the feature itself, I really think it will be super great if, let's say, Pine short tips can be used not only with Pinecil V2 but with other T12-compatible irons.

That was the motivating goal here :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Serious issue or problem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants