CS Assertion polarity - any support for active high devices?

[michthom]

There was discussion in the comments for #1 about adding the ability to support CS lines that require active high, understandably in the minority so deferred. I'm currently trying to build an SPIDevice to talk to an ST7920 graphic LCD panel, and have code that adapts the venerable https://github.com/JMW95/pyST7920, but fails for (I'm assuming) this reason.
Is there any appetite for a PR that implements an additional parameter, and inverts the logic in __enter__() and __leave__() when requested?
I'm thinking about something like:

def __init__(self,
    spi: Any,
    chip_select: Any = None,
    *,
    cs_active_low: bool = True,
    baudrate: int = 100000,
    polarity: int = 0,
    phase: int = 0,
    extra_clocks: int = 0) -> None

and then in __enter__()

        if self.chip_select:
            self.chip_select.value = False if cs_active_low else True

and __leave__()

        if self.chip_select:
            self.chip_select.value = True if cs_active_low else False

Of course the alternative of inverting the signal in hardware remains an option, however unappealing, if this is a permanently closed topic?

[ladyada]

@tannewt any thoughts? there are some chips having inverted-polarity even tho its quite rare.

[tannewt]

I would do it as cs_active_value: bool = False and then self.chip_select.value = self._cs_active_value and self.chip_select.value = not self._cs_active_value. We'll need to update the native bus device implementation to match: https://github.com/adafruit/circuitpython/blob/main/shared-bindings/adafruit_bus_device/SPIDevice.c

[michthom]

Sounds like a plan - but I'm 100% less confident about a PR against the shared binding stuff. I can take a swing at it, but I'll need to review the contribution guidance first and understand how / if I should link the PRs between the two repositories???

[michthom]

Does this look like what you were expecting @tannewt?

Adafruit_CircuitPython_BusDevice/adafruit_bus_device/spi_device.py:

--- ./Adafruit_CircuitPython_BusDevice_orig/adafruit_bus_device/spi_device.py	2021-09-21 19:43:02.000000000 +0100
+++ ./Adafruit_CircuitPython_BusDevice/adafruit_bus_device/spi_device.py	2021-09-21 19:48:39.000000000 +0100
@@ -55,6 +55,7 @@ class SPIDevice:
        spi,
        chip_select=None,
        *,
        cs_active_value=False
        baudrate=100000,
        polarity=0,
        phase=0,
@ -66,6 +67,7 @@ class SPIDevice:
        self.phase = phase
        self.extra_clocks = extra_clocks
        self.chip_select = chip_select
        self.cs_active_value = cs_active_value
        if self.chip_select:
            self.chip_select.switch_to_output(value=True)

@ -76,12 +78,12 @@ class SPIDevice:
            baudrate=self.baudrate, polarity=self.polarity, phase=self.phase
        )
        if self.chip_select:
            self.chip_select.value = False
            self.chip_select.value = self.cs_active_value
        return self.spi

    def __exit__(self, exc_type, exc_val, exc_tb):
        if self.chip_select:
            self.chip_select.value = True
            self.chip_select.value = not self.cs_active_value
        if self.extra_clocks > 0:
            buf = bytearray(1)
            buf[0] = 0xFF

and circuitpython/shared-bindings/adafruit_bus_device/SPIDevice.c:

--- ./circuitpython_orig/shared-bindings/adafruit_bus_device/SPIDevice.c	2021-09-21 19:45:09.000000000 +0100
+++ ./circuitpython/shared-bindings/adafruit_bus_device/SPIDevice.c	2021-09-21 21:16:53.000000000 +0100
@@ -73,10 +73,11 @@
 STATIC mp_obj_t adafruit_bus_device_spidevice_make_new(const mp_obj_type_t *type, size_t n_args, const mp_obj_t *pos_args, mp_map_t *kw_args) {
     adafruit_bus_device_spidevice_obj_t *self = m_new_obj(adafruit_bus_device_spidevice_obj_t);
     self->base.type = &adafruit_bus_device_spidevice_type;
-    enum { ARG_spi, ARG_chip_select, ARG_baudrate, ARG_polarity, ARG_phase, ARG_extra_clocks };
+    enum { ARG_spi, ARG_chip_select, ARG_cs_active_value, ARG_baudrate, ARG_polarity, ARG_phase, ARG_extra_clocks };
     static const mp_arg_t allowed_args[] = {
         { MP_QSTR_spi, MP_ARG_REQUIRED | MP_ARG_OBJ },
         { MP_QSTR_chip_select, MP_ARG_OBJ, {.u_obj = MP_OBJ_NULL} },
+        { MP_QSTR_cs_active_value, MP_ARG_KW_ONLY | MP_ARG_BOOL, {.u_bool = false} },
         { MP_QSTR_baudrate, MP_ARG_KW_ONLY | MP_ARG_INT, {.u_int = 100000} },
         { MP_QSTR_polarity, MP_ARG_KW_ONLY | MP_ARG_INT, {.u_int = 0} },
         { MP_QSTR_phase, MP_ARG_KW_ONLY | MP_ARG_INT, {.u_int = 0} },
@@ -87,7 +88,7 @@
 
     busio_spi_obj_t *spi = args[ARG_spi].u_obj;
 
-    common_hal_adafruit_bus_device_spidevice_construct(MP_OBJ_TO_PTR(self), spi, args[ARG_chip_select].u_obj, args[ARG_baudrate].u_int, args[ARG_polarity].u_int,
+    common_hal_adafruit_bus_device_spidevice_construct(MP_OBJ_TO_PTR(self), spi, args[ARG_chip_select].u_obj, args[ARG_cs_active_value].u_bool, args[ARG_baudrate].u_int, args[ARG_polarity].u_int,
         args[ARG_phase].u_int, args[ARG_extra_clocks].u_int);
 
     if (args[ARG_chip_select].u_obj != MP_OBJ_NULL) {```

[michthom]

I've forked both circuitpython and Adafruit_CircuitPython_BusDevice and put the proposed patches into a branch on each. I'm not confident of how to build / test the result on my end, I'm sorry to say. Are you willing for me to open a PR on the two Adafruit repos to pull in my branches based on the diff above or does it need more scrutiny / work on my end?

[michthom]

I'm in the process of building circuitpython under Ubuntu on RaspberryPi (my target platform will be Raspberry Pi OS but I took the guidance on the learn site at face value).
When it's syncing the submodules, it's grabbing the default unpatched BusDevice submodule as part of that process (reasonable!).
Cloning into '/home/ubuntu/circuitpython/frozen/Adafruit_CircuitPython_BusDevice'.

• Does this mean I'll need to manually patch that frozen version before building circuitpython?
• Is there a way to tell circuitpython to use my patched version of the BusDevice submodule directly?
• Or can I subsequently override the version bundled with circuitpython from my patched repo?

Sorry if these are dumb questions, but this is all still rather new to me...

[jerryneedell]

The search path looks at the root folder before looking at the "frozen" folder, so if you put you new version in the root folder "/" on the CIRCIUTPY drive, it will be used instead of the "frozen" version.
so copy your modified "adafruit_busdevice" folder to the CIRCUITPY drive.

[jerryneedell]

ah -- but I forgot, then there is now a "core module" for bus_device and I am not sure how you override that , someone more familiar with that my be able to help. Sorry for any confusion.

You may need to disable CIRCUITPY_BUSDEVICE in your build so it uses the Python Module instead fo the Core Module.

[michthom]

Thanks @jeryneedell. I'm wondering if I need to follow the build instructions, but then after pulling the submodules in from the default location, I remove the Adafruit_CircuitPython_BusDevice directory and clone from my repo with the patches (before compiling)?
According to this page I'd need a release tag on my branch to avoid errors... Not sure my code is quite ready for release yet (though at least it compiles without any errors now).

[michthom]

I've opened a PR against this repo and also adafruit/circuitpython for the corresponding shared module / shared binding code. Treat me gently as I'm struggling to test the code - while I was able to compile cleanly for a RP2040 board, I'm baffled as to how I actually take the compiled circuitpython code and test it on my target platform (Raspberry Pi 3B+)?