Discussion: SPI Master & SDCard updates

[enelson1001]

Background

1. To create an SDCard object with the latest esp-idf requires that the spi-bus-master be initialized using spi_bus_initialize(host, bus_config, dma_chan). The current implementation of smooth::core::io::spi::Master initializes the spi-bus-master as part of the creation of an SPIDevice. The smooth::core::io::spi::Master needs to be updated to separate the spi-bus-master initialization from the creation of an SPIDevice.
2. The spi-bus-master must only be initialized once using - spi_bus_initialize(host, bus_config, dma_chan). If you try and initialize a second time esp-idf will throw an error.
3. In my case I am using the M5Stack which has the LCD and the SDCard or on the same spi-bus (VSPI_HOST).
4. The SDcard class should be modified so the constructor includes which spi_host is being used for the spi-bus-master. The current implementation assumes the spi-host is hspi_host.

Approach #1

In this approach a singleton is used in creating a Master. The Master has an empty constructor and some new methods.

Pros:

1. Allows the initialization of a hspi-bus-master or vspi-bus-master.
2. Separates the creation of an SPIDevice from the initialization of spi-bus-master.
3. Prevents the initializing of a particular spi-bus-master (hspi or vspi) more than once.

Cons:

1. Doesn't guarantee that the spi-bus-mater has been initialized before creating an SPIDevice. We could have two methods for device creation instead of one to eliminate this problem - std::unique_ptr create_hspi_device(Args&& ... args) and std::unique_ptr create_vspi_device(Args&& ... args)
2. Singletons seem to frowned upon in the programming community.

Implementation details:
I placed spi-bus-master initialization inside my DisplayDriver.cpp and inside SDCard.cpp. The spi-bus-master was initialized using the following statement.

         Master::get_instance().initialize_vspi_master(SPI_DMA::DMA_BLOCK_1,         // use DMA
                                                         GPIO_NUM_23,                // mosi gpio pin
                                                         GPIO_NUM_19,                // miso gpio pin  (full duplex)
                                                         GPIO_NUM_18,                // clock gpio pin
                                                         MAX_DMA_LEN);               // max transfer size

The following is the header file MasterV1.

MasterV1.h
#pragma once

#include <memory>
#include <mutex>
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wconversion"
#pragma GCC diagnostic ignored "-Wsign-conversion"
#pragma GCC diagnostic ignored "-Wold-style-cast"
#include <driver/gpio.h>
#include <driver/spi_common.h>
#include <driver/spi_master.h>
#pragma GCC diagnostic pop

namespace smooth::core::io::spi
{
    enum SPI_DMA
    {
        NO_DMA = 0,
        DMA_BLOCK_1,
        DMA_BLOCK_2,
    };

    class MasterV1
    {
        public:
            MasterV1(const MasterV1&) = delete;                     // Copy constructor prohibited  
            MasterV1(MasterV1&&) = delete;                          // Move constructor prohibited
            MasterV1& operator=(const MasterV1&) = delete;          // Assignment operator prohibited
            MasterV1& operator=(MasterV1&&) = delete;               // Move operator prohibited

            static MasterV1& get_instance();

            void initialize_hspi_master(SPI_DMA dma_channel,
                                        gpio_num_t mosi,
                                        gpio_num_t miso,
                                        gpio_num_t clock,
                                        int transfer_size = 0,
                                        gpio_num_t quadwp_io_num = GPIO_NUM_NC,
                                        gpio_num_t quadhd_io_num = GPIO_NUM_NC
                                        );

            void initialize_vspi_master(SPI_DMA dma_channel,
                                        gpio_num_t mosi,
                                        gpio_num_t miso,
                                        gpio_num_t clock,
                                        int transfer_size = 0,
                                        gpio_num_t quadwp_io_num = GPIO_NUM_NC,
                                        gpio_num_t quadhd_io_num = GPIO_NUM_NC
                                        );

            /// Create a device of the given type.
            /// \tparam DeviceType The type of device to create must inherit from SPIDevice
            /// \tparam Args Parameter argument types
            /// \param args Additional arguments that should be passed to the device.
            /// \return A unique pointer to a device, or an empty on failure.
            template<typename DeviceType, typename... Args>
            std::unique_ptr<DeviceType> create_device(Args&& ... args);

        private:
            MasterV1();
            virtual ~MasterV1();

            bool hspi_initialized {false};
            bool vspi_initialized {false};

            std::mutex guard{};
    };

    template<typename DeviceType, typename... Args>
    std::unique_ptr<DeviceType> MasterV1::create_device(Args&& ... args)
    {
        std::unique_ptr<DeviceType> dev;
        std::lock_guard<std::mutex> lock(guard);
        dev = std::make_unique<DeviceType>(guard, std::forward<Args>(args)...);
        return dev;
    }
}

The cpp file for MasterV1.cpp

MasterV1.cpp
#include "smooth/core/io/spi/MasterV1.h"
#include "smooth/core/logging/log.h"

using namespace smooth::core::logging;

namespace smooth::core::io::spi
{
    static constexpr const char* log_tag = "SPIMasterV1";

    MasterV1::MasterV1() {}

    MasterV1::~MasterV1()
    {
        if (hspi_initialized)
        {
            hspi_initialized = false;
            spi_bus_free(HSPI_HOST);
        }

        if(vspi_initialized)
        {
            vspi_initialized = false;
            spi_bus_free(VSPI_HOST);
        }
    }

    MasterV1& MasterV1::get_instance()
    {
        static MasterV1 instance;
        return instance;
    }

    void MasterV1::initialize_hspi_master(SPI_DMA dma_channel,
                                          gpio_num_t mosi,
                                          gpio_num_t miso,
                                          gpio_num_t clock,
                                          int transfer_size,
                                          gpio_num_t quadwp_io_num,
                                          gpio_num_t quadhd_io_num)
    {
        std::lock_guard<std::mutex> lock(guard);

        spi_host_device_t spi_host = HSPI_HOST;
        spi_bus_config_t bus_config;
        bus_config.mosi_io_num = mosi;
        memset(&bus_config, 0, sizeof(spi_bus_config_t));  // zero out otherwise flags may be set causing errors in runtime
        bus_config.mosi_io_num = mosi;
        bus_config.miso_io_num = miso;
        bus_config.sclk_io_num = clock;
        bus_config.quadwp_io_num = quadwp_io_num;
        bus_config.quadhd_io_num = quadhd_io_num;
        bus_config.max_transfer_sz = transfer_size;
        

        if (!hspi_initialized)
        {
            hspi_initialized = spi_bus_initialize(spi_host, &bus_config, dma_channel) == ESP_OK;

            if (!hspi_initialized)
            {
                Log::error(log_tag, "SPI Master HSPI Initialization failed");
            }
            else
            {
                hspi_initialized = true;
                Log::verbose(log_tag, "HSPI Master initialized, Host {}, DMA {}", spi_host, dma_channel);
            }
        }
    }


    void MasterV1::initialize_vspi_master(SPI_DMA dma_channel,
                                          gpio_num_t mosi,
                                          gpio_num_t miso,
                                          gpio_num_t clock,
                                          int transfer_size,
                                          gpio_num_t quadwp_io_num,
                                          gpio_num_t quadhd_io_num)
    {
        std::lock_guard<std::mutex> lock(guard);

        spi_host_device_t spi_host = VSPI_HOST;
        spi_bus_config_t bus_config;
        memset(&bus_config, 0, sizeof(spi_bus_config_t));  // zero out otherwise flags may be set causing errors in runtime
        bus_config.mosi_io_num = mosi;
        bus_config.miso_io_num = miso;
        bus_config.sclk_io_num = clock;
        bus_config.quadwp_io_num = quadwp_io_num;
        bus_config.quadhd_io_num = quadhd_io_num;
        bus_config.max_transfer_sz = transfer_size;
        

        if (!hspi_initialized)
        {
            hspi_initialized = spi_bus_initialize(spi_host, &bus_config, dma_channel) == ESP_OK;

            if (!hspi_initialized)
            {
                Log::error(log_tag, "SPI Master HSPI Initialization failed");
            }
            else
            {
                hspi_initialized = true;
                Log::info(log_tag, "HSPI Master initialized, Host {}, DMA {}", spi_host, dma_channel);
            }
        }
    }

}

Approach #2

Requires the programmer to be more knowledgeable in the operation of SPI. In this approach creation of the SPI device was removed from the Master. The Master would only be used for initialization of the spi-bus-master. The programmer would place the initialization of the Master before object creation of the SDCard and the derived SPIDevice (ex LCDSpi). The object creation of a derived SPIDevice (ex LCDSpi) would be done the normal way and not thru Master::create_device. The mutex would be a variable in SPIDevice and not passed in as part of the constructor.

Pros:
Simplifies SPIMaster and SPIDevice creation.

Cons:
Requires the programmer to be more knowledgeable in SPI operation.

Implementation details:
I placed spi-bus-master initialization in my App.cpp. Placed my LCDSpi instantiation inside my DisplayDriver.cpp Did not do anything special with SDCard upon using the new SDCard.cpp

The following is the header file.

Master.h
#pragma once

#include <memory>
#include <mutex>
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wconversion"
#pragma GCC diagnostic ignored "-Wsign-conversion"
#pragma GCC diagnostic ignored "-Wold-style-cast"
#include <driver/gpio.h>
#include <driver/spi_common.h>
#include <driver/spi_master.h>
#pragma GCC diagnostic pop
#include "smooth/core/logging/log.h"

namespace smooth::core::io::spi
{
    enum SPI_DMA
    {
        DMA_NONE = 0,
        DMA_1,
        DMA_2,
    };

    class Master
    {
        public:
            /// Create a driver for the specified host using the specified I/O pins.
            /// @note Set unused pins to -1.
            /// \param host The host, either HSPI_HOST or HSPI_HOST.
            /// \param mosi Master Output, Slave Input, i.e. data pin for sending data to slave.
            /// \param miso Master Input, Slave Output, i.e. data pin for receiving data from slave.
            /// \param clock Clock pin.
            /// \param transfer_size Maximum transfer size, in bytes. Defaults to 4094 if 0.
            /// \param quadwp_io_num GPIO pin for WP (Write Protect) signal used in D2 4-bit communication modes
            /// \param quadhd_io_num GPIO pin for HD (HolD) used in D3 4-bit communication modes
            Master(spi_host_device_t host,
                   SPI_DMA dma_channel,
                   gpio_num_t mosi,
                   gpio_num_t miso,
                   gpio_num_t clock,
                   int transfer_size = 0,
                   gpio_num_t quadwp_io_num = GPIO_NUM_NC,
                   gpio_num_t quadhd_io_num = GPIO_NUM_NC
                   );

            ~Master();

            /// Initialize the SPI host.
            /// \return true on success, false on failure
            bool initialize();

            /// Create a device of the given type.
            /// \tparam DeviceType The type of device to create must inherit from SPIDevice
            /// \tparam Args Parameter argument types
            /// \param args Additional arguments that should be passed to the device.
            /// \return A unique pointer to a device, or an empty on failure.
            //template<typename DeviceType, typename... Args>
            //std::unique_ptr<DeviceType> create_device(Args&& ... args);

        private:
            /// Do initailization
            /// Performs steps to initialize the SPI Master
            //void do_initialization();

            /// Deinitialize
            /// Perfoms steps to de-initialize the SPI Master
            void deinitialize();

            bool initialized = false;
            std::mutex guard{};
            spi_bus_config_t bus_config{};
            spi_host_device_t host;
            SPI_DMA dma_channel;
    };
/*
    template<typename DeviceType, typename... Args>
    std::unique_ptr<DeviceType> Master::create_device(Args&& ... args)
    {
        std::unique_ptr<DeviceType> dev;

        if (initialize())
        {
            std::lock_guard<std::mutex> lock(guard);
            dev = std::make_unique<DeviceType>(guard, std::forward<Args>(args)...);
        }
        return dev;
    }
*/
}

The following is the cpp file.

Master.cpp
#include "smooth/core/io/spi/Master.h"
#include "smooth/core/logging/log.h"

using namespace smooth::core::logging;

namespace smooth::core::io::spi
{
    static constexpr const char* log_tag = "SPIMasterV3";

    Master::Master(
        spi_host_device_t host,
        SPI_DMA dma_channel,
        gpio_num_t mosi,
        gpio_num_t miso,
        gpio_num_t clock,
        int transfer_size,
        gpio_num_t quadwp_io_num,
        gpio_num_t quadhd_io_num
        )
            : host(host),
              dma_channel(dma_channel)
    {
        bus_config.mosi_io_num = mosi;
        bus_config.miso_io_num = miso;
        bus_config.sclk_io_num = clock;
        bus_config.quadwp_io_num = quadwp_io_num;
        bus_config.quadhd_io_num = quadhd_io_num;
        bus_config.max_transfer_sz = transfer_size;
    }

    Master::~Master()
    {
        deinitialize();
    }

    bool Master::initialize()
    {
        std::lock_guard<std::mutex> lock(guard);
        if (!initialized)
        {
            initialized = spi_bus_initialize(host, &bus_config, dma_channel) == ESP_OK;

            if (!initialized)
            {
                Log::error(log_tag, "Initialization failed");
            }
            else
            {
                Log::verbose(log_tag, "SPI initialized, Host {}, DMA {}", host, dma_channel);
            }
        }
        //do_initialization();

        return initialized;
    }

/*
    void Master::do_initialization()
    {
        if (!initialized)
        {
            initialized = spi_bus_initialize(host, &bus_config, dma_channel) == ESP_OK;

            if (!initialized)
            {
                Log::error(log_tag, "Initialization failed");
            }
            else
            {
                Log::verbose(log_tag, "SPI initialized, Host {}, DMA {}", host, dma_channel);
            }
        }
    }
*/
    void Master::deinitialize()
    {
        if (initialized)
        {
            initialized = false;
            spi_bus_free(host);
        }
    }
}

Approach #3

In this approach the programmer would place the intialization of the Master before object creation of the SDCard and the derived SPIDevice (ex LCDSpi). Then the classes that rely on the SPI Master would have a reference to the Master passed in as part of the constructor of those classes. I did not do this approach because (1) was not sure how it would work in multi thread design and (2) seem to clutter the existing class that rely on the SPI Master.

New SDCard cpp file (not final needs more work)

Note The deinit() method is not shown.

#include "smooth/core/logging/log.h"
#include "smooth/core/filesystem/SDCard.h"
#include "smooth/core/filesystem/FSLock.h"

using namespace smooth::core::logging;

namespace smooth::core::filesystem
{
    bool SDCard::do_common_initialization(const MountPoint& mount_point,
                                          int max_file_count,
                                          bool format_on_mount_failure,
                                          void* slot_config)
    {
        // **********   initialize spi bus master OR the spi bus master should already be initialized

        esp_vfs_fat_sdmmc_mount_config_t mount_config{};
        mount_config.format_if_mount_failed = format_on_mount_failure;
        mount_config.max_files = max_file_count;

        //mount_config.allocation_unit_size = 16 * 1024;

        host = (sdmmc_host_t)SDSPI_HOST_DEFAULT();

        // ********* M5Stack uses VSPI as host,  spi-host should move to constructor ??????????????????
        host.slot = VSPI_HOST;

        sdspi_device_config_t slot_config = SDSPI_DEVICE_CONFIG_DEFAULT();

        // *********** Need to include chip select of of SDCard in constructor ??????????
        slot_config.gpio_cs = GPIO_NUM_4;
        slot_config.host_id = static_cast<spi_host_device_t>(host.slot);

        auto mount_result = esp_vfs_fat_sdspi_mount("/sdcard", &host, &slot_config, &mount_config, &card);
        bool initialized = mount_result == ESP_OK;

        if (initialized)
        //if (mount_result)
        {
            Log::info(TAG, "SD Card initialized");
            sdmmc_card_print_info(stdout, card);
        }
        else
        {
            if (mount_result == ESP_FAIL)
            {
                Log::error(TAG, "Failed to mount the file system.");
            }
            else
            {
                Log::error(TAG, "Failed to initialize SD Card.");
            }
        }

        FSLock::set_limit(max_file_count);

        return initialized;
    }

Recommendations

Are any of these approaches a good staring point for a pull request or did you have something compeletey different in mind?. Let me know how you want to proceed.

[PerMalmberg]

I like the first approach though there's no need for a singleton, I think. Make the members static instead and have the create_-functions be static too. The drawback to that solutions is that you can't destroy the master before program exit, but I don't think that is a problem?

[enelson1001]

This discussion can be closed