G28 Or G28 X crashes X axis if the end-stop is in a triggerd state when homing
Home the X axis, with G28, for example my print end script does this to move the tool away from the print.
Send a G28 - I do this before every print.
Expected behavior: The Xaxis should detect the X endstop is in a triggerd state already and bump the end-stop, then Y then Z should probe.
Actual behavior: The X endstop does not bump it just tries to move the axis continuously into the already triggered endstop untill you send an emergency stop.
This is the third endstop I've tried. The endstop always works as long as it does not start homing while its in a triggered state. This is 100% reproduce-able every time.
HOWEVER If you shut the printer off while the X endstop is triggered, turn it back on and Home. It works as it should. This only happens after its homed at least once already.
Below is a video Showing it.
Please ignor the slight skipped steps on the X, Ive reduced the current dramatically to the bare minimum to move the axis so i dont chew up my belts showing this issue.
The video shows me homing the printer with G28. Then Homing the X axis with G28 X which parks the axis on the endstop leaving it in a triggered state. I then send G28 to show that instead of detecting its triggering and bumping the endstop it drives it into the end stop crashing the axis until I send M112.
Are you able to narrow down where in the code the error lies?
I wish i could @thinkyhead I am by no means a coder, I am very much a mooch of Marlin. I can tell you it happened After September 17th as that was my previous Pull. I try to move forward with the bugfix every couple of weeks so i can stay current-ish if i have to report a bug.
Please ZIP your configurations and attach them to a reply, as requested in the issue template.
Marlin_config.zip
Here you are.
Meanwhile, give this version (October 17) a test and see if the issue still exists: https://github.com/MarlinFirmware/Marlin/archive/c36773bffba9f5300764238a06a7cb9e04c315da.zip
@thinkyhead
void Endstops::enable(const bool onoff) {
enabled = onoff;
#if ENABLED(ENDSTOP_INTERRUPTS_FEATURE)
update();
#endif
}
I think this must be done also when no interrupt enabled because my last modification removed endstops status after home and they are not updated refreshed anymore when re-enabled (I use interrupt and it works fine)
@GhostlyCrowd try to remove #if #endif
lines in endstops.cpp fuction enable
Edit: but keep update
line
@thinkyhead even if this code:
// At this point, we must ensure the movement about to execute isn't
// trying to force the head against a limit switch. If using interrupt-
// driven change detection, and already against a limit then no call to
// the endstop_triggered method will be done and the movement will be
// done against the endstop. So, check the limits here: If the movement
// is against the limits, the block will be marked as to be killed, and
// on the next call to this ISR, will be discarded.
endstops.update();
should act the same...
Edit: this may work delayed because of filter threashold...
@thinkyhead
that build Does indeed act properly with my end stops
Yes i had to disable the endstop inttrupts because i have TMC2130's and for some reason with the latest TMC library it has conflicts with endstop inturrupts. There was an issue posted about it but only suggested disabling endstop inturrupts was the solution.
Edit, I am trying the code modification to endstops.cpp now
@GMagician might have the right idea. Try this one: https://github.com/thinkyhead/Marlin/archive/f96be3c3dd36a13d9de3aae0f831bc604a1c2b73.zip
If that works, I'll submit a patch shortly.
@thinkyhead I tried what you suggested, it did not work.
I downloaded and compiled what @GMagician has given you to give to me, it also did not work.
Still same issue.
The October 17th pull you sent me DID work properly.
Ok, since the version from Oct 17 worked, the change must have been after that. Please try this one, from October 28: https://github.com/MarlinFirmware/Marlin/archive/5ead0269676b80292a360e5b132204ebecdc40a5.zip
@thinkyhead October 28th Is broken, So something between October 17th and October 28th
@GhostlyCrowd and @thinkyhead I think my solution doesn't work because of ENDSTOP_NOISE_THRESHOLD
. Update
must be called I think ENDSTOP_NOISE_THRESHOLD+1 times before it really updates endstops status
@GhostlyCrowd you may try this to confirm my suspicion.
Inside Enable
function write:
update();
endstop_poll_count=1;
update();
this is no a solution but if it works then that's the way...
@GMagician I cannopt get it to compil adding
the code you suggested to endstop.cpp
Marlin\src\module\endstops.cpp: In static member function 'static void Endstops::enable(bool)':
Marlin\src\module\endstops.cpp:273:8: error: 'Update' was not declared in this scope
Update();
^
@GhostlyCrowd sorry I mis typed is all lowercase
@GMagician That did indeed fix the problem
Is this a dirty work around or is it the fix?
That just prove my doubt and works only for configurations with ENDSTOP_NOISE_THRESHOLD
enabled, this can't ever be called 'dirty work around' it's worst than that ;-)
@GMagician Ahhh i understand, so whats the underlying issue you suspect?
The problem is that to let machine works after a home, endstops status is cleared. In code when a new movement is inserted in planner there is a call to update endstops status but works only for "interrupt" mode, when you have ENDSTOP_NOISE_THRESHOLD
, update must be called ENDSTOP_NOISE_THRESHOLD + 1 times to really update endstops but this call is done in temperature polling routine (1KHz) but it's too slow and will refresh endstop status too late.
My 'dirty' code speeds things up but I think that some different solution must be found.
Since this "delay" is done to filter noise is not either correct to update status like I did...maybe the enable
call should wait all debounce time before move over (but in planner can't be done) but it's also true that if update is always called when endstops are enabled maybe is not needed anymore in planner.
makes sense why i didnt notice this until i had to disable the endstop interrupts because of the TMC library conflicting with it for some odd reason.
@thinkyhead is it "legal" to replace update
with something like forceupdate
that wait until endstop_poll_count
is 0?
eg:
FORCE_INLINE void forceupdate() {
#if ENDSTOP_NOISE_THRESHOLD
do { update(); } while (endstop_poll_count);
#else
update();
#endif
}
and remove update
call inside planner? I think that this last call is not needed because of "always" up to date endstops status when they they are enabled (even with interrupts)
Ok so @GMagician your work around seemed to work for homing while the end stop is triggered but it has an adverse effect in the fat that now when my print finishes and it tries to park the X axis with G28 X its not seeing the endstop trigger and its crashing after the print now.
Edit: I'm doing another fast print now to verify this.
@GhostlyCrowd that should not affect G28.
@GMagician You're correct I forgot to increase my stepper current after doing all these tests and it was the sound of it losing steps when bumping. Just did another print to verify.
@GhostlyCrowd try replacing update
call in enable
with code proposed for forceupdate
. I'm not sure such blocking code can be done without a safedelay
? call
@GMagician I can do this tomorrow morning for you to test. I have left my workshop for the day.
is it "legal" to replace update with something like forceupdate that wait until
endstop_poll_count
is 0?
I think this is starting to look like a hack to work around something fundamentally broken by the earlier update. @ejtagle worked hard to get the endstops checking correct, and the recent change didn't take into account the theory of its mechanism. I would recommend you revert back to before the change was made and reconsider what you were trying to accomplish with the recent change.
@thinkyhead ... you are right. We always (sometimes me too!) fail to get the whole picture here. The problem stated exists, but the fix must be done at a higher level - The stepper and the endstops class is not the problem.
How does this thing work ?
1) If ENDSTOPS_INTERRUPTs is disabled:
-You just get polled endstops every 1khz - If there is NOISE FILTER enabled, then you can get a variable delay between switch press and actually taking it into account, otherwise actions are taken inmediately
2) If ENDSTOPS_INTERRUPTs is enabled, and no filtering, then the switchs are read and action is taken inmediately
3) If ENDSTOPS_INTERRUPTs is enabled, and filtering,, then each interrupts triggers periodic polling (1khz) until values settle down. Then polling ends.
What makes things more complex is that polling or updating state is only done when the endstops are enabled.... That is what is usually causing issues, as endstops, unless ALWAYS_ENABLED, will not update on changes
Right, so… Instead of a simple enabled state, should we make it have a state 2 … where the endstops are being primed, and a state 1 … where the endstops are now being read for actual use?
We could try to keep just the state always updated, but not the actions.
That should be easy
That was how I previously tried to implement it. So if it's not that way now, it's a regression.
@ejtagle is it legal to havew interrupt and filter? If I wants interrupt is because I want fast response if I have noise no interrupt is logic to me...
@thinkyhead and @ejtagle we may also keep endstop always working when ENDSTOP_NOISE_THRESHOLD
is enabled (this is the only situation where status is delayed) but not copy live_state
to validated_live_state
until endstops are enabled
@GMagician : Yes, it is legal to have interrupts with noise filtering. You can think it as a timer. The interrupt is used to start the timer that polls the endstops. And the timer autoshutdowns when the endstops are stable
@thinkyhead : Most of the issues with the endstops are caused, by, let´s call it, edge triggered actions.
So, if updates are disabled (abort_enabled() returning false), then update() does not execute- There is a risk of losing edges (transitions from non triggered-triggered-non triggered) while disabled. (usually this is not a problem at all)
The other thing that sometimes has caused problems is that there is a delay between the endstop being triggered and Marlin considering it as triggered. This small delay can cause race conditions if movements are too fast, or too short.
This specially holds true when enabling endstops, as there must be some time before endstops stabilize to their correct values!
What is the possible fix for the G28 command ? - Well, i think the best approach is to slightly modify the flow:
When an G28 command is executed, the first step should be to enable endstops, then perform an small wait (100mS) to allow the noise filter to stabilize, and THEN start the homing process. Alternatively, do a 10mS delay, then wait until endstop_poll_count equals 0.
That is probably the best approach. And 100mS of extra delay when homing is innoticeable! :+1:
So, if updates are disabled (
abort_enabled()
returningfalse
), thenupdate()
does not execute
This is not so, at least not before the recent change. Temperature::isr
calls poll()
and (when ENDSTOP_INTERRUPTS_FEATURE
is disabled) poll()
always calls update()
. And if ENDSTOP_NOISE_THRESHOLD
is set then update()
continues forward… Once the validated_live_state
is tested for, then update()
will return without triggering anything unless abort_enabled()
is true.
It seems to me that the testing of abort_enabled()
at the very start of update()
should not be there. The collection of the endstop state should be allowed to proceed no matter what. And then below, where it currently only does logic for ENDSTOP_NOISE_THRESHOLD
it should also have a block to do logic when ENDSTOP_NOISE_THRESHOLD
is not set. Specifically, just updating the live_state
.
@thinkyhead : I think you are right. State should always be kept. The actions is what we disable
That should remove the processing delays
The downside is that it means the Temperature ISR will always have the added overhead of checking all the endstop pins.
If you use the interrupt on change feature, update should ONLY be called when there is a change.
If you are not using that feature, you get continuous polling, so you are right...
I see your point. The other choice would be to keep it as it is right now, but add the delays i suggested everytime endstops are enabled
This delay could be added to endstops::enable() and endstops::enable_locally() and document them as "stabilization delays"
The forceupdate
approach also makes sense… Keep calling update()
until the polling count (if existing) goes down to 0. Then we know the endstop state is confirmed and recorded.
if you are not using that feature, you get continuous polling,
Currently you would only get continuous polling when using the threshold.
Basically yes. There is something strange here... the update() call should be cancelling any movement against endstops...
Perhaps the forceupdate
only needs to be called when turning endstops/probe off.
…and when the threshold is in use.
Maybe i know what is going on here...
update() is only called when a change of endstops happen (if ENDSTOPS_INTERRUPTS) enabled.
And, if a new movement is queued, update() will be called ONCE from the stepper ISR (because the endstop was already pressed and nothing is resetting the endstop_poll_count) - This is not enough due to the FILTER_THRESHOLD, that requires more calls in order to recognize the endstops as not triggered.
...
But again, the movement should never be against the endstops... unless.. Let´s assume endstops were disabled... in that case, the update() call from the stepper is not enough to cancel the movement...
True! So, have we confirmed the problem only occurs with ENDSTOP_INTERRUPTS
?
Oh, right. I see that the update
call from stepper is only done when first fetching the block.
So, do we make an exception for the call from stepper? i.e., Set the polling count to 1 before the call so that only one sample is needed before it's considered confirmed?
Certainly, without ENDSTOP_INTERRUPTs, there will be calls to update() from the temperature ISR, so movement should be cancelled.
The only "dangerous" combination is ENDSTOP_INTERRUPT and FILTER_THRESHOLD, because it is required to call several times update() to change the detected state... But still something does not make sense...
The only way this could happen is if the filter has to run for all FILTER_THRESHOLD to recognize that the endstops are pressed, BUT that should be done ...
Well also, the interrupt won't ever be generated if the switch is already pressed when the move starts. (Not a problem for sensorless homing.)
Exactly, but the state should already reflect at that point that the endstop is pressed
I've been auditing the procedure we currently follow. Maybe if I continue that process it will make more sense. Did you examine the recent change to see how it causes this issue to now manifest, where it didn't before?
Let me see if i can find the change...
If noone is touching the endstop state, then the endstop should be detected as pressed with just one call to update() from the stepper ISR
aa92022 (#12158)
This is incorrect, but the previous code is also incorrect.
If we clear live_state, and we have enabled FILTER_THRESHOLD, then the call to update() from the stepper ISR will never cancel the move.
The reason, as you stated (and you are right) is that there will never be an endstop change interrupt, so there is not enough calls to update to reflect the change of endstop state, and the movement will never be aborted
Should it only clear just not clear any of those states?hit_state
then, or
The reason of cleaning both, i guess was to allow a "window of time" with hit_state = 0
Because, if you don´t clean the valid_state, as soon as you call update(), hit_state will be set to the triggered endstop
I see. Well we call hit_on_purpose
when we really care about clearing the hit_state
. So I suppose the order in which we call not_homing
and hit_on_purpose
is important.
But certainly this is not correct. valid_state and endstop_poll_count should never be touched, because that interfers with the filter delay
I concur with that part. The only question, then, is should we "fake" an interrupt and start the polling count whenever re-enabling the endstops so it will start the process of validation?
Exactly! ,, The order is pretty important - you must call hit_on_purpose() when you are sure the endstops are NOT triggered - In the right moment of the calibration process
You don´t need to fake an interrupt. Just setting endstop_poll_count to FILTER_THRESHOLD is enough.
But if you don´t touch live_state, that is not required (because the valid_state will already contain the valid state, and there is no need to wait to detect endstops again)
This comment
// Still 'enabled'? Then endstops are always on and kept in sync.
// Otherwise reset 'live's variables to let axes move in both directions.
Means changing could break what that piece of code was supposed to be fixing. I can´t remember writing it myself...
Hey hit_on_purpose is not enough...If you work with interrupt enabled and no threshold and end homing over endstops what happens? live state report that axis is over endstops and if you move away from it live_state is never updated so what happens? you can only move in one direction
So setting endstop_poll_count
can be done in enable()
and enable_globally()
when the onoff
parameter is true
. The call to hit_on_purpose()
is best done after (but in practice, I think that means shuffling around all the routines that do endstop-enabled moves).
And then, should the removal of the #if ENABLED(ENDSTOP_INTERRUPTS_FEATURE)
in aa92022 also be reverted, or is that still sensible?
We used to only abort moves if the steppers were actually moving _towards_ the triggered endstop.
And then, should the removal of the #if ENABLED(ENDSTOP_INTERRUPTS_FEATURE) in aa92022 also be reverted, or is that still sensible?
Please don't revert it...'live' variables need to be reset after homing
@ejtagle the comment is part of my fix...don't forget what I said above. live_state needs to be cleared after home (or better after endstops are disabled) to avoid issues.
So setting endstop_poll_count can be done in
if you fake variable you lose filtering action
FFS — How about reverting to the endstop methodology from 1.1.8?
@thinkyhead : That is still true: Movements are only cancelled if going against endstops.
@GMagician : The problem was dual endstops ?
Personally I think @ejtagle approach is good it just need to be refined
@ejtagle with dual endstop the problem is present for sure but live state variable prevent moving over endstops and if it is left set I think can give issues also with single switch
I know what is going on... We abort only on movements against endstops, but there are cases when we need to run against the endstops: Dual endstops! or dual axis! ... You normally cancel movements against the limit switches if any limit switch triggers, except when homing each axis separately...
Ok, well…
ENDSTOP_NOISE_THRESHOLD
in place with ENDSTOP_INTERRUPTS_FEATURE
in #12373.forceupdate
actually works in all scenarios. Right now the code looks potentially wrong. (i.e., It should probably call update()
and safe_delay()
both in that while
loop.)You don´t need to call update() ... just setting endstop_poll_count to anything != 0 will make the temperature isr call update() for you at 1khz
Then that's better. But then, in some ways is that not like not-touching live_state
and validated_live_state
in the first place?
But, @thinkyhead , i am not sure if setting live_state to 0 is the right thing to do. The "time window" that it opens seems more like a workaround than a proper fix.
I changed forceupdate... now I call update first and then I do while calling safe_delay
Setting live_state to 0 gives FILTER_THRESHOLD milliseconds of no endstops triggered. And the code somehow depends on it to work... :O
Well, the idea there was just to let it get refreshed later, on the next enable of the endstops.
I set them to 0 when endstop check is disabled.. and since it is disabled variable is not updated anymore nor poll_count is updated
Not so. Look at update()
. I mean, setting poll_count
to 0 will prevent the update so long as the state stays the same. But the validation code does occur.
"bad" I made a wrong change...the idea is to call update to start filter engine and then wait it to elaps
(edited my prior comment)
Certainly is a time window of no endstops (i am not worried about 5 milliseconds, that is not the point) - There is something on the homing code that is not doing the waits in the right order...
I also remove update call from stepper because with this modify endstops are correctly updated and there should be no necessity to call it when starting a movement
Did we determine that the fundamental issue was that because no interrupt was being generated, there was no time after enabling endstops that the state would be updated?
Well to prevent any issue endstop filtering, when active, should always works even when not needed. Ortherwise if filter check is disabled, when you reenable it you still have to wait for it to properly resync
That is the reason: the lonely call to update() from the stepper ISR is not enough to detect endstops if FILTER_THRESHOLD is enabled and also using ENDSTOP_INTERRUPTS
…If so, then simply setting the poll count to some number upon enabling endstops/probe should allow the updates to start.
yes but you can't do anything else that keep status updated when you have threasold. faking poll_count there is not as good choice because you break filter action
Yes @thinkyhead . setting endstop_poll_count to FILTER_THRESHOLD makes sure state will update. But take note that could probably break @GMagician fix.
because the code right now sets an _infinite_ window of time of no endstops until the call to
It's worth a try, and I think it's also correct to set the live_state
/validated_live_state
to 0 in not_homing
.
sorry for my last post..not clear...faking poll_count is like not having filter at all (or may be said you may assume a wrong status)
It's worth a try, and I think it's also correct to set the live_state/validated_live_state to 0 in not_homing.
if you want to solve "only one direction move" after home
Basically, i am not sure if this will work at all when FILTER_THRESHOLD is disabled
Because…
#if !ENDSTOP_NOISE_THRESHOLD
if (!abort_enabled()) return;
#endif
it works it works and for sure original code doesn't
@ejtagle please check what happens with no thresold and interrupt enable when you terminate home with endstops pressed. live variables is set right? ok what happens if after home you move away endstops.....live variables are still set
but since stepper isr check these variables to block movements...you will never move other direction (against endstops)
Well, let's take the best elements of what we discussed and incorporate it into #12373, and be sure we understand all the potential code paths that may be taken. Remember that in our current implementation, the only thing that endstop interrupts do is prime the poll count.
Not arguing against that @GMagician . The problem is real. No doubt about it. Just that adding extra workarounds makes (again) code very difficult to maintain. You are introducing on purpose a race condition or a "temporal disabling" of endstops to solve a problem. Maybe that is the right decision, maybe a better aproach would be to find the root cause...
There is no race condition since endstops status is not updated when they are disabled. and forceupdate just wait until filter action has been done. Only issue is if noise is cointinuos. if will hang waiting stabilization
Oops, my mistake. Endstop interrupts seem to just call endstops.update
.
There is no race condition since endstops status is not updated when they are disabled.
Not so. The live state is constantly updated when the threshold is in play.
@thinkyhead they are not updated when interrupt is enabled and thresold isn't
Yes @thinkyhead . But calling update() is enough, as endstops are read, and endstop_poll_count will be set to FILTER_THRESHOLD, as there was a change ;)
Not race condition in the "traditional sense". There is no danger of variable updates. It is a race condition between the main thread and the temperature ISR call to update()
Ok, I see what you're both saying.
live variables should not be reset when thresold is active
To make it as clear as possible: @GMagician opens a "window of time of no endstops" and some code (the homing procedure) must complete before that "time window" closes. I don´t know what, probably part of the homing process
Are you referring to the changes in #12373?
window of time of no endstops
what code does this? I'm losing myself about what is problem of previous fix and current issue...sorry but it's hard since English is not my language
Yes... There was a reason for that call to update() inside the stepper - I would be the happiest man if that call can be removed (less time in the stepper)
But i find no reason for that call now (and i can´t remember the original reason). It was something related to homing, probably - I know blocks will not cancel inmediately without that call, but eventually they will, so it should work.
@GMagician : Resetting the filter state should never be done. It is a filter. You don´t want to pay the penalty of time of redetection of state - The reason you do the resetting is that you get "no endstop detection" for 5 milliseconds or more. That probably allows to queue a new movement and start executing it, and thus change the endstop state to not pressed
And that is the race condition i am talking about: You need the queuing to be done and start executing on that time, or the fix will not work
call in stepper is needed because of interrupt are not triggered when you are still over endstops. but why status is not updated? because you may disable endstops enabled after home. Then if status is restored when you reenabled them then no need for such call
if status is restored when you reenabled them then no need for such call
True. Of course note that this call to update
occurs only when the next block is fetched, so it's ok as long as endstops are enabled and update
is ready to go before planning a homing move.
So what are we left with? Do we see flaws with…
not_homing
setting [validated_]live_state
to 0 that breaks update
?not_homing
setting endstop_poll_count
to 0 that breaks update
?update
from the stepper ISR? <-- Apparently not needed…enabled
and enable_globally
needing to call update
(or forceupdate
)?endstop_poll_count
and just calling update
?Would this then be enough…?
void Endstops::force_update() {
#if ENDSTOP_NOISE_THRESHOLD
endstop_poll_count = 1; // get current state now
#endif
update();
}
```cpp
void endstop_ISR(void) { endstops.force_update(); }
```cpp
void Endstops::enable(const bool onoff) {
enabled = onoff;
forceupdate();
}
void Endstops::enable_globally(const bool onoff) {
enabled_globally = enabled = onoff;
forceupdate();
}
not_homing setting live_state to 0 that breaks update?
after homing, if endstops are not kept operative, is mandatory otherwise you can only move one direction
not_homing setting endstop_poll_count to 0 that breaks update?
same as before, to reset live if must be set to 0
the call to update from the stepper ISR? <-- Apparently not needed…
true if you are sure to keep endstop updated before starting move
enabled and enable_globally needing to call update (or forceupdate)?
yes if you want to be sure endstop is updated correctly after update has been stopped
@thinkyhead :
1) The general approach is correct
2) The disabling of endstop polling when endstops are disabled is desirable, but the "wake up" procedure needs to be tuned. We probably lost interrupts in the process, so we must set endstops_poll_count to a sensible value on wakeup
3) Something strange with the need to set live_state to 0. Maybe the homing procedure, maybe the dual axis setup that can´t move the 2nd axis when the min endstop is triggered?
4) setting endstop_poll_count to 0 means no endstop detection until the next change in switches (only the ISR can call update and reload endstop_poll_count) : That is the "no endstops" window i am talking about
The call to stepper.update() was required for the same problem: No ISR, no update of endstops, no cancellation of blocks.
I´d say:
force_update() should set endstop_poll_count to FILTER_THRESHOLD
endstop_ISR only needs to call update() (
the change to enable_globally() is OK
Something strange with the need to set live_state to 0.
Well, we do need to be able to allow endstops to always be on, and yet still be able to move away from a triggered endstop.
But still @GMagician should test if that solves the problem.
Probably the call to update() in stepper could be removed with these changes, as we are force_updating before moving
Something strange with the need to set live_state to 0.
Well, we do need to be able to allow endstops to always be on, and yet still be able to move away from a triggered endstop.
Already being done. You can always move in the opposite direction of an endstop, but the homing procedure of dual axis requires a different behaviour
…And I think the idea was that we didn't want to have an endstop interrupt occur and yet still have the old validated_live_state
fall-through, causing the move to get canceled.
Well, with dual endstops, I believe we treat both switches as a single bit in the hit_state
.
@thinkyhead Exactly. And that DOES NOT work for homing!
That is the issue! -- For normal operation, ANY endstop of the pair has to abort the movement!
But in homing, this is not true: We need to be able to move the other axis, even if one of the endstops is triggered!
Haven't we got flags to ignore one of the pair? I thought we had that solved.
@thinkyhead hit_state is not enough to let motors move.. live state is used to prevent step pins from being set
I see. We set hit_state
as part of canceling the move.
yes but live_state prevent further moves by blocking physical pins
Really? I thought we were just canceling and throwing away the current block.
There is a function stepper.set_separate_multi_axis() that should be used to avoid this kind of blocking
And it is being used for that Look at DUAL_ENDSTOP_APPLY_STEP()
But, perhaps, the problem is the condition itself... or the function stepper.set_separate_multi_axis() not being called at the proper moment
I thought we were just dealing with a general G28
single-endstop issue right now.
Ok, well see if the modified approach I've submitted to #12373 works. And maybe it also removes the need to set live_state
, validated_live_state
, and endstop_poll_count
in not_homing
.
It will not work...live_state is set because after home endstops are pressed (and this is correct). So if you don't force it to 0....one direction only
If endstops are turned off, no movement blockage, correct?
If the values are cleared before endstops are turned on, then it's just the same, no?
And if endstops are always-on, then…?
And… what about this? Only call force_update
if they're not already turned on:
void Endstops::enable(const bool onoff) {
if (onoff && !enabled) force_update();
enabled = onoff;
}
…plus notice it calls force_update
before it enables them. That would be to prevent planner.endstop_triggered
from being called immediately, since that should really only get called after the next move has started.
Maybe I'm over-thinking it, but I sense that this could occur.
Now I have to go and can't be of help but when you turn off endstops when they are engaged live_state is != 0 so you if you prevend them to update state (and you have to block live_state update otherwise you are with endstops alwyas on) you will only move one direction because, i may be worng but macro WRITE uses live_state, it prevent moving
when you turn off endstops when they are engaged live_state is != 0 so you if you [prevent] them to update state
ENDSTOP_NOISE_THRESHOLD
is not enabled, the endstops are not tested while off.ENDSTOP_NOISE_THRESHOLD
is enabled, live_state
is always updated, even when off.i may be [wrong] but macro WRITE uses live_state, it prevent moving
I'm not sure where this is supposed to be. Endstop triggering kills the current block, but it doesn't disable movement in any low-level way (that I can think of at the moment).
@thinkyhead I'll try to explain again...why don't you understand Italian or I write a better English?, it's so hard to transfer such details... ;-)
When you terminate homing you have your endstops pressed (in dual axis probably just one), this gives you live_state set (and this is right).
Once home is terminated you disable endstops check so live_state keeps its old value, note that when threshold is active you keep updated live_state but copy such values to other after filter time, but this doesn't change anything, motors are still stopped with endstop engaged so live_status or its brother is set. Once endstops check is disabled with 'not_homing' live state are never updated anymore but remember, they are set!!!!
What do you thing will happens to this:
DUAL_ENDSTOP_APPLY_STEP
when you move away from endstops (live_state will stay set) and then you try to move against endstops?
@ejtagle problem is only for dual endstops systems because above macro is not used with single
@thinkyhead I don't like your proposed solution because forcing poll to 1 before update will read current input status losing by the fact the filter action
maybe a clean solution may be to revert my previous fix and let state()
not return live_state when endstops don't need to be checked.
@ejtagle and @thinkyhead
But, perhaps, the problem is the condition itself... or the function stepper.set_separate_multi_axis() not being called at the proper moment
I may try to use this function to prevent live_state lock dual stepper after homing. Give me couple of hour and I'll post a new PR with a 2nd solution proposal
wow you guys are busy, so. What should i be testing if anything for you?
@GhostlyCrowd nothing for the moment... give me a couple of hours and I'll post a PR that should follow @ejtagle original endstop work idea, works with dual endstops and solve your issue
@GMagician : You should admit this thing is strange: The DUAL_ENDSTOP_APPLY_STEP
macro implementation, clearly forbids going against the limits _of the associated switch_ ... So, it should not forbidding the movement of the other "twin" axis, if the associated endstop limit switch is not pressed
If this macro is preventing the axis from moving, then separate_multi_axis
is true
,
As your workaround seems to "fix" the problem, then it is not a locked_motor
, so, as you say, it must be endstops::state() returning true
for the associated axis.
endstops::state()
returns validated_live_state
, but on endstops::update()
/**
* Check and update endstops
*/
#if HAS_X_MIN
#if ENABLED(X_DUAL_ENDSTOPS)
UPDATE_ENDSTOP_BIT(X, MIN);
#if HAS_X2_MIN
UPDATE_ENDSTOP_BIT(X2, MIN);
#else
COPY_LIVE_STATE(X_MIN, X2_MIN);
#endif
#else
UPDATE_ENDSTOP_BIT(X, MIN);
#endif
#endif
It is clear that there are 2 independant bits, one for each endstop bit... So movements should not be aborted...
But, there is a possibility this whole thing can fail: It is a bug of the homing procedure.
For a moment, let`s assume BOTH endstop switches are pressed (X1_MIN and X2_MIN)
The homeaxis()
function does the following:
1) We move the X1 axis against the X1 endstop switch
2) The switch is already pressed, so the first movement is cancelled right away.
3) The second axis is moved against the endstop limit, and as it was pressed, the movement is cancelled right away.
There is no bump for dual axis ...
I can´t understand why, but this homing procedure for dual axis is flawed. Doesn´t feel right. The procedure should be:
1) If the endstop is already pressed, perform a very small movement to release the endstop switch. 2mm seems reasonable - Wait until the endstop is ACTUALLY released
2) Perform the movement in the endstop direction - Wait until the endstop is hit and the movement cancelled
3) (Optative bump) Perform again a small move to release the endstop - Wait until it actually IS released
4) (Optative bump) Perform again a slower move in the endstop direction to press the endstop switch (at an slower speed) until the endstop is pressed.
This should be done for ALL axis.
But the step (1)
that is extremely important, is missing!! - Can @thinkyhead or @GMagician explain why this very important step is missing from the homing procedure ? ... I think that missing step is the cause of all the issues and workarounds proposed here!!
(And there is a reason for the need of that missing step (1)... When an axis is pressing the endstop, it can be, due to malfunction of the previous move,the axis is forcing the endstop. Having the endstop switch pressed is not enough to be sure that the axis is in the proper home position. So, that extra conditional step at the beginning is a MUST - We want a procedure sensible to transitions of the endstop switches - That is the trick for maximum precision
In fact, there is even an optimization here: If step (1) is applied, and bump is enabled, there is no need to execute steps (2) and (3)
@ejtagle the problem is not during home, that phase is working correclty, is after.
I think that as you state separate_multi_axis
is left true after homing and here raise the problem. I want to reset it after homing, restore code to previous #11900 and see what happens
@ejtagle and @thinkyhead give me a chance, I'll post soon a PR which fix dual enstops reverting #11900 and such modify should also fix this issue..then let's discuss about such solution
I've merged #12382, as it is very elegant and fixes our 💩 separate_multi_axis
wtf oversight.
I have pulled and compiled this. It solves my problem perfectly. I'm going to go ahead and close this.
Thanks for the report, @GhostlyCrowd. It was a good opportunity for us to review the endstops/probe code, get far too re-familiarized with it, and solve what was sure to be a vexing issue for many.
Thanks to everyone who participated i read every single comment. I learned some things along the way.
This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Most helpful comment
@GhostlyCrowd nothing for the moment... give me a couple of hours and I'll post a PR that should follow @ejtagle original endstop work idea, works with dual endstops and solve your issue