Skip to content

馃敡 Enable EDGE_STEPPING by default with TMC drivers - #27292

Merged
thinkyhead merged 2 commits into
MarlinFirmware:bugfix-2.1.xfrom
thisiskeithb:pr/enable_edge_stepping_default
Jul 22, 2024
Merged

馃敡 Enable EDGE_STEPPING by default with TMC drivers#27292
thinkyhead merged 2 commits into
MarlinFirmware:bugfix-2.1.xfrom
thisiskeithb:pr/enable_edge_stepping_default

Conversation

@thisiskeithb

@thisiskeithb thisiskeithb commented Jul 19, 2024

Copy link
Copy Markdown
Contributor

Description

Enable EDGE_STEPPING by default. This was originally planned to be enabled by default after a test period back in 2019, but never happened (source).

This is something I enable on my builds and haven't run into any issues. It's been around long enough that I think it should be enabled by default.

Requirements

Any TMC-based config.

Benefits

Better defaults for TMC-based configs.

Configurations

Any TMC-based config.

@ellensp

ellensp commented Jul 20, 2024

Copy link
Copy Markdown
Contributor

I disagree with doing this, default is for 'lowest' spec everything. ie in this case A4988's which do not support edge stepping.

@thisiskeithb

Copy link
Copy Markdown
Contributor Author

I disagree with doing this, default is for 'lowest' spec everything. ie in this case A4988's which do not support edge stepping.

Good thing this feature is guarded by HAS_TRINAMIC_CONFIG.

@thinkyhead

Copy link
Copy Markdown
Member

Good thing this feature is guarded by HAS_TRINAMIC_CONFIG

Options enabled in the configs should reflect the real configuration as closely as possible, regardless of how the conditionals might adjust them, especially for "add-on" features. So if there are no Trinamic drivers, this should be disabled.

I do think it makes sense to throw a #warning, or even an #error when users forget to set this option in the presence of Trinamic steppers, but still allow the option to be disabled for development and testing purposes.

@thinkyhead

Copy link
Copy Markdown
Member

Anyway, the default of EDGE_STEPPING enabled does make sense for this case since it's inside the guards.

@thisiskeithb

thisiskeithb commented Jul 20, 2024

Copy link
Copy Markdown
Contributor Author

So if there are no Trinamic drivers, this should be disabled.

That's exactly what happens when there are no (smart) Trinamic drivers enabled since HAS_TRINAMIC_CONFIG will be false.

@ellensp

ellensp commented Jul 20, 2024

Copy link
Copy Markdown
Contributor

How many example configs need updating as they now fail to build with this new #error? Needs checking...
We can already see it breaks CI tests that didn't enable EDGE_STEPPING (or the override) as now needed

@ellensp

ellensp commented Jul 20, 2024

Copy link
Copy Markdown
Contributor

My initial reluctance was due to missing that EGDE_STEPPING is in a #if HAS_TRINAMIC_CONFIG block.
I no longer have any concerns other than needing to update CI tests and example configs before this is merged.

@ellensp

ellensp commented Jul 20, 2024

Copy link
Copy Markdown
Contributor

I do wonder if "we" should just go one step further.

Remove edge stepping option entirely from the the Configs and just replace it in code with HAS_TRINAMIC_CONFIG tests

Ie why would you ever want or need to disable it?

@thisiskeithb

Copy link
Copy Markdown
Contributor Author

Remove edge stepping option entirely from the the Configs and just replace it in code with HAS_TRINAMIC_CONFIG tests

I'd be fine with that as well. Just hold changes until after 2.1.3 in case there's some unexpected issue that we've yet to run into.

thinkyhead added a commit to MarlinFirmware/Configurations that referenced this pull request Jul 20, 2024
@thisiskeithb thisiskeithb changed the title 馃敡 EDGE_STEPPING enabled by default 馃敡 Enable EDGE_STEPPING by default with TMC drivers Jul 20, 2024
@thinkyhead

Copy link
Copy Markdown
Member

I think it's important to keep the option because it's a feature of the steppers drivers and it's nice to keep it exposed.

@thinkyhead
thinkyhead merged commit 6afaf00 into MarlinFirmware:bugfix-2.1.x Jul 22, 2024
@thisiskeithb
thisiskeithb deleted the pr/enable_edge_stepping_default branch July 22, 2024 01:59
rngkll pushed a commit to rngkll/Configurations that referenced this pull request Oct 24, 2024
EvilGremlin pushed a commit to EvilGremlin/Marlin that referenced this pull request May 15, 2025
Co-authored-by: Scott Lahteine <thinkyhead@users.noreply.github.com>
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