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

🔧 Enable EDGE_STEPPING by default with TMC drivers #27292

Conversation

thisiskeithb
Copy link
Member

@thisiskeithb thisiskeithb commented Jul 19, 2024

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
Copy link
Contributor

ellensp commented Jul 20, 2024

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
Member 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
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
Member

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

@thisiskeithb
Copy link
Member Author

thisiskeithb commented Jul 20, 2024

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
Copy link
Contributor

ellensp commented Jul 20, 2024

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
Copy link
Contributor

ellensp commented Jul 20, 2024

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
Copy link
Contributor

ellensp commented Jul 20, 2024

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
Member 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
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
63 checks passed
@thisiskeithb thisiskeithb deleted the pr/enable_edge_stepping_default branch July 22, 2024 01:59
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.

None yet

3 participants