The new SpiManager: supporting NRF+CMT+HUAWEI/Meanwell+CAN Battery

[skippermeister]

As @schlimmchen mentioned, there are bittersweet news regarding the PR/feature upstream implementation of the SpiManager

The problem is that the MCP2515 CAN library, like the NRF24 library, is not based on shared SPI access, but uses the Arduino-compatible implementation and need "direct SPI access".

I think the idea of ​​the new Spi Manager is very good and we should do everything we can to port all SPI devices to this SpiManager.

I recognized this early on and am already developing an MCP2515 CAN driver, which uses the shared SPI from the SpiManager. This then replaces the mcp_can library.

I develop the whole thing in my fork.

I will upload a first version of my development in the next few days.

So we shouldn't go back to the SpiPortManager developed by me @skippermeister and @AndreasBoehm

[skippermeister]

My first version of a mcp2515_can library which supports the SpiManager is uploaded.

Please find here https://github.com/skippermeister/MCP2515-CAN-Bus-Library-with-Shared-SPI

An example (sorry not working because it needs the SpiManager library from the project OpenDTU-onBattery) is for illustrating the usage of my library.

The new library replaces the mcp_can library https://github.com/coryjfowler/MCP_CAN_lib currently used in OpenDTU-onBattery.

In my fork https://github.com/skippermeister/OpenDTU-OnBattery the new library is under test.

[LennartF22]

@skippermeister I just had a quick glimpse at the new library and have a few remarks:

1. You can get rid of the pre_cb and post_cb functions by setting spics_io_num in the device_config struct. You should then also get rid of the four lines here and the user parameter in the transactions. In the CMT2300A driver, the callbacks are used for toggling the correct CS line, because it needs two and we don't want to occupy a second SPI device unnecessarily (the ESP32 SPI peripheral only supports three devices per bus).
2. You should set cs_ena_pretrans and cs_ena_posttrans to 0 as the parameter is not supported in full-duplex mode, and the used values were for the CMT2300A specifically. According to the datasheet, the MCP2515 requires the CS line to go low at least 0.5 clock cycles before the first data bit (and go high 0.5 clock cycles after the last bit), but it will most probably be fine without it (the ESP32 already has some delay).
3. I think it would be nicer if the library would simply required an initialized spi_device_handle_t from the user (i.e. OpenDTU), instead of doing the setup itself. When I wrote the SpiManager, it was my intention that the library for interacting with the SPI device does not need any "knowledge" of the SpiManager, the only requirement is that it takes an ESP-IDF SPI device handle. The library could be tested/used in any ESP-IDF project independent of the SpiManager. The SPI cleanup/deinitialization would then also be handled by the user of the library instead of the library itself. A proper cleanup is not implemented for the SpiManager yet though (there is more that would need to be done than just calling spi_bus_remove_device).
4. The string passed to SpiManagerInst.alloc_device is the name for the bus. When two or more devices use the same bus name, they will share the bus. Since an empty string is used in the CMT2300A driver, an empty string should be used here too to share the bus with it. I will create a PR for OpenDTU to optionally pass a bus name for each device via the pin_mapping.json, so that one can control what devices share a bus with each other.
5. The mutex is only needed when OpenDTU accesses the MCP2515 driver from multiple tasks simultaneously, which would ideally be prevented by OpenDTU itself. For the nRF and CMT it is currently necessary to have the locks because the connection check in the web interface of OpenDTU happens in a different task and lacks locking.

@schlimmchen Before this new library starts being used in OpenDTU OnBattery, SpiManagerInst.claim_bus_arduino may be used to claim a bus for the MCP2515 here like this:

auto spi_bus = SpiManagerInst.claim_bus_arduino();
if (!spi_bus)
    return false;

SPI = new SPIClass(*spi_bus);

It will not share the SPI bus with other device then, but at least the MCP2515 can be used with the nRF or CMT for the time being. SpiManagerInst.claim_bus_arduino is currently used for the nRF too, because we cannot pass a spi_device_handle_t to that library yet (we are working on adding that feature to the RF24 library, though!).

[skippermeister]

@LennartF22
Thank you very much for your feedback

I have already implemented a few changes.

to point 1:
I completely agree with you. I set the spics_io_num parameter in the device_config structure variable and the pre_cb and post_cb parameters to nullptr. As well as the two associated macros pre_cb and post_cb deleted.
In all variables trans of type structure spi_transaction_t I set the parameter user to 0.

to point 2:
cs_ena_pretrans and cs_ena_posttrans are unnecessary since I use the non-extended spi_transaction_t structure.

to point 3:
When developing the initialization routines in my library mcp2515_can, I used the cmt_spi3.cpp module. I actually think the initialization implemented there is very good. If the spi_device_handle should be passed to the library as a parameter, I have no idea yet. The handle is generated by SpiManagerInst::alloc_device.
My intention when developing my library was to develop it for the OpenDTU project and not as a general use library

to point 4:
I think it's a good idea to give the shared SPI bus a readable name. In my fork I also gave the CMT2300A the SPI bus name “SPI”.

to point 5:
I am of the opinion that a mutex is necessary here because the IO functions can be addressed by several tasks. This makes it possible to send commands to the charger via the web GUI and MQTT. spi_device_polling_transmit is not thread safe. However, I gave my library its own mutex.

[LennartF22]

@skippermeister

Point 3:
I'm currently working on some additions for the SpiManager, which will most probably change the way it needs to be used/initialized. If your library is supposed to just be copied to the lib folder of the OpenDTU-OnBattery repository, then I guess it's fine to do SpiManager related stuff in the library. However, if the library is supposed to stay in a separate repository, it will be an unnecessary hassle for @schlimmchen when merging upstream changes of the SpiManager to this fork, because then, he would have to create a PR for your Library ahead of time.

Point 4:
I don't intend to change the name of the default shared SPI bus in the upstream, because as mentioned above, that part will most likely be replaced as well (right now, the name is pretty much useless, as its hard-coded instead of being specified by the pin_mapping.json). I would advise you to leave the name at an empty string for now, in order to not create unnecessary merge conflicts.

[skippermeister]

@LennartF22
I changed my library. It is now independent of the SpiManager and expects a spi_device_handle_t as a transfer parameter for initialization. The handle can be created outside, also via the SpiManager.

The initialization sequence can be done as follows:

#include <SpiManager.h>
#include <mcp2515_can.h>

/*
 * stub for initializing the spi handle via the SpiManager
 * /
spi_device_handle_t mcp2515_init_spi(const int8_t pin_miso, const int8_t pin_mosi, const int8_t pin_clk, const int8_t pin_cs, const int32_t spi_speed)
{
    spi_device_handle_t spi;

    auto bus_config = std::make_shared<SpiBusConfig>(
        static_cast<gpio_num_t>(pin_mosi),
        static_cast<gpio_num_t>(pin_miso),
        static_cast<gpio_num_t>(pin_clk)
    );

    spi_device_interface_config_t device_config {
        .command_bits = 0, // set by transactions individually
        .address_bits = 0, // set by transactions individually
        .dummy_bits = 0,
        .mode = 0, // SPI mode 0
        .duty_cycle_pos = 0,
        .cs_ena_pretrans = 2, // only 1 pre and post cycle would be required for register access
        .cs_ena_posttrans = static_cast<uint8_t>(2 * spi_speed / 1000000), // >2 us
        .clock_speed_hz = spi_speed, // 10000000, // 10mhz
        .input_delay_ns = 0,
        .spics_io_num = pin_cs,
        .flags = SPI_DEVICE_HALFDUPLEX,
        .queue_size = 1,
        .pre_cb = nullptr,
        .post_cb = nullptr,
    };

    spi = SpiManagerInst.alloc_device("", bus_config, device_config);
    if (!spi)
        ESP_ERROR_CHECK(ESP_FAIL);

    return spi;
}

MCP2515Can* _CAN;

setup()
{
   // create the spi handle
    spi_device handle_t spi = mcp2515_init_ spi(pin_miso, pin_mosi, pin_clk, pin_cs);

    // initialize the MCP2515 object
    _CAN = new MCP2515Class(spi, pin_irq);

    // initialize the MCP2515 controller
    _CAN-begin(MCP_ANY, CAN_500KBPS, MCP_8MHZ);
    ....
}

loop()
{
    // your application
}