RFC: Multi CS Spi Implementation

[Vollbrecht]

trafficstars

Design Goals:

• Have a Driver implementation that allows multiple Devices.
• Have a Mechanism that allows iteration of all devices but also allow to target only a specific Device, or a subset
• Allow different Devices configs for each Device
• Driver should manage the adding and removing of Devices and allow more then the esp-idf limit of 3 Devices per Bus.

Current Implementation Details:

• The Driver itself stores all devices itself in an HashMap. This allows to reference all Devices by an id( currently the cs pin number as i32)

  • Pros: enables easy iteration over devices and a single device can be referred to with there id
  • Cons:
    • the type of the collection is of SpiSlave -> This approach dont allow different SpiDevice impl in a single collection
    • the Driver still cant work with other Drivers that impl an SpiDevice

• The Device id is an i32 that represent the cs pin number

  • Pros:
    • simple concept
    • easy to use a specific device by using an number
  • Cons:
    • it is only linked to the real gpio pin on creation.
    • users can enter an arbitrary device-id this has to be checked at runtme / no 0 cost magic :disappointed:
    • device id is not owned by anything / just a number

• Allowing more than 3 Devices per bus comes with the overhead of continuously keeping track of the registered bus_devices. To make it a bit more refined a VeqDeque is used to remove the oldest not used device from the bus. If a Device is used multiple times in a row it can still reuse the spi_device_handle given by rtos

• don't support DMA

• all possible Devices will be created when SpiMasterDriver::new() is called. no adding or removing devices after that.

Current Usage Example:

fn main() -> Result<(), EspError>{
    // Temporary. Will disappear once ESP-IDF 4.4 is released, but for now it is necessary to call this function once,
    // or else some patches to the runtime implemented by esp-idf-sys might not link properly.
    esp_idf_sys::link_patches();

    esp_idf_svc::log::EspLogger::initialize_default();

    let peripherals = Peripherals::take().unwrap();
    let spi = peripherals.spi2v2;
    let sclk = peripherals.pins.gpio2;
    let tx = peripherals.pins.gpio8;
    let rx = peripherals.pins.gpio9;

    // create a collection of gpio pins by refering to them
    // with there gpio number 
    // representing all cs pins
    let cs_pin_numbers = [0, 1, 10, 3, 4, 5, 6, 7, 11];
    let mut cs_pins: Vec<AnyIOPin> = Vec::with_capacity(10);
    for &n in &cs_pin_numbers {
        unsafe {
            cs_pins.push(AnyIOPin::new(n));
        }
    }
    // create device configs for every cs_pin 
    // will be zipped to the cs_pin inside the driver 
    let mut conf_list = Vec::with_capacity(10);
    for _ in &cs_pins {
        let conf = config::Config::new().baudrate(10.MHz().into());
        conf_list.push(conf);
    }
    let mut spi = SpiMasterDriver::<SPI2>::new(spi, sclk, tx, Some(rx), cs_pins, conf_list).unwrap();

    let mut buf = [0u8; 4];
    let mut read = [0u8; 4];
    let write = [0xde, 0xad, 0xbe, 0xef];

    loop {        
        // we can iterate over different spi_devices by refering to them
        // with there cs_pin gpio number as an device id
        for p in cs_pin_numbers {
            thread::sleep(Duration::from_millis(500));
            spi.transaction(p, |bus| bus.transfer(&mut read, &write))?;
            println!("SPI Device {:?} -> Wrote {:x?}, read {:x?}", p, write, read);
        }
        // we can also directly target an devices via its cs_pin gpio number as an id
        for _ in 0..5 {
            spi.write(0, &write)?;
            spi.read(0, &mut read)?;
            spi.transfer(1, &mut read, &write)?;
            spi.transfer_in_place(1, &mut buf)?;
        }
    }
}

Open Questions

• How could an alternative look like that enable the Driver to take generic SpiDevices?
• Alternatives to the Device ID as an i32 ? Some Pin Magic ?
• How should a better Error Handling look like?
• Should the Driver be behind a feature flag ? ( Dont Change current Driver for low overhead if one dont need multi cs)
• How does a better naming Scheme for the Driver and the internal SpiSlave struct could look? (to make it possible to merge with the default driver)
• Any possible No-Go's you think ?

Todo

• [ ] ErrorHandling: Replace the println Errors with Specif Errors
• [ ] merge it from spi_v2.rs into spi.rs

[Dominaezzz]

Imo it's rather inconvenient that you have to create all the devices upfront. One should be able to create a bus then create devices using the bus.

[Vollbrecht]

Imo it's rather inconvenient that you have to create all the devices upfront. One should be able to create a bus then create devices using the bus.

I see what you mean. The current implementation does this to make it a simpler implementation. It is easy to add the functionality to add new devices. But i hesitated a bit because of the following implications:

• If one wants an dedicated add devices fn, one maybe want also an dedicated rm device fn.
• In practice i think most of the time devices connected via spi dont change that much and could be defined somewhat at a single point in time.

I think the 2nd point would only not apply if you have some sort of plugable spi devices or your devices needs two different configurations for initialization. Do we need to support such a case is a question. The first point is somewhat more complicated. Allowing adding new devices should not be a big problem. The unique cs pin should help here. But if we allow removing a device and not let it handle internally by the driver, we need an additional guard, that only removes the device if it is currently not in use ( currently in an transaction - still blocking the bus - holding an bus device handle) so no race condition can happen.

If we implement an option do add devices question is how this should change the interface. Should a ::new() call to spimasterdriver then never get an list of initial devices and add every device by some add_device fn? We than need an additional runtime check, checking if we have at least one devices when starting to iterate over the devices. But the call to new would be a bit cleaner for that. A 2nd option is to allow both. Make it with some initial devices than we don't need the check but allow appending.

What do you think?

[Vollbrecht]

Another problem when allowing an dedicated rm of a device -> Because the identifier for an device is just an i32 this is a weak link. An device could be removed but someone still wants to talk to that device using an i32. This call would fail. Here the simplicity of the i32 don't look to good, we than need to inform the cally about that. So allowing an rm of a device needs the existence of an stronger link. But the question is does we really need to rm devices at runtime separated from removing the bus? If we dont need it we should be fine they way it is. Currently all devices gets removed when the bus gets removed.

[Dominaezzz]

I think the 2nd point would only not apply if you have some sort of plugable spi devices or your devices needs two different configurations for initialization. Do we need to support such a case is a question.

This is required for SD cards. When an SD card is to be used, it has to be configured to be in SPI mode, doing this requires sending some SPI signals to it much slower than usual, after which the SD card can be communicated with in SPI mode and at much higher speeds. Achieving this with esp-idf means creating a device twice, once with the slow config and then again with the fast config. Though SD cards do require the entire bus and not a CS pin but that can be designed later. Device recreation will do for now.

But if we allow removing a device and not let it handle internally by the driver, we need an additional guard, that only removes the device if it is currently not in use ( currently in an transaction - still blocking the bus - holding an bus device handle) so no race condition can happen.

Yes I imagine this would be another guard type like so.

let bus = SpiMasterDriver::new(.....)?;
let device1 = SpiDevice::new(&bus, cs1)?;
let device2 = SpiDevice::new(&bus, cs2)?;

We than need an additional runtime check, checking if we have at least one devices when starting to iterate over the devices. But the call to new would be a bit cleaner for that.

I don't understand the need for a runtime check or iteration. Each device can keep track of it's own stuff and the bus doesn't need to iterate.

But the question is does we really need to rm devices at runtime separated from removing the bus?

Yes. My SD card example requires this at least.

Here the simplicity of the i32 don't look to good, we than need to inform the cally about that.

An issue with using an i32 is the device can't then conform the embedded-hal's SpiDevice trait, which makes using drivers that require spi difficult.

[Dominaezzz]

Driver should manage the adding and removing of Devices and allow more then the esp-idf limit of 3 Devices per Bus.

I missed this when I initially looked at the PR, so I was confused at the device list tracking but I see the goal now. I just want to note that the device tracking comes at the cost of safe concurrency, since each operation requires a mutable reference. Some users will want to talk to multiple SPI devices in parallel which esp-idf allows.

Another way to surpass the esp-idf limit of 3 devices per bus is to do software CS. This means creating the spi bus without the CS pin, then manually toggling the CS pin outside the driver. embedded-hal allows for this with the SpiBus trait, so 3rd party libraries can implement bus sharing with unlimited devices. Doing it this way will allow for concurrency but it will be at the cost of the overhead managing CS in software. At least users will have the option of hardware CS when they have up to 3 devices or software CS when they have more than 3 devices.

[Vollbrecht]

Thank you for your feedback. I tried really hard to create something that works without the i32 id's and have a similar interface like you proposed. I have a first working version now ready :tada: There are now some things i need to get rid of again. I had a hard time fighting some circular life times. currently i cheated and created some internal static that lives long enough for all the SpiDevices and the SpiMaster. I need to eliminate that now and make it work Generic for all SPI's not only SPI2.

[Vollbrecht]

Current Interface looks something like the following

    let spi = SpiMaster2::new::<SPI2>(spi, sclk, tx, Some(rx));
    spi.init_master();

    let mut slave1 = SpiSlave::new(cs_pins.pop().unwrap(), conf_list[0])?;
    let mut slave2 = SpiSlave::new(cs_pins.pop().unwrap(), conf_list[1])?;
    let mut slave3 = SpiSlave::new(cs_pins.pop().unwrap(), conf_list[2])?;
    let mut slave4 = SpiSlave::new(cs_pins.pop().unwrap(), conf_list[3])?;

[Dominaezzz]

It looks like the only use of the global static is to make sure that all devices a free'd before the bus is free'd. The Rust compiler is able to do this for you. So if you take a reference to the bus when creating the SpiSlave (confusing name, I though this was referring to the SPI slave API), the Rust compiler will complain if the user tries to free the bus before the devices.

[Dominaezzz]

Let me know (here or on Matrix) if you need any help. I'd like this PR to land ASAP or within the next few weeks before the next version is released. This is a fantastic opportunity to make breaking changes to the existing SPI interface since the next version will already contain breaking changes.

[Vollbrecht]

I got it down now for the most part. It is fully functional and i tested it for 11 parallel SPI devices. Usage Interface currently looks like the following:

let spi = SpiMasterDriver::new::<SPI2>(spi, sclk, tx, Some(rx))?;
let spi = Arc::new(Mutex::new(spi));

let slave1 = EspSpiDevice::new(spi.clone(), pin, conf)?;
let slave2 = EspSpiDevice::new(spi.clone(), pin, conf)?;
....
let slave12 = EspSpiDevice::new(spi.clone(), pin, conf)?;

pros of the current design:

• you can add or delete devices as you like
• a device could have an update_conf function that updates for example the bus speed for that device "on the fly"
• should be possible to use over thread bounds

cons :

• all lifetimes needed to be checked at runtime
• because we use the managed cs pins - we are forced to track all the valid handles this leads to the situation that every SpiDevice needs ability to revoke a handle from another when not in use to get one handle for itsel. -> Central List with a reference to all SpiDevices -> Because Device need to be in the list we only can have an &T or an &mutT but not an owned T of the Device -> I opted for an Arc ( could be updated to an Arc(Mutex) or an non thread safe equivalent aka Rc / RefCell ) -> Every SpiDevice gives you an Arc<SpiDevice>. In Practice i don't see a problem here because we can use all the SpiDevice Fn's [transaction, write, read ...] just fine that way.

there are still some cleanups to do but there is one other problem i have to investigate... i noticed when a device gets removed from the bus via esp-idf spi_bus_remove_device() fn the cs_line gets driven low for a short amout of time. that would be not accaptable when frequent device removes are happening.

for compairsion i will later implement a version with software managed cs pins. after that i will have a look to make the driver async rdy

[Dominaezzz]

It's possible to improve the lifetime situation still. I see now that every device now requires a Arc<Mutex<SpiMasterDriver>> to be created. First improvement is to move the mutex bit into the SpiMasterDriver so it's internal. No need to have the user create a non-optional mutex when it can be hidden away. Now user just needs a Arc<SpiMasterDriver> to create devices.

Second improvement is to remove the forced use of Arc, not all users will want to pay the cost of runtime tracking and/or atomics. To allow flexibility, device constructor should take a Borrow<SpiMasterDriver> instead. This will require another generic parameter but it means that user could provide a SpiMasterDriver, &SpiMasterDriver, Rc<SpiMasterDriver>, Arc<SpiMasterDriver>, etc. and rust compiler would make sure the bus lives as long as it needs to and that it can only be shared amongst threads if Arc or similar primitive is used.

[Vollbrecht]

Second improvement is to remove the forced use of Arc, not all users will want to pay the cost of runtime tracking and/or atomics. To allow flexibility, device constructor should take a Borrow<SpiMasterDriver> instead. This will require another generic parameter but it means that user could provide a SpiMasterDriver, &SpiMasterDriver, Rc<SpiMasterDriver>,

Need i didn't now about Borrow. I will try to implement it asap. Thanks for the Input :smiling_face_with_three_hearts:

[Vollbrecht]

hmm i am kinda stuck, i don't think it is possible to use Borrow<SpiMasterDriver>. Or maybe i am not understanding it correctly. The problem where it all breakes apart is that i want to store the Borrow in the struct and that is not easyly possible in this special situation. Maybe i am not understanding it correctly but here is an example:

If only one struct has a reference to another struct this works as expected

struct slave<T> {
    master: T
}
impl<T> slave<T>
where T: Borrow<Master> {
    fn new(master: T) -> Self {
        slave{master}
    }
}
struct Master {
    something: i32
}
impl Master  {
    fn new() -> Self {
        Master{42}
    }
}

But my problem is because we also have a reference from slave inside master this breakes. I got the following circular problem. No i need to make Master Generic over slave<T> -> this feeds back to slave that needs now a generic Master<Y>.

struct slave<T> {
    master: T
}
impl<T,Y> slave<T>
where 
    T; Borrow<Master<Y>>
    Y; Borrow<Slave<T>> 
    {
    fn new(master: T) -> Self {
        slave{master}
    }
}
struct Master<Y> {
    slave: Weak<Y>
}
impl<Y> Master<Y>  
where
    T; Borrow<Master<Y>>
    Y; Borrow<Slave<T>> 
   {
    fn new() -> Self {
        Master{Weak::new()}
    }
}

can you think of an solution to this problem ?

[Dominaezzz]

Well, the ideal solution would be to remove the tracking completely and leave the user to handle unlimited CS pins themselves. 😛

The solution you're looking for is to not hold a reference to the device directly in the bus but instead hold a reference to the data inside the device that you need in the bus i.e. the RefCell<Option<spi_device_handle_t>>.

[Vollbrecht]

The solution you're looking for is to not hold a reference to the device directly in the bus but instead hold a reference to the data inside the device that you need in the bus i.e. the RefCell<Option<spi_device_handle_t>>.

thats not the problem here. the problem arises because one device needs to revoke an handle from another device when there is currently no empty handle free. so there have to be a place for all the devices to look at and revoke a handle from another device. otherwise every SpiDevice works complitly autonomous. Alternativly each devices has to give up its handle after usage. but this would make it more bad lots of device_rm_from_bus calls :sob: i dont like a software cs solution, its to "blocking" for me . but maybe when making it all async i just dont have to care :see_no_evil:

[Dominaezzz]

Sorry, I missed the fact that the EspDevices are actually wrapped in Arc as well. (pro tip: Don't read code on your phone) You can move the Arc down into the EspDevice.

So instead of

pub struct EspSpiDevice{
    handle: RefCell<Option<spi_device_handle_t>>,
    // .....
}

pub struct SpiMasterDriver {
    devices: HashMap<i32, RefCell<Weak<EspSpiDevice>>>,
    // ....
}

let device: Arc<EspSpiDevice>

you can just do this

pub struct EspSpiDevice {
    handle: Arc<RefCell<Option<spi_device_handle_t>>>,
    // .....
}

pub struct SpiMasterDriver {
    devices: HashMap<i32, RefCell<Weak<RefCell<Option<spi_device_handle_t>>>>>,
    // ....
}
let device: EspSpiDevice

this way you don't need the master to be generic based on the device.

[Dominaezzz]

i dont like a software cs solution, its to "blocking" for me . but maybe when making it all async i just dont have to care :see_no_evil:

Well, what you currently have is kinda software CS, since you're having to destroy the device and recreate it, no? Though I suppose it's slightly better than plain software CS. What happens when 4 devices try to send a big write transaction at the same time? Won't this mean the 4th device will destroy the 1st device before it's transaction is over? Which would be very bad.

[Vollbrecht]

i dont like a software cs solution, its to "blocking" for me . but maybe when making it all async i just dont have to care see_no_evil

Well, what you currently have is kinda software CS, since you're having to destroy the device and recreate it, no? Though I suppose it's slightly better than plain software CS. What happens when 4 devices try to send a big write transaction at the same time? Won't this mean the 4th device will destroy the 1st device before it's transaction is over? Which would be very bad.

The trick is that i dont destroy devices. All devices life as long as they like only there handles get set to None from other Devices if it is needed. That is the hole "trick". (Or do you mean when releasing in RTOS?) Since SPI is inherently Serial it is only possible that only one Device can send at a Time. If Device 4 sends for a very large time, all others have to wait for it. That's why i guard the SpiMasterDriver with a Mutex on transaction, while transaction is going no other device can switch out a handle if it has none. There is no ques option since in the current state the driver already only utilize a blocking transfer with spi_device_aquire_busand spi_device_release_bus. Your suggested solution to remove the EspSpiDevice pointer from the HashMap could work though when each Device checks out if it still finds its handle in the HashMap and so know that its valid before each transaction ... Man this is an over engineered mess :clown_face: I have this special hardware board that have 11 SpiDevices on one bus so thats why i am pushing this a bit :joy_cat:

[Vollbrecht]

At least its not a total disaster:

• We got a seperation from SpiDevice to SpiMasterDriver -> that allows multiple Devices :tada:
• if one only ever create max 3 devices on esp32 / s3 or max 6 devices on c3 we dont need the complete tracking - can delete all the hashmap stuff
• if we have more than that devices -> use software cs ?

How does this sound ? question is now how can we detect how many SpiDevices are there? Would like to have a nice compile time solution. What do you think ?

[Dominaezzz]

Or do you mean when releasing in RTOS?

Yes, 4 devices on 4 different threads. Though since the ESP is limited to 2 cores with cooperative scheduling it's probably not possible. Though it becomes possible when this becomes async and uses interrupt instead.

Since SPI is inherently Serial it is only possible that only one Device can send at a Time.

Yes but ideally we want to queue up all the transactions ahead of time to reduce software overhead in the peripheral. Only matters for DMA though, which you don't have yet but it'll have to be added.

That's why i guard the SpiMasterDriver with a Mutex on transaction, while transaction is going no other device can switch out a handle if it has none.

Where is this? I don't see this guard being used during the transaction. The guard is only used to get a handle. So the first 3 devices will start their transactions without creating a guard since they already have a handle. The 4th device though, will create a guard to get a handle but the guard won't block since no other device has created a guard. :boom:

I have this special hardware board that have 11 SpiDevices on one bus so thats why i am pushing this a bit

Ohhhh, you should've said so. I would've been more helpful with the idea if you did. So this hole "trick" can be safely implemented on top of the thin shim you just suggested in your last comment.

How does this sound ?

Good and you can still implement the "hole trick" on top (as a wrapper) of what you've just described. I'm happy to help with this as well as I think it's a cool feature but one that should be optional.

question is now how can we detect how many SpiDevices are there? Would like to have a nice compile time solution.

I'd say not to bother with this. esp-idf returns an error when the user creates too many devices on the same bus which will be good enough, since the user knows to handle it. For now let's leave software CS for another PR.

For what I currently have in mind. I think this PR should just be simple multi device implementation with hardware CS (and no tracking).

    let spi = SpiMaster2::<SPI2>::new(spi, sclk, tx, Some(rx));

    let mut slave1 = SpiSlave::new(&spi, cs_pins.pop().unwrap(), conf_list[0])?;
    let mut slave2 = SpiSlave::new(&spi, cs_pins.pop().unwrap(), conf_list[1])?;
    let mut slave3 = SpiSlave::new(&spi, cs_pins.pop().unwrap(), conf_list[2])?;

Then in a separate file in this PR (or another PR), we can have a pooling struct.

    let spi = SpiMaster2::<SPI2>::new(spi, sclk, tx, Some(rx));
    let spi = SpiMasterPool::new(spi);

    let mut slave1 = SpiPoolSlave::new(&spi, cs_pins.pop().unwrap(), conf_list[0])?;
    let mut slave2 = SpiPoolSlave::new(&spi, cs_pins.pop().unwrap(), conf_list[1])?;
    let mut slave3 = SpiPoolSlave::new(&spi, cs_pins.pop().unwrap(), conf_list[2])?;
    let mut slave4 = SpiPoolSlave::new(&spi, cs_pins.pop().unwrap(), conf_list[3])?;

SpiPoolSlave will hold a RefCell<Option<SpiSlave>> in it, if that makes sense.

This way we can have the best of both worlds! Also, this way the Rust compiler can highlight any bugs like the one I described above.

[Vollbrecht]

Api example for the SpiConfigPool with SpiPoolDevice

    let cs_pin_numbers = [0, 1, 10, 3, 4, 5, 6, 7, 11, 20, 21];
    let mut cs_pins: Vec<AnyIOPin> = Vec::with_capacity(11);
    for &n in &cs_pin_numbers {
        unsafe {
            cs_pins.push(AnyIOPin::new(n));
        }
    }
    let spi = SpiMasterDriver::new::<SPI2>(spi, sclk, tx, Some(rx))?;
    
    let mut device_configs = HashMap::new();
    for idx in 1..=6 {
        let val = rng.gen_range(1..10000);
        let conf = config::Config::new().baudrate(val.kHz().into());
        device_configs.insert(idx, conf);
    }

    let pool = SpiConfigPool::new(&spi, device_configs)?;
    let mut slaves = Vec::new();

    let mut rng = thread_rng();
    for pin in cs_pins {
        let val = rng.gen_range(1..=6);
        let slave = SpiPoolDevice::new(&pool, val, pin)?;
        slaves.push(slave);
    }
    
   loop {
        for slave in &mut slaves {
            println!("Enter Slave {:?}", slave.pin_driver.pin());
            let mut read = [0u8; 4];
            let write = [0xde, 0xad, 0xbe, 0xef];

            slave.transaction(|bus| bus.transfer(&mut read, &write))?;
            println!(
                "SPI Device {:?} -> Wrote {:x?}, read {:x?}",
                slave.pin_driver.pin(),
                write,
                read
            );
        }
        thread::sleep(Duration::from_millis(10));
    }
    

[Vollbrecht]

given the following code. is this a correct way to use the criticalsection to recreate an mutex'ish behaviour. (given that no one manual overrides the locked AtomicBool)

struct SpiLock {
    locked: AtomicBool,
    guard: CriticalSection,
}
impl SpiLock {
    fn new() -> Self {
        Self{
            locked: AtomicBool::new(false),
            guard: CriticalSection::new(),
        }
    }
    fn is_locked(&self) -> bool {
        self.locked.load(Ordering::SeqCst)
    }
}

pub struct SpiMasterDriver<'d> {
    lock: SpiLock
    // ..ommited.. //
}

// fn on SpiDevice struct
pub fn transaction<R,E>( ) -> Result<R,E>
where T: Borrow<SpiMasterDriver> 
{
    let master: &SpiMasterDriver = self.driver.borrow();

    // lock variant 1 : Spinlock -> potential deadlock risk without timeout/yield?
    while !master.lock.is_locked() {
        let _guard = master.lock.guard.enter();
        if !master.lock.is_locked() {
            master.lock.locked.store(true, Ordering::SeqCst)
        } else {
            panic!("guard didnt protect us. someone else locked while we trying to lock");
        }            
    }

    // lock variant 2 : with yielding
    if !master.lock.is_locked() {
        let _guard = master.lock.guard.enter();
        if !master.lock.is_locked() {
            master.lock.locked.store(true, Ordering::SeqCst)
        } else {
            panic!("guard didnt protect us. someone else locked while we trying to lock");
        }            
    } else {
        println!("yielding from spi transaction");
        // problem with master borrow variable: is it still valid after we continue ? if we continue do we start after yield (not locking error)?
        do_yield();
    }
    
    // DO STUFF

    // unlock
    if master.lock.is_locked() {
        let _guard = master.lock.guard.enter();
        if master.lock.is_locked() {
            master.lock.locked.store(false, Ordering::SeqCst)
        }
    } else {
        panic!("We got unlocked from somewhere else while we still should be locked");
    }
}

[ivmarkov]

given the following code. is this a correct way to use the criticalsection to recreate an mutex'ish behaviour. (given that no one manual overrides the locked AtomicBool)

I think you are over-complicating it, in that what you really need is actually just a critical section, not a mutex. The difference is blurry, and one can be emulated with another, but it boils down to whether you want to synchronize the execution of a piece of code (as in just one thread can be executing it at a time) = critical section, or you want to synchronize access to a piece of data (= mutex).

Or in other words, a critical section is like a mutex, except it does not wrap the data it locks (if there is data).

But perhaps your biggest confusion is that somehow esp_idf_hal::task::CriticalSection does something very different from a Rust STD Mutex. It doesn't. They are pretty much equivalent, in that CriticalSection is using a FreeRtos mutex behind - just like the Rust STD mutex - so I'm not sure what you are trying to protect against with all these spinlocks, do_yield and suchlike?

Also not sure from where your "deadlock" concerns are coming? Deadlocks are a thing when you use multiple critical sections / mutexes as then you risk deadlocking by acquiring these in a different order. Deadlocking can never occur if you use a single mutex only.

Or roughly:

pub struct SpiMasterDriver<'d> {
    cs: CriticalSection,
    // ..ommited.. //
}

// fn on SpiDevice struct
pub fn transaction<T, R, E>(&self ) -> Result<R, E>
where T: Borrow<SpiMasterDriver> 
{
    let master: &SpiMasterDriver = self.driver.borrow();

    let _guard = master.cs.enter();

    // DO STUFF

    // unlocking happens automatically, when the `_guard` var is dropped
}

[ivmarkov]

OK. Forgot the one major difference between our CriticalSection implementations and a standard STD mutex:

• Our critical sections are re-entrant (as in if a thread already keeps the lock of a critical section, entering it a second time will not crash / block). This is achieved by utilizing the "recursive mutex" variant of FreeRtos mutex.
• STD Mutex utilizes a standard mutex, and as such calling its lock method twice will lead to UB

Perhaps this was your concern with "deadlocking".

[Vollbrecht]

But perhaps your biggest confusion is that somehow esp_idf_hal::task::CriticalSection does something very different from a Rust STD Mutex. It doesn't

thx for the feedback! i somehow thought that for the lifetime of the guard interrupts were disabled so i tried to only be a short amount of time in the guard updating the value and leave. this is of course wrong, i misread the code there :sweat_smile:

[ivmarkov]

But perhaps your biggest confusion is that somehow esp_idf_hal::task::CriticalSection does something very different from a Rust STD Mutex. It doesn't

thx for the feedback! i somehow thought that for the lifetime of the guard interrupts were disabled so i tried to only be a short amount of time in the guard updating the value and leave. this is of course wrong, i misread the code there 😅

You do realize we have two CriticalSection implementations in esp-idf-hal?

• task::CriticalSection uses a FreeRtos mutex, not disable-all-interrupts. So it plays fine with the rest of the "user-space" code, but cannot be used from an ISR context, and, you CAN be interrupted by an ISR (or a higher prio thread for that matter) while you are holding the lock on that cs
• interrupt::CriticalSection DOES disable all interrupts (but can still be used in a recursive fashion). This one should be used with care, and its use cases are very rare on a RTOS env like ESP IDF

[Vollbrecht]

You do realize we have two CriticalSection implementations in esp-idf-hal?

yes i know, i was using the task criticalsection. thanks for sorting that out! i was confused by the usage of the Inner AtomicBool in the CriticalSection. Thinking that it indicates the state of the lock. But i saw that it was not reset to false on the guard drop / exit fn . But the taking and giving back is all done in the esp-idf MutexRecursive fn's and the AtomicBool is just to ensure a Mutex is given back only when we had one before.

[Vollbrecht]

because i only have an esp32c3(only one core) i cant really test if the locking mechanism for spi_pool.rs work as intended when two threads accessing the same Device in the spi_pool.rs would be nice if someone can confirm that it works.

[ivmarkov]

@Vollbrecht

I'm pulling out this comment out of the code review, as it is related to the general approach in terms of spi_pool (the rest looks OK modulo some renames).

My biggest concern with spi_pool is that it is a lot of code yet the purpose it achieves - I suspect - can also be achieved without it - and with only a minor modification to the rest of the code (if any):

• First, correct me if I'm wrong, but the reason spi_pool exists in the first place is so that you are able to multiplex many "logical" devices, on top of a single ESP IDF spi_device_handle_t instance (and thus all those will share a single configuration of course). The trick - as far as I understand it - is to let the user control the CS pin, rather than delegating that to the underlying ESP IDF driver
• But if the above is true, why bother with all of the above infrastructure? Why not just create a single EspSpiDevice without passing it a CS pin, and then - in your app code - multiplex on top of it as many "devices" as you want by just "manually" toggling on/off the "logical" CS pin (as ESP IDF is unaware of it) before / after the transaction?

[Vollbrecht]

@Vollbrecht

I'm pulling out this comment out of the code review, as it is related to the general approach in terms of spi_pool (the rest looks OK modulo some renames).

My biggest concern with spi_pool is that it is a lot of code yet the purpose it achieves - I suspect - can also be achieved without it - and with only a minor modification to the rest of the code (if any):

* First, correct me if I'm wrong, but the reason `spi_pool` exists in the first place is so that you are able to multiplex many "logical" devices, on top of a single ESP IDF `spi_device_handle_t` instance (and thus all those will share a single configuration of course). The trick - as far as I understand it - is to let the user control the CS pin, rather than delegating that to the underlying ESP IDF driver

yes that's the main goal, and to share the handles is currently the best working way i found. initially it worked different but it evolved to the current solution.

* But if the above is true, why bother with all of the above infrastructure? Why not just create a single `EspSpiDevice` **without passing it a CS pin**, and then - in your app code - multiplex on top of it as many "devices" as you want by just "manually" toggling on/off the "logical" CS pin (as ESP IDF is unaware of it) before / after the transaction?

yes but unfortunately that wont work so easily because of some subtle problems.

First problem: The current Design don't allow for an special use case someone might want ... I thought it was fine to have a unique pin for each devices, that a device without an defined cs pin made no sense on a shared bus. But this excludes the special case someone who only ever want one devices and dont want an CS pin and dont want to share. Because both EspSpiDevice and SpiPoolDevice both need an cs_pin. current behavior of EspSpiDevice: cs is given -> it always set SPI_TRANS_CS_KEEP_ACTIVE flag in the polling_transmit Fn -> this does't allow set the cs_pin config to -1 because if we use the SPI_TRANS_CS_KEEP_ACTIVE flag we need to make sure that we provided it with an cs_pin. can change to: i can change it to an Option<cs_pin_ref> here and make it work. if its None we can unset the SPI_TRANS_CS_KEEP_ACTIVE to let in not crash.Small drawback,because of the option we need one comparison extra per transaction but it should be fine... I will create an update asap.

Second problem: Because currently spi_pool works as a software cs impl -> For performance reasons:

• i wanted tighter control over when the chip_select gets pulled low/high
• because the sharing of the spi_device_handle_t is not thread save i am forced on an criticalsection locking mechanism -> i have now a guard mechanism -> i don't need the provided spi_device_acquire_bus() spi_device_release_bus() Fn's that are needed otherwise. so the SpiPoolDevice transaction implementation is slimmer in that regard. This is not possible if the user can only work with an EspSpiDevice and not an SpiPoolDevice -> one is forced to wrap around the EspSpiDevice transaction even not needing it in full and cannot write its own. I initially had the criticalsection inside the SpiMasterDriver though but @Dominaezzz did't wanted that so i only use it inside the pool_rs and now still using spi_device_acquire_bus() in EspSpiDevice.

Third problem: the user also cannot create its own spi_pool or only an SpiPoolDevice with its own transaction Fn that impl the above because the inner fields of the SpiBusMasterDriver are all private. -> Again only wrapping solution with the inner EspSpiDevice transaction Fn with unneeded calls

Fourth minor problem: Because the SpiConfigPool doesn't need an EspSpiDevice its more ergonomic. If one need's to write its own pool wrapper taking EspSpiDevices than in the users Borrow impl kinda explode's to Borrow<pool_wrapper<EspSpiDevice<SpiMasterDriver<...>>> kinda ugly.

yes the pool_rs is not ultra nice because of the code duplication but as you see there are some subtle differences. But i think i can reduce it a bit more. 50 lines of code are the config::config Fn inside spi_pool and because i make EspSpiDevice now take an optional chip_select i can pull that out and point both to the same create_conf Fn. It only created the pool file to separate it better in the developing phase, it would be nicer if its at the end of spi.rs with an mod super::* behind an feature flag or something to reduce more duplication. In the end its just two structs with max 180 lines of code that i think are worthwhile.

I personally need an Driver that supports more than the limited number of Devices, and i think others would also appreciate an turnkey solution for it but i understand that its a bit more than what the hal may want to include . In the end i wanted to finish this dam thing and it works now relative nice :laughing: