Wrong gpio_out_setup during hw spi initialization

[Delsian]

Looks like logical mistake in Python code when SPI initialization required.

Sent 65 68184.429559 68184.429279 7: seq: 10(80), allocate_oids count=2
Sent 66 68184.430139 68184.429819 8: seq: 11(81), set_digital_out pin=PA4 value=1
Sent 67 68184.430923 68184.430363 14: seq: 12(82), config_spi oid=0 bus=0 pin=PA4$
Sent 68 68184.431283 68184.430669 9: seq: 13(83), config_thermocouple oid=1 spi_o$
Sent 69 68184.431723 68184.430878 11: seq: 14(84), finalize_config crc=1574980214

Klippy issues set_digital_out command for CS pin before SPI setup, and this command have gpio_out_setup call inside (line 100 file gpiocmd.c) Later, in config_spi command we're trying to init same gpio again: spi->pin = gpio_out_setup(args[2], 1); (line 39 file spicmds.c) and this second pin setup looks as error because GPIO already initialized. I can prepare fix for this issue, but want to know whish way is better - removing extra GPIO setup before SPI setup, or send "struct gpio_out" with initialized pin to command_config_spi? klippy.log

[Delsian]

The third way is keeping bus.py unchanged (line 21), but add __weak modifier to command_set_digital_out and implement pin state change without pin allocation in target code Probably this way should be better than two previous.

[KevinOConnor]

The setting of the cs pin separately from the configuration of the spi is intentional. When multiple devices share a single spi bus, it's important to make sure all cs pins are set high prior to initializing the spi clock/data lines. Otherwise, a device may interpret the initialization of the clock/data pins as clocking out a request and it could get into an incorrect state. So, the bus.py code is arranged so that all cs pins are initialized prior to the config_spi.

It was not intended for gpio_out_setup() to be exclusive. It was thought that calling gpio_out_setup() multiple times, or even a gpio_out_setup() followed by a gpio_in_setup() would be okay. We could change that (to make it exclusive), but for now I would think the simplest solution would be for the stm32f0 code be changed to not require it to be exclusive. (If I've missed something, or that's hard to do, then let me know.)

Cheers, -Kevin

[Delsian]

Well, I understand the reason of CS setting, and moreover, commit 4a35f927fc1d7b8c3b34f32c1c889fb433752dcc has the same functionality (btw, you can simplify the code by moving CS init into INITIAL_PINS by default). But my proposal has slightly different topic. In stm32f0 code I've implemented GPIO checking functionality to avoid wrong (or by mistake or typo) pin allocation when the same pins used already as SPI or ADC. Sure, it's possible to move such checking one layer higher (to host sw), but in this case, we need to inform host about pin alternative functions. I have no idea how to do this in right way. But anyway - one target has such check, and I hope you'll implement similar func in future for other targets. In this case better to split gpio_out_setup into "gpio_out_setup with using return value" and "gpio_out_set" with only changing initial pin state and allow to use pin's alternative function.

And one another reason: now I'm working on NRF52 target (and control board with only power wires) - NRF chips contain absolutely different pin allocation way, and proposed change in #1437 strongly required for this chip.

[KevinOConnor]

At a high level, I think these types of pin checks should be done at the host level. A number of micro-controllers are starved for memory and code space (eg, the avr chips, the PRU, and some of the SAMD21 chips). So, performing these types of checks in the micro-controller can consume scarce resources that are often better allocated to run-time requirements. Also, reporting descriptive error messages from the micro-controller code is difficult (again, ultimately due to limitations on many of the micro-controllers).

I agree it would be good to provide error messages on SPI, I2C, and UART/USB pin conflicts. At a high-level, my plan to get there looks something like:

1. add enumeration support for pins (just recently implemented)
2. convert spi/i2c buses to use enumerations (also addresses #952)
3. add a DECL_CONSTANT() for each SPI/I2C/etc. bus that lists the reserved pins for that bus
4. update bus.py to read the given constants from the micro-controller and reserve the given pins when a bus is requested.

I'm not familiar with the NRF52, so I can't really provide any advice there. If a particular board would create an electrically unstable state or internally inconsistent state then I think the mcu code should check for that bad state and issue a shutdown(). I think that should be inaddition to host checks. In general, however, if the config just doesn't work as expected, then I'm not sure lots of mcu checks are necessary.

-Kevin

[Delsian]

Thanks, I understand your point of view and agree. I'll remove gpio check from my commit in this case, and close this issue.