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

Add spi0e bus for rp2040, used on pitb v2 by fysetc #6683

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KoiosLabs
Copy link

@KoiosLabs KoiosLabs commented Sep 10, 2024

I purchased the fysetc PITB v2 board, which is not functioning via software spi, and I'm not 100% sure why.

The issue with hardware spi, is that this board is wired such that:
SCK: gpio2
MOSI: gpio3
MISO: gpio4

however, the spi0 configuration currently in the klipper codebase, only allows an spi permutation of:
SCK: 2
MOSI: 3
MISO: 0

However, that said, I've seen a few cases around the internet in troubleshooting this, that show people using the rp2040 spi bus configured this way, as well as the PITB v2 board. The rp2040 datasheet (page 13: here) shows it as a supported configuration, so I added spi0e as an option, and everything now started up and works.

I'd almost suggest perhaps a more configurable way of managing these spi buses for this chip, as many gpio can be used for spi0 for example, but I'm not familiar enough with the klipper code to realize it. This solved my immediate issue so I offer it up for the masses to accept or not :)

Once this is done, the config looks like this for that board:

[stepper_x]
step_pin: PITB:gpio6
dir_pin: PITB:gpio5
enable_pin: !PITB:gpio20
rotation_distance: 40
microsteps: 16
full_steps_per_rotation:200  #set to 400 for 0.9 degree stepper
endstop_pin: EBB:PB8
position_min: 0
position_endstop: 350
position_max: 350
homing_speed: 25   #Max 100
homing_retract_dist: 5
homing_positive_dir: true

[tmc5160 stepper_x]
spi_bus: spi0e
cs_pin: PITB:gpio1
interpolate: False
diag1_pin: PITB:gpio7
run_current: 0.800 

[stepper_y]
step_pin: PITB:gpio13
dir_pin: PITB:gpio23
enable_pin: !PITB:gpio22
rotation_distance: 40
microsteps: 16
full_steps_per_rotation:200  #set to 400 for 0.9 degree stepper
endstop_pin: PG6
position_min: 0
position_endstop: 350
position_max: 350
homing_speed: 25  #Max 100
homing_retract_dist: 5
homing_positive_dir: true

[tmc5160 stepper_y] 
spi_bus: spi0e
cs_pin: PITB:gpio21
diag1_pin: PITB:^gpio14
interpolate: False
run_current: 0.8

@mboyer85
Copy link

mboyer85 commented Sep 10, 2024

Hi, Good job and thanks a lot for the help, but DECL_ENUMERATION‎ index number for spi1a/b/c must be incremented, no?

@KoiosLabs
Copy link
Author

Good catch, totally didnt even see that, I'll add a fix shortly :)

@KoiosLabs
Copy link
Author

KoiosLabs commented Sep 11, 2024

I'm far from a klipper expert, mostly just stumbling around until i got it working, buut, with that said, I think you need to make sure you've got the PR checked out, and flashed to the PITB for this to work, for example, mine the version looks like this:
image

Take a look at this: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally guide for how to get the pr locally if your unfamiliar, and ignore all this if you did that already and I'm just missing something because it worked fine for me

Here is a sample of mine, I've successfully homed my x/y, though my printer is still otherwise in pieces, it shows the spi bus working on the PITB:

20:54:32 
$ STEPPER_BUZZ STEPPER=stepper_y
20:54:40 
$ STEPPER_BUZZ STEPPER=stepper_x
20:54:42 
$ DUMP_TMC STEPPER=stepper_x
20:54:47 
$ DUMP_TMC STEPPER=stepper_y
20:54:53 
// ========== Write-only registers ==========
20:54:53 
// GLOBALSCALER: 00000043 globalscaler=67
20:54:53 
// IHOLD_IRUN: 00061f1f ihold=31 irun=31 iholddelay=6
20:54:53 
// MSLUT0:     aaaab554 mslut0=2863314260
20:54:53 
// MSLUT1:     4a9554aa mslut1=1251300522
20:54:53 
// MSLUT2:     24492929 mslut2=608774441
20:54:53 
// MSLUT3:     10104222 mslut3=269500962
20:54:53 
// MSLUT4:     fbffffff mslut4=4227858431
20:54:53 
// MSLUT5:     b5bb777d mslut5=3048961917
20:54:53 
// MSLUT6:     49295556 mslut6=1227445590
20:54:53 
// MSLUT7:     00404222 mslut7=4211234
20:54:53 
// MSLUTSEL:   ffff8056 w0=2 w1=1 w2=1 w3=1 x1=128 x2=255 x3=255
20:54:53 
// MSLUTSTART: 00f70000 start_sin90=247
20:54:53 
// TPWMTHRS:   000fffff tpwmthrs=1048575
20:54:53 
// TCOOLTHRS:  00000000
20:54:53 
// THIGH:      00000000
20:54:53 
// COOLCONF:   00000000
20:54:53 
// DRV_CONF:   00000400 bbmclks=4
20:54:53 
// PWMCONF:    c40c001e pwm_ofs=30 pwm_autoscale=1 pwm_autograd=1 pwm_reg=4 pwm_lim=12
20:54:53 
// TPOWERDOWN: 0000000a tpowerdown=10
20:54:53 
// ========== Queried registers ==========
20:54:53 
// GCONF:      00000008 multistep_filt=1
20:54:53 
// CHOPCONF:   34410153 toff=3 hstrt=5 hend=2 tbl=2 tpfd=4 mres=4(16usteps) intpol=1 dedge=1
20:54:53 
// GSTAT:      00000000
20:54:53 
// DRV_STATUS: 011f0027 sg_result=39 cs_actual=31 stallguard=1
20:54:53 
// FACTORY_CONF: 0000000e factory_conf=14
20:54:53 
// IOIN:       30000041 refl_step=1 sd_mode=1 version=0x30
20:54:53 
// LOST_STEPS: 00000000
20:54:53 
// MSCNT:      0000033e mscnt=830
20:54:53 
// MSCURACT:   0067011f cur_a=-225 cur_b=103
20:54:53 
// OTP_READ:   0000000e otp_fclktrim=14
20:54:55 
// GLOBALSCALER: 00000043 globalscaler=67
20:54:55 
// IHOLD_IRUN: 00061f1f ihold=31 irun=31 iholddelay=6
20:54:55 
// MSLUT0:     aaaab554 mslut0=2863314260
20:54:55 
// MSLUT2:     24492929 mslut2=608774441
20:54:55 
// MSLUT3:     10104222 mslut3=269500962
20:54:55 
// MSLUT4:     fbffffff mslut4=4227858431
20:54:55 
// MSLUT6:     49295556 mslut6=1227445590
20:54:55 
// MSLUT7:     00404222 mslut7=4211234
20:54:55 
// MSLUTSEL:   ffff8056 w0=2 w1=1 w2=1 w3=1 x1=128 x2=255 x3=255
20:54:55 
// MSLUTSTART: 00f70000 start_sin90=247
20:54:55 
// TPWMTHRS:   000fffff tpwmthrs=1048575
20:54:55 
// TCOOLTHRS:  00000000
20:54:55 
// THIGH:      00000000
20:54:55 
// COOLCONF:   00000000
20:54:55 
// DRV_CONF:   00000400 bbmclks=4

*Edit: Also, your log shows this

Git version: '1cde5629'
Branch: pitbv2
Remote: origin
Tracked URL: https://github.com/ejhoness/klipperBigtreetech.git

further your log shows

mcu 'mcu': got {'oid': 18, 'next_clock': 915840000, 'value': 32754, '#name': 'analog_in_state', '#sent_time': 3616.384293564, '#receive_time': 3616.844897648}
Loaded MCU 'PITB' 121 commands (v0.12.0-287-gf71d2c7c / gcc: (15:8-2019-q3-1+b1) 8.3.1 20190703 (release) [gcc-8-branch revision 273027] binutils: (2.35.2-2+14+b2) 2.35.2)
MCU 'PITB' config: ADC_MAX=4095 BUS_PINS_i2c0a=gpio0,gpio1 BUS_PINS_i2c0b=gpio4,gpio5 BUS_PINS_i2c0c=gpio8,gpio9 BUS_PINS_i2c0d=gpio12,gpio13 BUS_PINS_i2c0e=gpio16,gpio17 BUS_PINS_i2c0f=gpio20,gpio21 BUS_PINS_i2c0g=gpio24,gpio25 BUS_PINS_i2c0h=gpio28,gpio29 BUS_PINS_i2c1a=gpio2,gpio3 BUS_PINS_i2c1b=gpio6,gpio7 BUS_PINS_i2c1c=gpio10,gpio11 BUS_PINS_i2c1d=gpio14,gpio15 BUS_PINS_i2c1e=gpio18,gpio19 BUS_PINS_i2c1f=gpio22,gpio23 BUS_PINS_i2c1g=gpio26,gpio27 BUS_PINS_spi0a=gpio0,gpio3,gpio2 BUS_PINS_spi0b=gpio4,gpio7,gpio6 BUS_PINS_spi0c=gpio16,gpio19,gpio18 BUS_PINS_spi0d=gpio20,gpio23,gpio22 BUS_PINS_spi1a=gpio8,gpio11,gpio10 BUS_PINS_spi1b=gpio12,gpio15,gpio14 BUS_PINS_spi1c=gpio24,gpio27,gpio26 CANBUS_FREQUENCY=1000000 CLOCK_FREQ=12000000 MCU=rp2040 PWM_MAX=255 RECEIVE_WINDOW=192 RESERVE_PINS_CAN=gpio9,gpio8 STATS_SUMSQ_BASE=256 STEPPER_BOTH_EDGE=1
mcu 'CB2': Starting connect
mcu 'mcu': got {'oid': 11, 'next_clock': 926880000, 'value': 13483, '#name': 'analog_in_state', '#sent_time': 3616.384293564, '#receive_time': 3617.074867189}

and mine has:

{'count': 1110, 'sum': 231841, 'sumsq': 686623, '#name': 'stats', '#sent_time': 812.225581145, '#receive_time': 812.2259410900001}
Loaded MCU 'PITB' 121 commands (v0.12.0-293-gde93322de-dirty-20240910_171339-Voron24 / gcc: (15:12.2.rel1-1) 12.2.1 20221205 binutils: (2.39-8+rpi1+18) 2.39)
MCU 'PITB' config: ADC_MAX=4095 BUS_PINS_i2c0a=gpio0,gpio1 BUS_PINS_i2c0b=gpio4,gpio5 BUS_PINS_i2c0c=gpio8,gpio9 BUS_PINS_i2c0d=gpio12,gpio13 BUS_PINS_i2c0e=gpio16,gpio17 BUS_PINS_i2c0f=gpio20,gpio21 BUS_PINS_i2c0g=gpio24,gpio25 BUS_PINS_i2c0h=gpio28,gpio29 BUS_PINS_i2c1a=gpio2,gpio3 BUS_PINS_i2c1b=gpio6,gpio7 BUS_PINS_i2c1c=gpio10,gpio11 BUS_PINS_i2c1d=gpio14,gpio15 BUS_PINS_i2c1e=gpio18,gpio19 BUS_PINS_i2c1f=gpio22,gpio23 BUS_PINS_i2c1g=gpio26,gpio27 BUS_PINS_spi0a=gpio0,gpio3,gpio2 BUS_PINS_spi0b=gpio4,gpio7,gpio6 BUS_PINS_spi0c=gpio16,gpio19,gpio18 BUS_PINS_spi0d=gpio20,gpio23,gpio22 BUS_PINS_spi0e=gpio4,gpio3,gpio2 BUS_PINS_spi1a=gpio8,gpio11,gpio10 BUS_PINS_spi1b=gpio12,gpio15,gpio14 BUS_PINS_spi1c=gpio24,gpio27,gpio26 CANBUS_FREQUENCY=1000000 CLOCK_FREQ=12000000 INITIAL_PINS=gpio15 MCU=rp2040 PWM_MAX=255 RECEIVE_WINDOW=192 RESERVE_PINS_CAN=gpio9,gpio8 STATS_SUMSQ_BASE=256 STEPPER_BOTH_EDGE=1
mcu_temperature 'PITB' nominal base=437.226612 slope=-1917.489831
mcu 'scanner': Starting CAN connect
Created a socket
mcu 'PITB': got {'oid': 6, 'next_clock': 1362858995, 'value': 31260, '#name': 'analog_in_state', '#sent_time': 812.278343849, '#receive_time': 812.372588849}

Notice, that it is missing spi0e, which corroborates that you have not flashed this PR further.

@ejhoness
Copy link

ejhoness commented Sep 11, 2024

Thanks for your attention.

I'm a bit flawed. I must have missed something, I updated everything including the linux process, to "new version" but...

I'll do it again based on your git. for a clean installation, Thank you very much.

Captura de tela de 2024-09-10 23-15-28

yes, my defect or my files, with clean files, no spi0e error :D S2
now I believe that my board is broken, no fans and no motors :(
thanks for your attention.

klippy (3).log
Captura de tela de 2024-09-11 00-15-33
Thank you very much. S2

@JamesH1978
Copy link
Collaborator

Thankyou for submitting a PR, please can you sign off your as mentioned in point 3 in https://github.com/Klipper3d/klipper/blob/master/docs/CONTRIBUTING.md#what-to-expect-in-a-review

Thanks
James

@KoiosLabs
Copy link
Author

Thankyou for submitting a PR, please can you sign off your as mentioned in point 3 in https://github.com/Klipper3d/klipper/blob/master/docs/CONTRIBUTING.md#what-to-expect-in-a-review

Thanks James

Hi James,

I think I got that message in there properly, and squashed all my non-sense, so the upstream repo stays clean :)

rp2040/spi: Add spi0e bus to support fysetc PITB V2

The Fysetc PITB V2 board uses a spi bus config that is supported by the
RP2040 chip, but not klipper, so this adds the relevant config to the file
to allow you to run the tmc5160's on the board via hardware SPI.  This
resolves the issue of software spi not working on this board, which I
was unable to fully understand.

I have also seen other users encounter similar bus config issues with
the rp2040 setting up things like accelerometers and such with this
pin layout.

Signed-off-by: Jessica Hunt <hunt.jessica@proton.me>
@KoiosLabs
Copy link
Author

KoiosLabs commented Sep 11, 2024

I think my code should meet all the stuff in the contributing doc.

  1. It works, I'm running it on my v2.4 without issue. It passed the regression testing.

  2. I've identified the real world benefit (fixing the fysetc PITB board, and enabling hardware spi on this combination of pins on the rp2040).

  3. I've signed the commit, and my contribution is fine to be released as public domain, or whatever license klipper is using, no worries. Its my original work, I just followed the existing code style.

  4. see # 3 above (not pr 3 lol, silly git), I matched the code style as best I could with the existing spi bus definitions.

  5. I originally went down this rabbit hole, as the various spi bus to pinouts dont seem to have documentation I could find? If it exists, please let me know and I'll update it accordingly, but I also dont really have time to document everything from scratch, sorry :(

  6. I've reduced my work to one squashed commit with the matching commit message format per the documentation. Its a relatively small change so this seemed appropriate.

@mboyer85
Copy link

Have you try sensorless ?

@KoiosLabs
Copy link
Author

I have not, but this is more about the communication bus than the tmc driver implementation, so I cant see why it wouldnt work. I have no use for sensorless homing on my printer.

@ejhoness
Copy link

ejhoness commented Sep 11, 2024

I don't want to abuse it, I know you've already done a lot, and I thank you, I would like to ask you for the pinout.cfg file, so I can make it as similar as possible, because I may be making a mistake in something, like before when there was no pitb update, shameful, but...

Otherwise, I thank everyone for everything. Long life and prosperity. S2

@KoiosLabs
Copy link
Author

KoiosLabs commented Sep 12, 2024

I don't want to abuse it, I know you've already done a lot, and I thank you, I would like to ask you for the pinout.cfg file, so I can make it as similar as possible, because I may be making a mistake in something, like before when there was no pitb update, shameful, but...

Otherwise, I thank everyone for everything. Long life and prosperity. S2

Hi @ejhoness

I did post my config for the motors above in the original PR, but if you read through your log, it will start connecting the various MCU's you have on your printer. In that look for your PITB, and it should list the various SPI and I2C busses it has, if it isnt listing the spi0e bus, then something went wrong with compiling and flashing this change to the mcu. You shouldnt need to reflash all your mcu's if your on a recent klipper version otherwise, but make sure the PITB is flased with this.

I basically made sure my klipper/src/rp2040/spi.c had the 6-7 lines updated if you look on the files changed tab here, you can match it up, its a fairly small code file. Then in the main klipper folder I did:

sudo service klipper stop
make clean
make menuconfig
make -j4
python3 ~/katapult/scripts/flashtool.py -i can0 -u eda86b2923a2 -f ~/klipper/out/klipper.bin
sudo service klipper start

This assumes you have it setup with katapult as your bootloader already (following something like esoterical's canbus guide for example), so you can flash over canbus. You obviously need to setup the menuconfig properly for the PITB, like this (again assuming katapult!):

image

Finally you need to make sure the canbus ID in my snippet above is accurate mine is eda86b2923a2

Once youve done this all, then check your klipper log for something like:

Loaded MCU 'PITB' 121 commands (v0.12.0-293-gde93322de-dirty-20240910_171339-Voron24 / gcc: (15:12.2.rel1-1) 12.2.1 20221205 binutils: (2.39-8+rpi1+18) 2.39)
MCU 'PITB' config: ADC_MAX=4095 BUS_PINS_i2c0a=gpio0,gpio1 BUS_PINS_i2c0b=gpio4,gpio5 BUS_PINS_i2c0c=gpio8,gpio9 BUS_PINS_i2c0d=gpio12,gpio13 BUS_PINS_i2c0e=gpio16,gpio17 BUS_PINS_i2c0f=gpio20,gpio21 BUS_PINS_i2c0g=gpio24,gpio25 BUS_PINS_i2c0h=gpio28,gpio29 BUS_PINS_i2c1a=gpio2,gpio3 BUS_PINS_i2c1b=gpio6,gpio7 BUS_PINS_i2c1c=gpio10,gpio11 BUS_PINS_i2c1d=gpio14,gpio15 BUS_PINS_i2c1e=gpio18,gpio19 BUS_PINS_i2c1f=gpio22,gpio23 BUS_PINS_i2c1g=gpio26,gpio27 BUS_PINS_spi0a=gpio0,gpio3,gpio2 BUS_PINS_spi0b=gpio4,gpio7,gpio6 BUS_PINS_spi0c=gpio16,gpio19,gpio18 BUS_PINS_spi0d=gpio20,gpio23,gpio22 BUS_PINS_spi0e=gpio4,gpio3,gpio2 BUS_PINS_spi1a=gpio8,gpio11,gpio10 BUS_PINS_spi1b=gpio12,gpio15,gpio14 BUS_PINS_spi1c=gpio24,gpio27,gpio26 CANBUS_FREQUENCY=1000000 CLOCK_FREQ=12000000 INITIAL_PINS=gpio15 MCU=rp2040 PWM_MAX=255 RECEIVE_WINDOW=192 RESERVE_PINS_CAN=gpio9,gpio8 STATS_SUMSQ_BASE=256 STEPPER_BOTH_EDGE=1
Configured MCU 'PITB' (1024 moves)

the really important key here is:
BUS_PINS_spi0e=gpio4,gpio3,gpio2

if that's not listed, then it didnt include this code change, and thus, you cant use the hardware spi configuration that is needed for the mcu processor to actually talk to the TMC drivers to control your motors. Software spi should work, but I didnt dig into why it actually doesnt, I could probe it and figure it out, but really hardware spi seems superior to me in every way I can think of, why emulate it in software if I dont need to...

Hopefully that helps, I know this is pretty nitty gritty embedded programming stuff that even I dont deal with often, as I'm more an enterprise software dev in my daily life.

Here is a complete snippet (including random commented out old crap etc) of everything in my code related to the PITB, just for posterity, im currently only using it for the A/B motors in my voron 2 (x/y in config), and a few thermal probes, but it is working fine:

[mcu PITB]
canbus_uuid: eda86b2923a2

[temperature_sensor BedSSR]
sensor_type: ATC Semitec 104GT-2
sensor_pin = PITB: gpio27
min_temp: -100
max_temp: 490

[temperature_sensor CHSSR]
sensor_type: ATC Semitec 104GT-2
sensor_pin = PITB: gpio26
min_temp: -100
max_temp: 490

[temperature_sensor pitb_mcu]
sensor_type = temperature_mcu
sensor_mcu = PITB

#####################################################################
#   X/Y Stepper Settings
#####################################################################

##  B Stepper - Left
##  Connected to MOTOR_0
##  Endstop connected to DIAG_0
[stepper_y]
step_pin: PITB:gpio6
dir_pin: PITB:gpio5
enable_pin: !PITB:gpio20
#step_pin: PF13
#dir_pin: !PF12
#enable_pin: !PF14
rotation_distance: 40
microsteps: 16
full_steps_per_rotation:200  #set to 400 for 0.9 degree stepper

endstop_pin: PG6
position_min: 0
##--------------------------------------------------------------------

##  Uncomment for 250mm build
#position_endstop: 250
#position_max: 250

##  Uncomment for 300mm build
#position_endstop: 300
#position_max: 300

##  Uncomment for 350mm build
position_endstop: 350
position_max: 350

##--------------------------------------------------------------------
homing_speed: 25  #Max 100
homing_retract_dist: 5
homing_positive_dir: true


##  Make sure to update below for your relevant driver (2208 or 2209)
# [tmc2209 stepper_x]
# uart_pin: PC4
# interpolate: false
# run_current: 0.8
# sense_resistor: 0.110
# stealthchop_threshold: 0
[tmc5160 stepper_y]
## Soft SPI
spi_bus: spi0e
#spi_software_sclk_pin: PITB:gpio2
#spi_software_mosi_pin: PITB:gpio3
#spi_software_miso_pin: PITB:gpio4
cs_pin: PITB:gpio1
interpolate: False
diag1_pin: PITB:gpio7
run_current: 0.800 
##stealthchop_threshold: 0



##  A Stepper - Right
##  Connected to MOTOR_1
##  Endstop connected to DIAG_1
[stepper_x]
# step_pin: PG0
# dir_pin: !PG1
# enable_pin: !PF15
step_pin: PITB:gpio13
dir_pin: PITB:gpio23
enable_pin: !PITB:gpio22
rotation_distance: 40
microsteps: 16
full_steps_per_rotation:200  #set to 400 for 0.9 degree stepper
endstop_pin: EBB:PB8
position_min: 0
##--------------------------------------------------------------------

##  Uncomment below for 250mm build
#position_endstop: 250
#position_max: 250

##  Uncomment for 300mm build
#position_endstop: 300
#position_max: 300

## Uncomment for 350mm build
position_endstop: 350
position_max: 350

##--------------------------------------------------------------------
homing_speed: 25   #Max 100
homing_retract_dist: 5
homing_positive_dir: true
##  Make sure to update below for your relevant driver (2208 or 2209)
# [tmc2209 stepper_y]
# uart_pin: PD11
# interpolate: false
# run_current: 0.8
# sense_resistor: 0.110
# stealthchop_threshold: 0
[tmc5160 stepper_x] 
# Soft SPI
#spi_speed: 500000
#spi_software_sclk_pin: PITB:gpio2
#spi_software_mosi_pin: PITB:gpio3
#spi_software_miso_pin: PITB:gpio4
spi_bus: spi0e
cs_pin: PITB:gpio21
diag1_pin: PITB:^gpio14
interpolate: True
run_current: 0.8
#hold_current: 0.5
##stealthchop_threshold: 0

Copy link

Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html

There are some steps that you can take now:

  1. Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review
    If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
  2. Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
  3. Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

@KevinOConnor
Copy link
Collaborator

Thanks. We can add this, but we've given up on introducing names like "spi0e". The new scheme is to use names like "spi0_gpio4_gpio3_gpio2" - see src/stm32/spi.c for examples.

Separately, I agree the spi (and i2c) interface would be better if the pins were passed as parameters explicitly from the host (instead of using these weird bus names). (Something like what src/atsamd/sercom.c does.) Something to look at refactoring in 2025.

Cheers,
-Kevin

@VORONENTHUSIAST
Copy link

Thanks for this - got mine working, appreciate the effort undertaken

@KoiosLabs
Copy link
Author

KoiosLabs commented Oct 9, 2024

I can look in a day or two at renaming the buses using the 'current' ("spi0_gpio4_gpio3_gpio2") format, but I definitely dont have the cycles to refactor it like the atsamd example sadly.

I'd be somewhat worried about this breaking a bunch of config files though, at least the approach i used is backwards compatible... whereas if its renamed, anyone using spi0d etc would need to update to the new name, and they seem rather poorly documented imo, I had to come to the spi source to figure it out, and why it wasnt working.

edit: or were you suggesting I just add the new spi0e bus as the new format, and leave the existing ones named as is?

@KevinOConnor
Copy link
Collaborator

or were you suggesting I just add the new spi0e bus as the new format, and leave the existing ones named as is?

Introduce new using the new format. It'd be nice to also add new definitions for the existing buses in addition to the existing definitions. See src/stm32/stm32f0_i2c.c for examples of how one would go about declaring both old and new definitions.

-Kevin

@KoiosLabs
Copy link
Author

I will take a look when I have a few minutes, that sounds like a reasonable approach!

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.

6 participants