-
-
Notifications
You must be signed in to change notification settings - Fork 19.1k
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
[BUG] CURRENT_STEP_DOWN
wrapping around to 65536 on multiple driver errors
#27102
Comments
You forgot to do this. Please attach your configs. |
Issue is in const uint16_t I_rms = st.getMilliamps() - (CURRENT_STEP_DOWN);
if (I_rms > 50) { eg |
why not use something like: |
say you had set the current set to 850 (a common value) This doesn't solve the issue either of these is better
eg or this
eg |
Would this fix work? I don't know how to test it, cause I would have to trigger fake TMC errors and I don't know how. And I would like to have some devs eyes tell me if that wouldn't work :) void step_current_down(TMC &st) {
if (st.isEnabled()) {
if (st.getMilliamps() < (st.getMilliamps() - (CURRENT_STEP_DOWN))) { // Adding this check to make sure this is possible
const int16_t I_rms = st.getMilliamps() - (CURRENT_STEP_DOWN);
if (I_rms > 50) { // This can then be removed?
st.rms_current(I_rms);
#if ENABLED(REPORT_CURRENT_CHANGE)
st.printLabel();
SERIAL_ECHOLNPGM(" current decreased to ", I_rms);
#endif
}
}
}
} I am not sure what is also supposed to happen once the value |
well then this should fix the issue. but is it the correct course of action? here the current will always be set to a minimum of 50. template<typename TMC>
void step_current_down(TMC &st) {
if (st.isEnabled()) {
const int16_t I_rms = ((st.getMilliamps() - CURRENT_STEP_DOWN) >= 50) ? (st.getMilliamps() - CURRENT_STEP_DOWN) : 50;
st.rms_current(I_rms);
#if ENABLED(REPORT_CURRENT_CHANGE)
st.printLabel();
SERIAL_ECHOLNPGM(" current decreased to ", I_rms);
#endif
}
} |
Did you test the latest
bugfix-2.1.x
code?Yes, and the problem still exists.
Bug Description
Upon encountering an error, the feature
CURRENT_STEP_DOWN
reduces current a certain amount in the hope to fix the error conditionIf the motor current
_CURRENT
is not a multiple ofCURRENT_STEP_DOWN
, the condition that triggers a stop if current goes below 50 mA will never trigger due to the int16 current value wrapping around to 65,536 and less.For instance,
X_CURRENT
== 1900 andCURRENT_STEP_DOWN
== 200Then decreasing on multiple errors would go 1700, 1500, 1300, [...], 300, 100, 65,000 (wrap around), and the condition will never be true (on top of failing the print, putting the driver in a dangerous power state and overloading the stepper motor)
Bug Timeline
New bug
Expected behavior
When the driver current reaches critical levels, stop the printer without wrapping the int16
Actual behavior
The stepper driver current wraps around to over 65,000 mA
Steps to Reproduce
For this you require a faulty condition that is not easy to reproduce.
Version of Marlin Firmware
Bugfix Feb. 2024
Printer model
--
Electronics
--
LCD/Controller
--
Other add-ons
--
Bed Leveling
None
Your Slicer
None
Host Software
None
Don't forget to include
Configuration.h
andConfiguration_adv.h
.Additional information & file uploads
Running Marlin Bugfix from February 2024
The text was updated successfully, but these errors were encountered: