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

Temperature singleton #3643

Merged

Conversation

thinkyhead
Copy link
Member

Following #3631 – Adding in a singleton called "Temperature". Let's see how Travis likes it.

#if HAS_DIGIPOTSS

// From Arduino DigitalPotControl example
void digitalPotWrite(int address, int value) {
Stepper::void digitalPotWrite(int address, int value) {
Copy link

@ghost ghost Apr 29, 2016

Choose a reason for hiding this comment

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

Is void Stepper::digitalPotWrite(int address, int value) { right? (same comment in PR #3631)

@thinkyhead thinkyhead force-pushed the rc_singletons_plus_temperature branch 16 times, most recently from 22ff892 to 6843619 Compare May 1, 2016 20:50
@thinkyhead thinkyhead changed the title Stepper, Endstops, Planner, Temperature singletons Temperature singleton May 1, 2016
@thinkyhead thinkyhead force-pushed the rc_singletons_plus_temperature branch 4 times, most recently from 6f5dd03 to dbb509b Compare May 2, 2016 03:22
@thinkyhead
Copy link
Member Author

thinkyhead commented May 2, 2016

@jbrazio What do you think? Should I leave the temperature singleton named "temperature", use the capital "T" name, or choose something else (thermal)? The word temperature just doesn't sound like a "thing" so I don't "love" it, semantically-speaking.

@Blue-Marlin
Copy link
Contributor

Blue-Marlin commented May 2, 2016

For me it looks as if we are (or should be) dealing with a 'Heater' and its properties.

@thinkyhead
Copy link
Member Author

Well, in geekspeak this class is a "manager" of heaters and sensors. It does not model a heater.

And I actually don't want to go down the road of turning every entity (for example, individual heaters) into classes at this time. But in future…

@Blue-Marlin
Copy link
Contributor

Well said. We already are on dangerous train with this and some other PRs of the last days - for an RC.

@thinkyhead thinkyhead force-pushed the rc_singletons_plus_temperature branch from dbb509b to 40ba1db Compare May 3, 2016 00:48
@jbrazio
Copy link
Contributor

jbrazio commented May 3, 2016

@thinkyhead If this is and will be the responsible for all temperatures on Marlin then I would suggest ThermalManager or ThermalManagement which ever sounds better to a native speaker. In the future we model the heaters and coolers objects who will be managed by this one.

I not keen to use "short cryptic names" (despite "T" being rather a standard convection).

@Blue-Marlin so far Scott has been doing refactoring by changing function names and encapsulating which does not changes behavior.

@thinkyhead
Copy link
Member Author

which does not change behavior

True, though I can understand the concern with the way the Arduino compiler has acted up in the past, and of course how easy it is to miss a subtle typo. I made a point to avoid trying to be "clever" and rewrite any code. I think if you look through this PR (and I hope everyone has scanned it for stupid errors) you will find it is especially cautious.

What I wanted for this PR was to corral the variable names into their own namespaces and make it more obvious what belongs to what, but without changing anything else. It makes the code a bit more readable, the ownership of properties more clear, and (I hope) leads to clearer thinking about the relationships between these code units. The main errors that I found with Travis were symbols that didn't get the prefix added.

I've been running this code on my own machine and it seems to be working well. I haven't compared the print results from 1.0.2-1 to RCBugFix, so if there are any subtle printing issues (with corners, for example) indicating behavior changes, I cannot say. My prints have come out very good.

@Blue-Marlin
Copy link
Contributor

@jbrazio
Count the follow-ups of #3631 to see what I mean.

@thinkyhead
Copy link
Member Author

thinkyhead commented May 3, 2016

Count the follow-ups of #3631 to see what I mean.

I expected some followups to catch missed symbols. Considering the scope of the change, it's not too bad. I will take the time to do a full search on all the method and symbol names to see that nothing else was missed. Regular expression searches help this a lot. To find a symbol missing a dot prefix (or underscore) simply use regex [^\._a-zA-Z]symbol_name and ignore those within the class itself.

Thanks to those of you (@esenapaj) who actually took the time to "science the shit out of this thing," examine at the code and see if it was good, bad, or otherwise. I realize it seems cold and scary when seen from a great height, but when you really get to know it, it's actually quite warm and fuzzy.

@thinkyhead thinkyhead force-pushed the rc_singletons_plus_temperature branch 4 times, most recently from 5c5dc21 to a0dad51 Compare May 4, 2016 00:09
@thinkyhead
Copy link
Member Author

thinkyhead commented May 4, 2016

Ok, so I went through the same process as @esenapaj did with Stepper/Planner, searching all the symbols in the Temperature class to make sure they're properly prefixed. This is totally patched up now!

If anyone would like to prove that assertion wrong, I challenge you to find a bug in this. If you do, I give you one paragraph to tell me off, but no more. Time is short.

I'm trying the name heatManager on for size. How does that look and feel to the other code-heads? I resist using tempManager because temp tends to imply "temporary". But here are some other names that might be just as good or better… or worse…

  • tempControl …… as in tempControl.autotempShutdown()
  • heatingMgr …… as in heatingMgr.degBed()
  • thermal …… as in thermal.setTargetHotend(180, 0)
  • thermals …… as in thermals.setTargetBed(70) (of course, "thermals" are long underwear)

@jbrazio
Copy link
Contributor

jbrazio commented May 4, 2016

@thinkyhead I vote for:

thermalManager.autoShutdown();
thermalManager.getBedTemperature();
thermalManager.setBedTemperature();
thermalManager.setHotendTemperature();
thermalManager.getHotendTemperature();

of course, "thermals" are long underwear

The things a non native English speaker learns on a coding website. :-P

@jbrazio jbrazio added this to the 1.1.0 milestone May 4, 2016
@thinkyhead thinkyhead force-pushed the rc_singletons_plus_temperature branch from abc9436 to 084f6b5 Compare May 5, 2016 02:42
@thinkyhead
Copy link
Member Author

thinkyhead commented May 5, 2016

s/temperature\.([a-zA-Z_]{4,})/thermalManager.$1/ it is, then.

@thinkyhead thinkyhead merged commit 4f6120f into MarlinFirmware:RCBugFix May 5, 2016
@thinkyhead thinkyhead deleted the rc_singletons_plus_temperature branch May 5, 2016 03:41
@thinkyhead
Copy link
Member Author

thinkyhead commented May 5, 2016

May The Force be with this PR. ⚡

thinkyhead added a commit that referenced this pull request May 5, 2016
Follow-up the PR #3643(Temperature singleton)
nomukaiki pushed a commit to nomukaiki/Marlin that referenced this pull request May 6, 2016
* RCBugFix:
  Use AxisEnum with _lcd_babystep()
  Localize babystepping in the Temperature class
  Follow-up the PR MarlinFirmware#3643(Temperature singleton)
  Temperature singleton class
  Disable THERMAL_PROTECTION_BED with no sensor
  Can't use the ENABLED macro as a boolean
  Moved DELTA radius/rod default trimmer values to Conditionals.h
@thinkyhead thinkyhead mentioned this pull request Jul 8, 2016
CONSULitAS pushed a commit to CONSULitAS/Marlin-K8200 that referenced this pull request Aug 18, 2016
Follow-up the PR MarlinFirmware#3643(Temperature singleton)

・Change from fanSpeedSoftPwm[0] to thermalManager.fanSpeedSoftPwm[0] in planner.cpp
It fix compilation error when FAN_SOFT_PWM is enabled.

・Remove declaration of setExtruderAutoFanState() in temperature.h
Because that function was abolished.

・Change from babystepsTodo to thermalManager.babystepsTodo in ultralcd.cpp
It fix compilation errors when BABYSTEPPING is enabled.
drewmoseley pushed a commit to drewmoseley/Marlin that referenced this pull request Nov 8, 2023
PFW-1358 More button utilizes text and an empty character on the right
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants