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

Fix minor INPUT_SHAPING config bugs #26882

Merged
merged 8 commits into from
Apr 2, 2024
Merged

Fix minor INPUT_SHAPING config bugs #26882

merged 8 commits into from
Apr 2, 2024

Conversation

Sophist-UK
Copy link
Contributor

Description

  1. Stepper::set_shaping_frequency takes freq as a float, so SHAPING_FREQ_X/Y should be defined in Configuration_adv.h as floats.

  2. SHAPING_FREQ_X/Y can be zero to indicate disabled. SanityChecks need to reflect this.

Requirements

No.

Benefits

Improved configuration hints.

Configurations

N/a

Related Issues

None.

@Sophist-UK
Fix minor INPUT_SHAPING config bugs
fe63324 
1. `Stepper::set_shaping_frequency` takes freq as a float, so SHAPING_FREQ_X/Y should be defined in `Configuration_adv.h` as floats.

2. SHAPING_FREQ_X/Y can be zero to indicate disabled. SanityChecks need to reflect this.
@Sophist-UK
Avoid division by zero if SHAPING_FREQ_* is zero for disabled.
688c39c
@thisiskeithb
Copy link
Member

thisiskeithb commented Mar 18, 2024 •
edited
Loading

2. SHAPING_FREQ_X/Y can be zero to indicate disabled. SanityChecks need to reflect this.

You can already compile IS in by default by enabling & setting SHAPING_MIN_FREQ to a value like 10/below any frequency that would affect a print. This allows you set INPUT_SHAPING_X/INPUT_SHAPING_Y to 0.

edit: Per the conversation over at #22547 (comment), Marlin needs to know how much ram to allocate to IS buffers at compile time and that is done through SHAPING_MIN_FREQ when you want to compile IS in, but leave it disabled.

Your changes to sanity checks should be reverted.

@Sophist-UK
Copy link
Contributor Author

On reflection, and prompted by Keith's @thisiskeithb comment, the RAM allocation is based on the minimum frequency, and we should not allow you to set SHAPING_FREQ_X/Y to zero and not specify a SHAPING_MIN_FREQ and then default MIN_FREQ to 1.

So I will revert part of the sanity checks changes so that you have to set a MIN_FREQ if you want to enable shaping but set it to zero.

@Sophist-UK
Revert "Avoid division by zero if SHAPING_FREQ_* is zero for disabled."
81b11d8 
This reverts commit 688c39c.
@Sophist-UK
Revert sanity checks for SHAPING_MIN_FREQ required for SHAPING_FREQ_X…
a13bc21 
…/Y=0
@Sophist-UK
Revert remaining sanity checks
ae2f003 
to allow user to save RAM by setting a higher MIN-FREQ in the expectation that actual speeds will not be at the stated maximum feedrate.
@thinkyhead
Copy link
Member

The main reason to define config options with a decimal point is to give a hint to Auto Build Marlin that these values are float, not int, but otherwise the program should be able to compile whether decimal points are used or not. If there was an #if statement sanity-check that referred to these values, they would just need to be modified to a static_assert.

@thinkyhead thinkyhead force-pushed the fix_bug_input_shape_freq_equals_zero branch from b9758a4 to 3dedc40 Compare March 22, 2024 22:23
@thinkyhead thinkyhead requested a review from tombrazier March 22, 2024 22:32
tombrazier
tombrazier approved these changes Mar 23, 2024
View reviewed changes
Copy link
Contributor

@tombrazier tombrazier left a comment

Choose a reason for hiding this comment

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

The changes look good to me.

@Sophist-UK
Copy link
Contributor Author

@thinkyhead Thanks for fixing this up.

thinkyhead added a commit to MarlinFirmware/Configurations that referenced this pull request Mar 24, 2024
@thinkyhead @Sophist-UK
📝 Decimal SHAPING_FREQ_* (ABM type hints)
ef84ca1 
MarlinFirmware/Marlin#26882

Co-Authored-By: Sophist <3001893+Sophist-UK@users.noreply.github.com>
@thinkyhead thinkyhead merged commit 60f2837 into MarlinFirmware:bugfix-2.1.x Apr 2, 2024
61 checks passed
smiksky added a commit to smiksky/Marlin that referenced this pull request Apr 4, 2024
@smiksky
Squashed commit of the following:
848274f 
commit 17dfe8e
Author: thinkyhead <thinkyhead@users.noreply.github.com>
Date:   Wed Apr 3 00:20:46 2024 +0000

    [cron] Bump distribution date (2024-04-03)

commit 7b6b6e1
Author: Scott Lahteine <thinkyhead@users.noreply.github.com>
Date:   Mon Apr 1 21:44:18 2024 -0500

    🩹 Revert motion change

commit 60f2837
Author: Sophist <3001893+Sophist-UK@users.noreply.github.com>
Date:   Tue Apr 2 03:30:04 2024 +0100

    🔧 Minor INPUT_SHAPING config fixes (MarlinFirmware#26882)

    Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>

commit ef5fb39
Author: thinkyhead <thinkyhead@users.noreply.github.com>
Date:   Tue Apr 2 00:20:47 2024 +0000

    [cron] Bump distribution date (2024-04-02)

commit 87e94f4
Author: Andrew <18502096+classicrocker883@users.noreply.github.com>
Date:   Mon Apr 1 16:05:11 2024 -0400

    🚸 Update ProUI Plot graph - part 2 (MarlinFirmware#26563)

    Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>

commit 017a903
Author: thinkyhead <thinkyhead@users.noreply.github.com>
Date:   Mon Apr 1 00:23:48 2024 +0000

    [cron] Bump distribution date (2024-04-01)

commit 466282f
Author: Scott Lahteine <thinkyhead@users.noreply.github.com>
Date:   Sun Mar 31 18:20:37 2024 -0500

    🩹 Misc. changes from ProUI / ExtUI updates (MarlinFirmware#26928)

commit 1759429
Author: thinkyhead <thinkyhead@users.noreply.github.com>
Date:   Thu Mar 28 00:20:53 2024 +0000

    [cron] Bump distribution date (2024-03-28)

commit 0b9f487
Author: Jordan Stocker <Pvthaggard@gmail.com>
Date:   Thu Mar 28 06:14:03 2024 +1030

    🔨 Fix binary upload firmware path (MarlinFirmware#26909)

commit d3e1a92
Author: Chris <52449218+shadow578@users.noreply.github.com>
Date:   Wed Mar 27 20:39:54 2024 +0100

    🔨 Fix HC32 preflight (MarlinFirmware#26912)

commit cd357b0
Author: Holger Mößinger <hm2dev@users.noreply.github.com>
Date:   Wed Mar 27 20:38:08 2024 +0100

    🔧🚸 Tweaks for (MiniRambo) CNC (MarlinFirmware#26892)

    Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>

commit d0d229e
Author: thinkyhead <thinkyhead@users.noreply.github.com>
Date:   Mon Mar 25 00:21:58 2024 +0000

    [cron] Bump distribution date (2024-03-25)

commit eb897e6
Author: Ikko Eltociear Ashimine <eltociear@gmail.com>
Date:   Mon Mar 25 03:49:25 2024 +0900

    📝 Fix Cutter.md typo (MarlinFirmware#26901)

commit 825ebfd
Author: thinkyhead <thinkyhead@users.noreply.github.com>
Date:   Sun Mar 24 00:23:21 2024 +0000

    [cron] Bump distribution date (2024-03-24)

commit 3ee3964
Author: John Robertson <john@cirtech.co.uk>
Date:   Sat Mar 23 00:57:23 2024 +0000

    🐛 Fix ESP32 laser M4 exception (MarlinFirmware#26884)

commit aecfb25
Author: Sophist <3001893+Sophist-UK@users.noreply.github.com>
Date:   Sat Mar 23 00:27:13 2024 +0000

    🚸 Hide auto-run as needed (MarlinFirmware#26853)

commit 896a6a9
Author: thinkyhead <thinkyhead@users.noreply.github.com>
Date:   Sat Mar 23 00:20:15 2024 +0000

    [cron] Bump distribution date (2024-03-23)

commit 075f96d
Author: Holger Mößinger <hm2dev@users.noreply.github.com>
Date:   Fri Mar 22 22:36:42 2024 +0100

    ✏️ Fix stepper MS pin typos (MarlinFirmware#26891)

commit 0589655
Author: thinkyhead <thinkyhead@users.noreply.github.com>
Date:   Mon Mar 18 00:21:39 2024 +0000

    [cron] Bump distribution date (2024-03-18)

commit ea6a891
Author: Manuel McLure <manuel@mclure.org>
Date:   Sun Mar 17 12:23:12 2024 -0700

    ✨ MAX7219_REINIT_ON_POWERUP (MarlinFirmware#26163)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tombrazier tombrazier tombrazier approved these changes

Assignees
No one assigned
Labels
C: Configuration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Sophist-UK @thisiskeithb @thinkyhead @tombrazier