Skip to content

Comments

Fix e jerk calculations for linear advance and junction deviation#11118

Merged
thinkyhead merged 1 commit intoMarlinFirmware:bugfix-2.0.xfrom
gloomyandy:fix-ejerk-calc
Jun 27, 2018
Merged

Fix e jerk calculations for linear advance and junction deviation#11118
thinkyhead merged 1 commit intoMarlinFirmware:bugfix-2.0.xfrom
gloomyandy:fix-ejerk-calc

Conversation

@gloomyandy
Copy link
Contributor

Description

This PR fixes errors in the calculation of the max e jerk setting used when linear advance is active along with junction deviation. The previous implementation was based on an incorrect definition. This version has replaced the previous split calculation to avoid calling SQRT (required by the corrected version) repeatedly. Instead it computes the full e jerk value whenever the contributing factors change.

Benefits

E jerk is now correctly calculated

Related Issues

See issue: #9917

@gloomyandy
Copy link
Contributor Author

Cleaned up version of previous PR. Code is the same but hopefully the git state is better.

@thinkyhead thinkyhead force-pushed the fix-ejerk-calc branch 2 times, most recently from bc31872 to 4a68b7b Compare June 26, 2018 18:48
Copy link
Member

@thinkyhead thinkyhead left a comment

Choose a reason for hiding this comment

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

Consider my comment about _EINDEX and if there's still an issue here then make the appropriate changes.

Copy link
Member

Choose a reason for hiding this comment

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

This cannot be changed, because _EINDEX relies on the extruder parameter passed to _populate_block. We cannot use active_extruder, which could very well have been changed before this function was called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that for the calculation to be correct the max_acceleration_mm_per_s2 term needs to be SQRT(max_acceleration_mm_per_sz[_EINDEX]), but I assume that it is not ideal to be calling SQRT here? Is this the case?

If it is undesirable to call SQRT in this context would it be acceptable to create an array of pre-computed SQRT values for max_acceleration_mm_per_s2 (updated if the underlying values is changed), one for each extruder? I'm not that familiar with the Marlin space/time trade offs but this would add (in the worst case) a second static array of floats with one entry per extruder. If you think this is OK I'll go ahead and modify the PR to do things that way.

Copy link
Contributor

@MagoKimbra MagoKimbra Jun 26, 2018

Choose a reason for hiding this comment

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

In my opinion the best thing is to use an array for max_e_jerk [EXTRUDERS], so calculate the value for each extruder with its corresponding acceleration and use the macro #define MAX_E_JERK max_e_jerk [active_extruder]

Copy link
Member

@thinkyhead thinkyhead Jun 26, 2018

Choose a reason for hiding this comment

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

@gloomyandy — A pre-computed array would do the trick. The array will only be needed when DISTINCT_E_FACTORS (and LIN_ADVANCE) is set. Otherwise a single computed value will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just pushed a change that should handle multiple extruders correctly. I did not special case the single extruder case though. Is the compiler smart enough to optimize a single element array accessed via constant value indices into just a single value?

Copy link
Member

Choose a reason for hiding this comment

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

When DISTINCT_E_FACTORS is disabled, no array is required, and max_acceleration_mm_per_s2 only has a single E entry. So I've patched the code to properly handle this case.

Copy link
Member

@thinkyhead thinkyhead Jun 26, 2018

Choose a reason for hiding this comment

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

That's the same as this:

#define MAX_E_JERK max_e_jerk[extruder]

@thinkyhead thinkyhead merged commit 3b3029c into MarlinFirmware:bugfix-2.0.x Jun 27, 2018
@gloomyandy gloomyandy deleted the fix-ejerk-calc branch June 27, 2018 06:37
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