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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 7 commits into
base: bugfix-2.1.x
Choose a base branch
from
Open

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

wants to merge 7 commits into from
+30 −11

Conversation

thisiskeithb
Copy link
Member

@thisiskeithb thisiskeithb commented Apr 16, 2024 •
edited

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 thisiskeithb linked an issue Apr 16, 2024 that may be closed by this pull request
[BUG] MKS_GEN_L extruder fan not working #26971
Open
1 task
@thisiskeithb thisiskeithb mentioned this pull request Apr 16, 2024
Open
1 task
@thisiskeithb
Copy link
Member Author

thisiskeithb commented Apr 16, 2024 •
edited

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.

@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 •
edited

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> 😄

sjasonsmith
sjasonsmith reviewed Apr 29, 2024
View reviewed changes
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 Show resolved Hide resolved
Marlin/src/pins/ramps/pins_MKS_GEN_L.h 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjasonsmith sjasonsmith sjasonsmith left review comments

@ellensp ellensp Awaiting requested review from ellensp

Assignees
No one assigned
Labels
A: ATmega2560 C: Boards/Pins PR: Bug Fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] MKS_GEN_L extruder fan not working
2 participants
@thisiskeithb @sjasonsmith