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

Make the simple 4 axis stepper control respect the axis inversion settings in Configuration_prusa.h #1263

Merged
merged 1 commit into from
Feb 15, 2022

Conversation

metacollin
Copy link

@metacollin metacollin commented Oct 18, 2018

Hi, I had heavily modified my MK2.5 printer by building a new, more rigid frame and swapping out the RAMBo mini for an Einsy. Part of all these modifications resulted in having to invert or not invert the axes differently from what the firmware would expect for a MK3 (or even a MK25 - I needed to invert all of my axes except for the extruder, opposite the MK25 and just plain wrong for the MK3).

This wouldn't be a problem, and it wasn't, except in one special case: the new "optical"/rastering XYZ calibration. I peeked at the code and realized that the simple 4 axis controller, in sm4.h and sm4.c and used exclusively for this new XYZ calibration, didn't actually respect the axis inversion or non inversion flags in the Configuration_prusa.h header.

I modified sm4.c such that it would respect these flags, and did my best to preserve the low cycle counts required for the various functions. There is an unavoidable timing increase in the sm4_set_dir function due to the comparison, but it doesn't seem to impact the calibration any a meaningful way. In fact, it makes the raster scanning part of the calibration noticeably quieter and smoother (I assume there are a large number of z-axis direction changes that are taking place and the added cycle time moves the direction changes a bit further from some resonance or something like that). The total time it takes to scan the region around a calibration point is certainly slightly longer, but the difference is so small that I couldn't notice any difference in the time required.

I know this pull request will have no effect for any of the supported printer configurations, but it would be extremely helpful to users like to modify their printers.

Plus, if or when the day arrives that Prusa Research engineers are developing some new product or tweaking an old one or whatever else, and they need to invert an axis... Well, this pull request ensures that their change doesn't immediately break XYZ calibration, and much potential frustration is now avoided.

Even if it isn't actually pulled into the code, at least it will be here in case someone finds it useful as a reference in the future.

Thank you for taking the time to look at this request!

@3d-gussner 3d-gussner mentioned this pull request Nov 2, 2018
@Lino77
Copy link

Lino77 commented Nov 4, 2018

Thanks exactly what I needed

@3d-gussner
Copy link
Collaborator

@PavelSindler This is a great bug-fix and i don't understand why Prusa didn't merged it yet.

@Dolnor Dolnor mentioned this pull request Feb 12, 2019
@3d-gussner
Copy link
Collaborator

@PavelSindler @mkbel Any update on this pull request as it works great and fixes a hard coded issue/bug

@leptun
Copy link
Collaborator

leptun commented Mar 28, 2019

Well for some reason adding these modifications to the latest branch results in random segmentation faults and missing _PRI_LANG_SIGNATURE on MK2_rambo13a multi-language. This is very weird since it shouldn't affect the languages in any way, but it does. I'll try to fix the issue and make a pull request to your branch if I succeed fixing it.

@metacollin
Copy link
Author

I can't reproduce this. And there is no way this could happen anyway. Sorry, but I think you're mistaken and have misattributed the problem to these changes.

The problems you describe are what happens when you incorrectly build the multilanguage version. Are you building it using the PR-build.sh? You won't get a viable multilanguage firmware if you don't build it using the instructions specific to PR-build (only supported on linux and windows, and requires some additional setup. See the README.md).

@cimoalpacino
Copy link
Contributor

I also spent a lot of time to modify the FW in sm4.c to work with my Arduino RAMPS board.
It was a pain and can definitely agree that it would be a great feature to not have it hardcoded like it is now

https://github.com/cimoalpacino/Prusa-Firmware/blob/MK3_3.6.0_ARDUINO_RAMPS/Firmware/sm4.c

@leptun
Copy link
Collaborator

leptun commented Mar 29, 2019

@metacollin Yes, you are right. Even without the modification it fails compiling some of the printers randomly. I am using PF-build.sh. I tried it again in a virtual machine and this time there was no problem. Maybe reinstalling git and the modifications to mingw will fix this. Thanks for pointing this out.

@3d-gussner
Copy link
Collaborator

@PavelSindler @mkbel @DRracer Is there any reason this PR didn't make it yet? I use it for months and just works great.

@metacollin
Copy link
Author

I think that it might be very low priority as it doesn't actually have any impact on the actual Prusa printers. And honestly, I wouldn't really blame them. They're job is to support Prusa hardware, and this PR doesn't fix anything on Prusa hardware. But they are leaving it open so the fix is at least available to other people. shrug. :shipit:

@3d-gussner
Copy link
Collaborator

@metacollin Their priority might be low for this one, but in my opinion it is a bug. Hardcode it in one part and use 'INVERT_?_DIR' every else isn't helping. Just imaging they decide to turn a motor for any reason, test its movement and it is working. But MBL may be off ... searching begins .... and that gets even harder when months/years past by and tons of commits are done.

@DRracer DRracer changed the base branch from MK3 to MK3_3.9.3 February 9, 2021 06:50
@DRracer DRracer changed the base branch from MK3_3.9.3 to MK3 February 9, 2021 06:50
@DRracer
Copy link
Collaborator

DRracer commented Feb 9, 2021

@wavexx please verify this PR in relation to your latest changes in sm4.* and twi.*

@DRracer DRracer removed the FW 3.10.x label Apr 21, 2021
@DRracer DRracer added this to the FW 3.11.0 milestone Apr 21, 2021
Copy link
Collaborator

@leptun leptun left a comment

Choose a reason for hiding this comment

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

I've tested these changes on the latest MK3 branch and they work correctly. Just finished an XYZ calibration on a stock MK3S. It remains to be tested also on a MK2.5S. If that printer passes the calibration, then I say the PR is ready to be merged

#define ZDIR INVERT_Z_DIR:!INVERT_Z_DIR
#define EDIR INVERT_E0_DIR:!INVERT_E0_DIR

uint8_t dir_mask = 0x0F^(INVERT_X_DIR | (INVERT_Y_DIR << 1) | (INVERT_Z_DIR << 2) | (INVERT_E0_DIR << 3));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Even though this is a non-constant variable that is declared globally, the compiler manages to optimize it completely because it is never written to it. So it works just as well as a define

Copy link
Collaborator

@DRracer DRracer left a comment

Choose a reason for hiding this comment

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

Approving after checking the expressions in both BOARD_RAMBO_MINI_1_0 and BOARD_EINSY_1_0a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants