Klipper3d / klipper

Klipper is a 3d-printer firmware
GNU General Public License v3.0
8.95k stars 5.16k forks source link

[manual_stepper] Add basic status. #6527

Closed viesturz closed 2 months ago

viesturz commented 3 months ago

Adding position and enabled in manual_stepper status. Enabled is already available through stepper_enable object. But this makes it more straightforward to access it.

github-actions[bot] commented 2 months ago

Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html

There are some steps that you can take now:

  1. Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
  2. Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
  3. Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards, ~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

KevinOConnor commented 2 months ago

Thanks. It seems fine to me. I didn't understand the comment Note that the stepper may have not reached this position yet if SYNC=1 is used. The returned position is always the last requested position, which may differ from the actual position - and as far as I know has no relation to the SYNC command-line option. Am I missing something?

Cheers, -Kevin

viesturz commented 2 months ago

Oh I meant the opposite, that stepper may not have reached the position yet if sync=0.

Or best to remove that remark altogether?

KevinOConnor commented 2 months ago

As I understand it, the value returned from rail.get_commanded_position() is the last requested position, and has no relationship with the SYNC parameter. Maybe I'm missing something.

-Kevin

viesturz commented 2 months ago

If SYNC=1 then the command waits for the motor to reach the requested position before returing, effectively ensuring any readers that the actual position will be the same as commanded position.

Updated the documentation.

KevinOConnor commented 2 months ago

If SYNC=1 then the command waits for the motor to reach the requested position before returing, effectively ensuring any readers that the actual position will be the same as commanded position.

That is not correct. Klipper always schedules moves at a time in the future. The next g-code command is likely to be processed well before the last requested move is even scheduled to start.

What SYNC=1 does is ensure that the next scheduled move on the main toolhead is scheduled at a time after the manual_stepper move is scheduled to complete. It ensures an ordering between movements on the toolhead and the manual_stepper - it has no relation to the time that g-code commands are processed.

Cheers, -Kevin

viesturz commented 2 months ago

I see, thanks, will update the PR tomorrow.

I re-read the code and just to confirm this is my understanding now: -moves of same manual stepper are sequenced, regardless of sync

On Wed, Apr 3, 2024, 22:45 KevinOConnor @.***> wrote:

If SYNC=1 then the command waits for the motor to reach the requested position before returing, effectively ensuring any readers that the actual position will be the same as commanded position.

That is not correct. Klipper always schedules moves at a time in the future. The next g-code command is likely to be processed well before the last requested move is even scheduled to start.

What SYNC=1 does is ensure that the next scheduled move on the main toolhead is scheduled at a time after the manual_stepper move is scheduled to complete. It ensures an ordering between movements on the toolhead and the manual_stepper - it has no relation to the time that g-code commands are processed.

Cheers, -Kevin

— Reply to this email directly, view it on GitHub https://github.com/Klipper3d/klipper/pull/6527#issuecomment-2035552356, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMQT44BHHPYJQ7RG3VNHHLY3RS73AVCNFSM6AAAAABEO3LEBOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZVGU2TEMZVGY . You are receiving this because you authored the thread.Message ID: @.***>

KevinOConnor commented 2 months ago
  • moves of same manual stepper are sequenced, regardless of sync

Yes.

  • sync ensures next toolhead move is after manual move AND next manual move is after toolhead moves.

A manual_stepper move always starts after the completion of the last requested toolhead move, regardless of SYNC. A SYNC=0 allows a following toolhead move to start prior to the nominal completion of the manual_stepper move.

  • no explicit way to sequence a manual move starting after a toolhead move.

This is always done. No current way to not do it.

-Kevin

viesturz commented 2 months ago

Thanks for the clarification.

This line threw me off, is that redundant? https://github.com/Klipper3d/klipper/blob/75a40e817db4d5bc9d7d893fad499907d2b910a2/klippy/extras/manual_stepper.py#L105

On Thu, Apr 4, 2024, 00:09 KevinOConnor @.***> wrote:

  • moves of same manual stepper are sequenced, regardless of sync

Yes.

  • sync ensures next toolhead move is after manual move AND next manual move is after toolhead moves.

A manual_stepper move always starts after the completion of the last requested toolhead move, regardless of SYNC. A SYNC=0 allows a following toolhead move to start prior to the nominal completion of the manual_stepper move.

  • no explicit way to sequence a manual move starting after a toolhead move.

This is always done. No current way to not do it.

-Kevin

— Reply to this email directly, view it on GitHub https://github.com/Klipper3d/klipper/pull/6527#issuecomment-2035687211, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMQT45FBHHE5OFXX2FYUSDY3R4ZZAVCNFSM6AAAAABEO3LEBOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZVGY4DOMRRGE . You are receiving this because you authored the thread.Message ID: @.***>

KevinOConnor commented 2 months ago

That line only gets run if a command does not contain a MOVE. As I understand it, it could be used for explicit syncing of upcoming toolhead moves - possibly useful after some number of moves with SYNC=1. For example, something like MANUAL_STEPPER MOVE=...some 20 second move..., G1 X...some 10 second move..., MANUAL_STEPPER SYNC=1, G1 X... - which would allow the first G1 to overlap, but not the second G1.

-Kevin

viesturz commented 2 months ago

Thanks, fixed the documentation.

KevinOConnor commented 2 months ago

Thanks.

-Kevin

gearslam commented 2 months ago

This just causes "Unhandled exception during run" for me.

viesturz commented 2 months ago

Seriously? I'm cursed, 2/2 PRs broke stuff.

On Fri, Apr 5, 2024, 20:45 Bill Brock @.***> wrote:

This just causes "Unhandled exception during run" for me.

— Reply to this email directly, view it on GitHub https://github.com/Klipper3d/klipper/pull/6527#issuecomment-2040433085, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMQT473FOE6ONNIM5BMAGLY33WNBAVCNFSM6AAAAABEO3LEBOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBQGQZTGMBYGU . You are receiving this because you authored the thread.Message ID: @.***>

gearslam commented 2 months ago

What's strange is only half of the Fluidd interface loads.

KevinOConnor commented 2 months ago

I reverted this commit (in commit 4cfa266e). The MCU_Stepper class does not have a is_motor_enabled() method, so this change just results in an internal exception.

-Kevin