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.

Sign up for GitHub

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

Jump to bottom

MPC differential tuning bug fixes #27274

Merged
merged 4 commits into from
Aug 5, 2024
Merged

MPC differential tuning bug fixes #27274

merged 4 commits into from
Aug 5, 2024

Conversation

tombrazier
Copy link
Contributor

@tombrazier tombrazier commented Jul 15, 2024 •
edited by thisiskeithb
Loading

Description

This PR fixes several problems with the MPC differential tuning method.

  • Moved option to enable MPC debugging to Configuration.h.
  • Fixed derivative calculation (was delta temp * delta time, should have been delta temp / delta time, worked anyway because delta time happened to be 1).
  • Fixed time calculation for when maximum rate is observed. The return value of get_elapsed_heating_time() was not yet initialised at the point where it was being called.
  • Fixed call to tuner.get_time_fastest()) which should have been tuner.get_temp_fastest()). (See equation 17 at https://marlinfw.org/docs/features/model_predictive_control.html.)
  • Added a second check for nonsensical MPC_BLOCK_HEAT_CAPACITY and MPC_SENSOR_RESPONSIVENESS values after the "Refining estimates..." step. This could happen when using the automatic tuning method which attempts an asymptotic tune and then falls back to differential if the results don't make sense. See [BUG] M306 T produces negative responsiveness value #27181.

Requirements

Enable MPCTEMP.

Benefits

Differential tuning now works and it is more likely that the automatic tuning method will fall back to differential tuning when needed. In particular this should make M306 T work seamlessly for Revo hotends.

Configurations

Enable MPCTEMP.

Related Issues

@thisiskeithb thisiskeithb linked an issue Jul 15, 2024 that may be closed by this pull request
[BUG] M306 T produces negative responsiveness value #27181
Closed
1 task
@tombrazier
Copy link
Contributor Author

Build error related to FT_MOTION, not this PR.

@thisiskeithb
Copy link
Member

Build error related to FT_MOTION, not this PR.

Breaking changes were reverted in ade05c0, so merge in the latest bugfix-2.1.x or rebase your PR & force-push changes.

@thinkyhead
Copy link
Member

Breaking changes were reverted…

NOTE: A PR doesn't need to be updated if its changes don't overlap with changes to bugfix-2.1.x. The CI tests are done by merging the changes from the PR onto the current bugfix-2.1.x at the time the tests are initiated. If the bugfix-2.1.x branch has a compile error and then it is fixed, you don't have to take extra steps to update your branch if there are no conflicting changes; just re-run the tests.

@thisiskeithb
Copy link
Member

just re-run the tests.

That only works if you have proper repo permissions, so merging in the latest changes works universally for everyone.

@thinkyhead
Copy link
Member

thinkyhead commented Jul 18, 2024 •
edited
Loading

Hmm, where did we put that "re-run yer own bloody PR's tests" permission? That seems like something we should have.

thinkyhead added a commit to MarlinFirmware/Configurations that referenced this pull request Aug 5, 2024
@thinkyhead
✨ MPC_AUTOTUNE_DEBUG
dc8b710 
MarlinFirmware/Marlin#27274
@thinkyhead thinkyhead merged commit 851257c into MarlinFirmware:bugfix-2.1.x Aug 5, 2024
62 checks passed
@thisiskeithb thisiskeithb linked an issue Aug 6, 2024 that may be closed by this pull request
[BUG] Wrong MPC settings can cause thermal runaway #27253
Closed
1 task
@thisiskeithb thisiskeithb mentioned this pull request Aug 6, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
C: Temperatures F: Calibration PR: Bug Fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Wrong MPC settings can cause thermal runaway [BUG] M306 T produces negative responsiveness value
3 participants
@tombrazier @thisiskeithb @thinkyhead