[possibly dangerous] DIAG1 pin in TMC2130 should not be set active high by default #10294
Comments
|
We can drop the config line. As far as sensorless homing is concerned, the only difference is needing to flip the end stop logic. |
|
How about adding a constant to the pins_XXX.h file for the board, such as X_DIAG1_PIN_ACTIVE_HIGH? That way once the user selected their board, the st.diag1_active_high(1) flag would be set correctly for their board. The other advantage of doing this is that sanitycheck.h could be updated to help the user configure X_[MIN/MAX]ENDSTOP_INVERTING correctly. i.e. It could warn the user to enable X[MIN/MAX]_ENDSTOP_INVERTING whenever DISABLED(X_DIAG1_ACTIVE_HIGH) and ENABLED(SENSORLESS_HOMING). |
|
If |
|
@teemuatlut : It's more than polarity. On page 25 of the datasheet, "5.1 General Configuration Registers", it defines diag1_pushpull as such:
The key here is that when st.diag1_active_high(1) is set, the driver both actively pushes and pulls, so this would lead to a short. But when st.diag1_active_high(0), it is a open-collector and only pulls (and thus you need an external pull-up resistor). |
Actually, the choice isn't arbitrary. In order for diag1_active_high(0) to work, the board needs a pull-up resistor (or the internal pull-up resistor must be enabled in the MCU input). In order for diag1_active_high(1) to be safe, then the board must not interconnect those two pins. So really the only option that would be safe for all boards would be to "diag1_active_high(0)", but then it would also be necessary to know whether the board had an pull-up resistor, or whether one would need to be set on the MCU input. |
|
By default ENDSTOPPULLUPS is enabled in "Configuration.h". If we remove With these new defaults, there is a possibility that on some boards the pull-up resistor would be doubled-up (one on the MCU via the default ENDSTOPPULLUPS and another on the board itself), but this unlikely to damage the driver and certainly is safer than the current default, which could cause a short on both pins. |
|
@teemuatlut : Made a PR with a suggested correction. |
- Added sanity check to require endstop inverting with SENSORLESS_HOMING
- Added sanity check to inform users to set the endstop to inverting when using SENSORLESS_HOMING
|
Okay. That explains why I don’t remember having to set endstoppullups. But that sanity check does exist in my currently installed working bugfix from April. But it doesn’t explain why with the most recent I no longer have motion on x and y. But I also have to point out that the firmware tried to require me to flip the z endstop as well. But my current configuration does not support this type of endstop. Not that I couldn’t flip the polarity of the switch but I got around this by commenting out the z sensitivity line to get around sanity check. But it still requires me to set from terminal on the running firmware the sensitivity of z to greater than 0. So it’s kinda broken there. I know that I broke it by commenting out a sub setting. But it did allow my z axis to move finally. And that setting is problematic on both my April build and this one. So the issue is that sensorless homing requires endstop pull-ups. And for those pull-ups to be set to true. Even though I need to set z to false. So there needs to be a way to determine which drivers you have sh set up on. |
You must comment out |
|
Yes that’s what I did. But it then required me to still enter a positive sensitivity number. I just randomly entered 14 and then I was able to use the z axis. For some reason it wouldn’t move with a sensitivity value of 0 and yes the z homing sensitivity value is still persistent even after commenting it out. Also, like I said on latest bugfix I cannot move the x or y axis at all.
…Sent from my iPhone
On Jul 3, 2018, at 7:15 PM, Scott Lahteine <notifications@github.com<mailto:notifications@github.com>> wrote:
I also have to point out that the firmware tried to require me to flip the z endstop as well.
Comment out Z_HOMING_SENSITIVITY to specify that Z is using a normal endstop.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<https://nam05.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMarlinFirmware%2FMarlin%2Fissues%2F10294%23issuecomment-402326468&data=02%7C01%7C%7C1e7782284c04415b7dc908d5e1433ec3%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636662601280244341&sdata=SK3Eb0A25yInohQ4zAKDBDdeE76wkgxwjZu8fZ8T0C8%3D&reserved=0>, or mute the thread<https://nam05.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAB7M2j354nqNWeY7PSm8MmOV_4JD1VsFks5uDAkegaJpZM4TFqHq&data=02%7C01%7C%7C1e7782284c04415b7dc908d5e1433ec3%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636662601280244341&sdata=EDshd4c5cqGq5weTCMgmdznJdAGMS6tWTqhW%2BjVLm18%3D&reserved=0>.
|
I'm sorry we can't do much about that without more data. Your issue is so far unique. |
|
Observe the output of |
- Added sanity check to require endstop inverting with SENSORLESS_HOMING
|
I would like to have the diag1 as aktiv high (pushpull). Where and how i had to add 'st.diag1_active_high'? |
|
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. |
The following line in Marlin is potentially dangerous:
st.diag1_active_high(1); // For sensorless homingThe reason is that some boards (for example, the EinsyRambo from Ultimachine), ties the two pins together (and then to VCC with a pull-up resistor). This is one of the possibilities mentioned in the data sheet, so it should be expected that more boards will choose this approach.
The default value of zero is safe, but setting diag1_active_high to 1 could cause the two diag pins to short out on such boards and I do not know whether the TMC drivers can survive it. This potentially unsafe condition should not be a default in Marlin.
I would think this should either be a configuration option, or the selection ought to be made via a variable in the in the pins_XXX.h file for the boards, as the choice is board specific.
The text was updated successfully, but these errors were encountered: