search

Klipper3d / klipper

Klipper is a 3d-printer firmware
GNU General Public License v3.0
8.95k stars 5.16k forks source link

Update Kconfig #6534

Closed TheFeralEngineer closed 2 months ago

TheFeralEngineer commented 3 months ago

add 48KiB bootloader option to STM32F401 for Artillery Sidewinder X3 boards

JamesH1978 commented 2 months ago

Thank you for submitting a PR. You will need to fix your code remove the trading spaces as per the checks error, And you will need to also need to do the regression tests and sign off your PR. Point 4 in the guidelines in https://github.com/Klipper3d/klipper/blob/master/docs/Example_Configs.md and point 3 in What to expect in https://github.com/Klipper3d/klipper/blob/master/docs/CONTRIBUTING.md

Thanks James

TheFeralEngineer commented 2 months ago

Thank you for submitting a PR. You will need to fix your code remove the trading spaces as per the checks error, And you will need to also need to do the regression tests and sign off your PR. Point 4 in the guidelines in https://github.com/Klipper3d/klipper/blob/master/docs/Example_Configs.md and point 3 in What to expect in https://github.com/Klipper3d/klipper/blob/master/docs/CONTRIBUTING.md

Thanks James

I'm completely new to this PR stuff, so bear with me...

JamesH1978 commented 2 months ago

Thank you for the changes you have made so far but you still need to provide a test/klippy/printers.test file with the entry added for your new file, so it can pass regression tests as per the doc i linked.

Thanks James

TheFeralEngineer commented 2 months ago

/src/stm32/Kconfig: Added MACH_STM32F401 to 48KiB bootloader for Artillery Sidewinder x3 printers /config: Added printer.cfg for Artillery Sidewinder x3 Plus printer

all I'm trying to do is upload a printer.cfg and a small change to Kconfig. I haven't changed anything else in the source code and all of the tests I've ran have passed. I'm kinda at a loss here and don't know what else to do.

Signed-off-by: Phil Timpsontheferalengineer@gmail.com

Sineos commented 2 months ago

Thanks for your changes. What is missing is adding your new config to the regression testing. See https://github.com/Klipper3d/klipper/pull/6119/files for an example.

I apparently missed one thing during my review:

[force_move]
enable_force_move: True

Please do not activate such functions in the default config unless there is a compelling reason to do so.

TheFeralEngineer commented 2 months ago

Thanks for your changes. What is missing is adding your new config to the regression testing. See https://github.com/Klipper3d/klipper/pull/6119/files for an example.

I apparently missed one thing during my review:

[force_move]
enable_force_move: True

Please do not activate such functions in the default config unless there is a compelling reason to do so.

30th time's a charm 🤞🤞

TheFeralEngineer commented 2 months ago

now it keeps failing. I'm no good at this 😭

TheFeralEngineer commented 2 months ago

oh, that's easy. I copied them directly from the bottom of my config (i guess the save_config puts them that way). I was starting to get a little distraught at all of the build failures before, but I think I've got it figured out now 🙂. I'll take care of it right away.

From: JamesH1978 @.> Sent: Wednesday, March 27, 2024 8:30 PM To: Klipper3d/klipper @.> Cc: TheFeralEngineer @.>; Author @.> Subject: Re: [Klipper3d/klipper] Update Kconfig (PR #6534)

@JamesH1978 requested changes on this pull request.

Sorry to give you one more set of adjustments :) but apart from my comments it all looks good to me as well.

and good work!

Thanks James

In config/printer-artillery-sidewinder-x3-plus-2024.cfghttps://github.com/Klipper3d/klipper/pull/6534#discussion_r1542178413:

+dir_pin: !PC10 +enable_pin: !PC12 +microsteps: 64 +nozzle_diameter: 0.400 +filament_diameter: 1.750 +heater_pin: PA6 +sensor_type: EPCOS 100K B57560G104F #Generic 3950 +sensor_pin: PC5 +min_extrude_temp: 170 +min_temp: 0 +max_temp: 300 +# Calibrate E-Steps https://www.klipper3d.org/Rotation_Distance.html#calibrating-rotation_distance-on-extruders +rotation_distance: 17.75 +# Calibrate PID: https://www.klipper3d.org/Config_checks.html#calibrate-pid-settings +# - Example: PID_CALIBRATE HEATER=extruder TARGET=200 +control = pid

Please can you normalise these to : instead of equals

Thanks James

In config/printer-artillery-sidewinder-x3-plus-2024.cfghttps://github.com/Klipper3d/klipper/pull/6534#discussion_r1542178606:

+# - Example: PID_CALIBRATE HEATER=extruder TARGET=200 +control = pid +pid_kp = 30.356 +pid_ki = 1.857 +pid_kd = 124.081 +# Calibrate PA: https://www.klipper3d.org/Pressure_Advance.html + +[heater_bed] +heater_pin: PA7 +sensor_type: EPCOS 100K B57560G104F +sensor_pin: PC0 +max_temp: 100 +min_temp: 0 +# Calibrate PID: https://www.klipper3d.org/Config_checks.html#calibrate-pid-settings +# - Example: PID_CALIBRATE HEATER=heater_bed TARGET=60 +control = pid

Same as above

In config/printer-artillery-sidewinder-x3-plus-2024.cfghttps://github.com/Klipper3d/klipper/pull/6534#discussion_r1542178695:

+off_below: 0.19 +max_speed: 1.0 +min_speed: 0.0 +control: watermark + +[filament_switch_sensor filament_sensor] +pause_on_runout: true +switch_pin: PC1 + +[probe] +pin: PC14 +x_offset:45.2 +y_offset:11.6 +speed:5 +lift_speed:15 +z_offset = 2.350

Same as above

— Reply to this email directly, view it on GitHubhttps://github.com/Klipper3d/klipper/pull/6534#pullrequestreview-1964964249, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ARUZ2LMXZWVEC5NKLGQSUNDY2NJDBAVCNFSM6AAAAABE4MHAX6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNRUHE3DIMRUHE. You are receiving this because you authored the thread.Message ID: @.***>

TheFeralEngineer commented 2 months ago

Yeah, sorry. Wasn't sure where to put it. I originally had it at the bottom of the dictionary, but the example that was sent to me showed the most recent addition at the top, that's why I put it there 🥴

Phil T. The Feral Engineer

KevinOConnor commented 2 months ago

Thanks. So is this PR now ready to merge?

-Kevin

TheFeralEngineer commented 2 months ago

it should be, yes. It passed all of the tests, I signed off on it, Sineos approved it but mentioned I should reorder the printers.test file, so I did. Other than that, everything should be all systems go.

From: KevinOConnor @.> Sent: Tuesday, April 2, 2024 9:51 PM To: Klipper3d/klipper @.> Cc: TheFeralEngineer @.>; Author @.> Subject: Re: [Klipper3d/klipper] Update Kconfig (PR #6534)

Thanks. So is this PR now ready to merge?

-Kevin

— Reply to this email directly, view it on GitHubhttps://github.com/Klipper3d/klipper/pull/6534#issuecomment-2033394700, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ARUZ2LMXOATW6AZNQRO6RB3Y3NODZAVCNFSM6AAAAABE4MHAX6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZTGM4TINZQGA. You are receiving this because you authored the thread.Message ID: @.***>

KevinOConnor commented 2 months ago

Thanks.

-Kevin