Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add 'tool' module #3995
Add 'tool' module #3995
Changes from 3 commits
cdc71da
cda450e
3ba62ab
40acc7c
eadd811
8ba87dd
c57abb6
6838763
87a57e4
7b9dd0e
6b8cfb1
ec89378
84b2629
d5892db
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Unfortunately, it's not valid to call
toolhead.register_lookahead_callback()
without holding the gcode mutex. That is, it can't be called directly from a reactor callback. You probably want to do something likeprint_time = self.mcu_pwm.get_mcu().estimated_print_time(eventtime)
instead - though the time will need to be padded by 100ms (to allow for transmission time to the mcu).-Kevin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it allowed to just set the pin directly, as it is now?
100ms time padding is now explicitly in the "relax_margin" which is supposed to give the scheduler some time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
toolhead.get_last_move_time()
and relatedtoolhead.register_lookahead_callback()
always return a time in the future, so that it is possible to schedule events at that time. A call tomcu.estimated_print_time(systime)
returns the time of "now" on the micro-controller - if one were to schedule an event at that time, it would always result in a "timer too close" as it takes time to transmit that schedule to the micro-controller.I'm not sure what the goal of the "relax_margin" is. You only need the PIN_MIN_TIME - any additional time is probably not a good thing as it limits how response future updates are.
-Kevin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the "relax margin" was supposed to reduce stress between "now" and the scheduled time the pin has to update (reducing the possibility of too close timers).
But if you say that this just removes responsiveness, it can be replaced for just the PIN_MIN_TIME, instead of 10% timeout + PIN_MIN_TIME.