Skip to content
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

🩹 Fix MKS Gen-L V1 pins, allow more RAMPS overrides #26974

Merged

Conversation

thisiskeithb
Copy link
Member

@thisiskeithb thisiskeithb commented Apr 16, 2024

Description

MKS Gen-L V1.0 moseft pins keep getting changed, with the latest being in #25717 since they assumed MKS' schematic was correct & the pins diagram wrong. I physically traced the pins on my MKS Gen-L V1.0 board and the pins diagram is correct, not the schematic.

They are:

     HEATER_BED = 8
       HEATER_0 = 10
HEATER_1 / FAN1 = 7 (FAN1 when used in a single hotend config / frequently used for E0_AUTO_FAN_PIN)
           FAN0 = 9

I fully defined the pins as they are on the board itself and added silkscreen labels & Marlin's custom MOSFET_*_PIN naming scheme as comments and disabled them for documentation purposes. Some #ifndefs were required in pins_RAMPS.h to prevent a wall of warnings due to pins being redefined.

Also, this board only ships with an ATmega2560, so I updated the board environment to reflect that.

Requirements

MKS Gen-L V1.0

Benefits

This will hopefully be the last MKS Gen-L V1.0 change since I physically traced pins in question to the MCU pins.

Configurations

Configs from #26971 or Artillery Genius BLTouch configs from our Configurations repo.

Related Issues

@thisiskeithb
Copy link
Member Author

thisiskeithb commented Apr 16, 2024

After chatting with @ellensp, I reverted to using the default MOSFET_*_PINs as defined in pins_RAMPS.h, but left the correct pins (commented out) in pins_MKS_GEN_L.h for documentation purposes so there's no confusion as to what they should be in the future.

@sjasonsmith sjasonsmith requested a review from ellensp April 17, 2024 05:48
@thisiskeithb thisiskeithb force-pushed the pr/fix_mks_gen_l_mosfet_pins branch from 5a358bb to 8eaac5f Compare April 18, 2024 21:00
@thisiskeithb
Copy link
Member Author

Merge conflicts caused by 9342dae have been resolved.

@sjasonsmith
Copy link
Contributor

@ellensp would you mind reviewing and commenting on this? I'm a bit confused by the back and forth regarding which incorrect pins to use versus comment out. It sounds like you know what's going on and will be better able to assess whether this is ready to go in than I am.

@thisiskeithb
Copy link
Member Author

thisiskeithb commented Apr 21, 2024

The commented out pins are there for documentation purposes. I physically traced out the connectors/mosfets to the MCU pins on my MKS Gen-L V1 and verified they are correct.

This PR is ready to merge.

@sjasonsmith
Copy link
Contributor

I just still don't understand what they mean. They are just commented out pins "for documentation purposes", but do they represent pins that are wrong? Are they from the schematic, from the board? I'm left guessing what a commented out pin names.

Perhaps more explanation is warranted in the comments to describe the errors in the schematic, rather than just the single line saying "there are errors on the schematic". I'm not sure how to interpret that information as I look at the rest of the file.

@thisiskeithb
Copy link
Member Author

I just still don't understand what they mean. They are just commented out pins "for documentation purposes", but do they represent pins that are wrong? Are they from the schematic, from the board? I'm left guessing what a commented out pin names.

I left some review comments, so hopefully that clears things up.

Perhaps more explanation is warranted in the comments to describe the errors in the schematic, rather than just the single line saying "there are errors on the schematic". I'm not sure how to interpret that information as I look at the rest of the file.

I'm not able to review MKS' schematic for all the pin & connector errors, but their pins diagram PDF is correct.

<sarcasm>Maybe there needs to be a STOP CHANGING THESE PINS warning at the top instead?</sarcasm> 😄

Copy link
Contributor

@sjasonsmith sjasonsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of the more explicit documentation of these that I suggest in this review?

Marlin/src/pins/ramps/pins_MKS_GEN_L.h Show resolved Hide resolved
Marlin/src/pins/ramps/pins_MKS_GEN_L.h Outdated Show resolved Hide resolved
Marlin/src/pins/ramps/pins_MKS_GEN_L.h Outdated Show resolved Hide resolved
Marlin/src/pins/ramps/pins_MKS_GEN_L.h Outdated Show resolved Hide resolved
@thisiskeithb
Copy link
Member Author

What do you think of the more explicit documentation of these that I suggest in this review?

All suggested changes accepted/merged. Should be good to go once CI is done.

@thisiskeithb thisiskeithb removed the request for review from ellensp May 20, 2024 03:03
@thisiskeithb thisiskeithb force-pushed the pr/fix_mks_gen_l_mosfet_pins branch from bf75bab to 863c8ad Compare July 18, 2024 13:14
@thisiskeithb thisiskeithb force-pushed the pr/fix_mks_gen_l_mosfet_pins branch from 863c8ad to 5ec84e0 Compare August 6, 2024 22:37
#define MOSFET_A_PIN 10 // HE0
#define MOSFET_B_PIN 7 // HE1 or FAN Hotend Cooling
#define MOSFET_C_PIN 8 // HBED
#define FAN0_PIN 9 // FAN Part Cooling
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either the MOSFET pin scheme needs to be retained, or this file needs to do the handling of the flags *_EFB, *_EEB, etc., for cases where the Extruder, Fan, Bed arrangement may change. These flags may already be set for the HAS_MULTI_HOTEND and/or HEATERS_PARALLEL and should be preferred over referring to these settings flags directly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder that the rule for Extruder, Fan, Bed pins is not to always define them according to their silkscreen label, but according to the configuration requirements. The board may say something like "H1, Fan, Bed" which would correspond to the common default FET_ORDER_EFB. But when two extruders are defined, the FET_ORDER_EEB flag gets set, so the pins should then be defined as "H1, H2, Bed" with no fan.

If the FET_ORDER_* flags aren't coming out right for this board according to those flags, either these FETs are just not defined in the correct sequence, or the general FET assignment in pins_RAMPS.h is wonky.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I wonder if it work just using the order 10, 9, 8, 7… But we have to look at the machines these ship in also and see if they are using non-standard ordering on shipping units.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either the MOSFET pin scheme needs to be retained, or this file needs to do the handling of the flags *_EFB, *_EEB, etc.,

I really don't follow all that FET_ORDER stuff since we don't do that on non-AVR boards, but I have physically traced the pins on the MCU and verified that they now match real hardware.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I wonder if it work just using the order 10, 9, 8, 7… But we have to look at the machines these ship in also and see if they are using non-standard ordering on shipping units.

We host several Artillery configs and the older/non-32-bit printers use a standard Gen-L: https://github.com/MarlinFirmware/Configurations/tree/import-2.1.x/config/examples/Artillery

@thinkyhead thinkyhead merged commit 4a9d380 into MarlinFirmware:bugfix-2.1.x Aug 11, 2024
64 checks passed
@thisiskeithb thisiskeithb deleted the pr/fix_mks_gen_l_mosfet_pins branch August 11, 2024 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] MKS_GEN_L extruder fan not working
3 participants