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

Improve comments on DEFAULT_MAX_FEEDRATE #26875

Open
wants to merge 2 commits into
base: bugfix-2.1.x
Choose a base branch
from

Conversation

Sophist-UK
Copy link
Contributor

Description

This is a documentation-only PR to improve comments relating to DEFAULT_MAX_FEEDRATE in order to reflect the impact of XY values on INPUT_SHAPING ram usage.

There are ZERO code changes in this PR.

Requirements

No.

Benefits

Improved documentation, easier configuration by users, avoiding research and false starts at alternative memory optimisations.

Configurations

N/A

Related Issues

None.

... to reflect impact of XY values on INPUT_SHAPING ram usage.
@classicrocker883
Copy link
Contributor

technically Note: should be NOTE: like the others.

@Sophist-UK
Copy link
Contributor Author

@classicrocker883

Actually in Configuration.h there are 8 cases of "NOTE:" and 10 cases of "Note:" (excluding this one).

I have no axe to grind either way - and since this is a documentation only PR I don't mind whether I leave it as is, change just this new "Note:" to "NOTE:", or make the entire file consistent to one or the other.

But since it is unclear, I won't do anything until there is a consensus on which way to go.

@thisiskeithb
Copy link
Member

thisiskeithb commented Mar 26, 2024

Adding a note to the Input Shaping-specific section (or Marlin’s hosted docs instead if not already mentioned) might be a better idea since we can call out other config items that can affect RAM usage like microstepping, steps/mm, etc. instead of sprinkling Input Shaping comments all over the config files.

@Sophist-UK
Copy link
Contributor Author

@thisiskeithb Yes - I fully understand the issue of comments creep. However I submitted this based on my own learning experience, whereby despite having read the online docs before enabling input shaping, it took me a while to work out why my firmware was rebooting. I still believe that a hint here can be very beneficial to users, though I can certainly shorten it to one line that summarises the issue and points to documentation.

@bmourit
Copy link

bmourit commented Apr 14, 2024

This note is good in that is says this effects RAM usage, but it doesn't explain how. So for someone with ram issues, should they increase or decrease here.
This is a common piece of info missing in all the ram usage warnings in Marlin that makes them minimally useful.

@bmourit
Copy link

bmourit commented Apr 15, 2024

Thanks for this! I know from a users perspective it is easy to assume lower value always decrease ram usage, but this isn't aways the case, which can be confusing. For example when the configurable values represent precision thresholds. IMHO this is very helpful.

@thisiskeithb
Copy link
Member

I still think that adding a note to the Input Shaping-specific section (or Marlin’s hosted docs instead if not already mentioned) would be a better idea since we can call out other config items that can affect RAM usage like microstepping, steps/mm, etc. instead of sprinkling Input Shaping comments all over the config files.

@Sophist-UK
Copy link
Contributor Author

@thisiskeithb I genuinely understand where you are coming from:

  1. Because the code could get inundated with comments addressing tiny minor issues; and equally
  2. Because the Input Shaping web page should indeed have everything there is to know about it (which I agree with).

However, I personally feel that this area is important enough to justify both a code comment and something on the Input Shaping web page.

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

4 participants