RFC: Multi CS Spi Implementation

[Vollbrecht]

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 😞
    • 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

Todo

• update example's
• 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 🎉
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. 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 🥰

[Vollbrecht]

hmm i am kinda stuck, i don't think it is possible to use Borrow. 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 -> this feeds back to slave that needs now a generic Master.

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 😭 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 🙈

[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 🙈

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 🤡 I have this special hardware board that have 11 SpiDevices on one bus so thats why i am pushing this a bit 😹

[Vollbrecht]

At least its not a total disaster:

• We got a seperation from SpiDevice to SpiMasterDriver -> that allows multiple Devices 🎉
• 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.
💥

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.

[Dominaezzz]

I see you're using the raw spi_device_handle_t, were you not able to do this?

pub struct SpiConfigPool<'d, T> {
    shared_handles: HashMap<u32, EspSpiDevice<'d, T>>,
    master: T,
    _p: PhantomData<&'d ()>,
}

[Vollbrecht]

I see you're using the raw spi_device_handle_t, were you not able to do this?

pub struct SpiConfigPool<'d, T> {
    shared_handles: HashMap<u32, EspSpiDevice<'d, T>>,
    master: T,
    _p: PhantomData<&'d ()>,
}

yeah that was the initial plan. i switched it back here to the raw handle because of some design decision

• because of the cs_glitches i got when i frequently removed an device handle with spi_bus_remove_device() i currently opted out of the idea of switching out the spi_device_handle_t actively ( this is a deeper dive first):
• for regular usage one can now use SpiMasterDriver with EspSpiDevice witch uses hardware cs
• redefining the purpose of the SpiConfigPool: The SpiConfigPool gets you up to 3/6 independent "transmission configs" ( an spi_device_handle_t).
  • because each "transmission configs" is now shared it should not allow to have an cs pin set to anything -> an EspSpiDevice allows to set an cs_pin,
  • its probably less resource intense to just store just the handle
  • the transaction from EspSpiDevice and the SpiPoolDevice are different. The First must call SpiBusMasterDriver with hardware_cs true / the later with false. SpiPoolDevice needs to manage the cs pin here now actively -> want the high/low calls close to the actual transaction. but yes yours look much nicer without so much duplication 😺
• because of that i didn't think about actually using an EspSpiDevice here

[Dominaezzz]

Ah okay I see. So for the new idea, what's the different between this and the simple hardware CS option? Does this give you unlimited devices but subsets of them share a spi_device_handle_t?

[Vollbrecht]

Ah okay I see. So for the new idea, what's the different between this and the simple hardware CS option? Does this give you unlimited devices but subsets of them share a spi_device_handle_t?

yes that's the basic idea. but the nice thing is you can use it in two ways here. the first is unlimited devices. but you can also use it for the reverse. if you have one device that needed different configs you can now just change configs without switching the actual config / handle of the device. an config switch is normally also an handle switch witch lets the cs_pin in an glitchy state currently. this does prevent it. want a different config to your device? just change the config_id in your SpiPoolDevice to one of the created configs in the SpiConfPool. All at the cost of using software cs

[Dominaezzz]

This should be BorrowMut as you want an exclusive ref to the driver for pooling.

Suggested change

	T: Borrow<SpiMasterDriver<'d>> + 'd,
	T: BorrowMut<SpiMasterDriver<'d>> + 'd,

[Vollbrecht]

yes and no ? 😄 technically you could have a subset of devices that uses pooling and some that are plain EspSpiDevices. i don't require it exclusively. like 2 normal ones and 6 devices that share one handle. with this you could use some devices with hardware cs and the rest with software cs no ?

[Dominaezzz]

Sure but how does the SpiConfigPool know how many devices are available for pooling then? Especially since this number could change at runtime.

[Vollbrecht]

Sure but how does the SpiConfigPool know how many devices are available for pooling then? Especially since this number could change at runtime.

how many configs are available uses the same restriction as creating normal EspSpiDevices. you crate an SpiConfigPool with an set of configs. one should be able to change an config but not alter the numbers of configs. the user must ensure that in total all numbers of EspSpiDevices in parallel with the number of configs given to the SpiConfigPool creation does not exide 3 / 6 devices. you than get en EspError if you use more than that. but yeah would be nice to have that sorted out at compile time.
i saw some crazy shit with constant generics combined with FnOnce impl where one can construct functions that are only callable N times and that is checked at compile time !!!

[Dominaezzz]

With the change in my first comment, you'd be able to simplify this whole function to just this.

Suggested change

	let device_handle:&spi_device_handle_t = pool.shared_handles.get(&self.config_id).unwrap();
	// lock the bus through the mutex in SpiMasterDriver
	let master: &SpiMasterDriver = pool.master.borrow();
	// should we check for poison here?
	let driver_lock = master.handle.lock().unwrap();
	//self.pin_driver.set_low()?;
	//let handle = self.handle.clone().into_inner().unwrap();
	let mut bus = SpiBusMasterDriver {
	handle: *device_handle,
	trans_len: SOC_SPI_MAXIMUM_BUFFER_SIZE as usize,
	hardware_cs: false,
	_p: PhantomData,
	};
	self.pin_driver.set_low()?;
	let trans_result = f(&mut bus);
	let finish_result = bus.finish();
	// Flush whatever is pending.
	// Note that this is done even when an error is returned from the transaction.
	let flush_result = bus.flush();
	self.pin_driver.set_high()?;
	drop(driver_lock);
	println!("after lock drop");
	let result = trans_result?;
	println!("after trans result");
	flush_result?;
	println!("flush and finish");
	finish_result?;
	println!("end Transaction");
	Ok(result)
	}
	let device = &pool.shared_handles.get(&self.config_id).unwrap();
	device.transaction(f);

[Vollbrecht]

as stated above. this is much nicer but i need the difference from EspSpiDevice because this is software_cs. Software_cs need a dfferent SpiBusMasterDriver flag + needs tight setting over the pins. Have to think how to still reduce some of the duplication efficiently.

[Dominaezzz]

Understood

[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));
    }
    

[ivmarkov]

Does not belong to this PR.

[ivmarkov]

?

[ivmarkov]

This is very confusing. What does spi22 mean? What does spi2v2 mean?

Also:

• you need #[cfg(not(feature = "riscv-ulp-hal"))] or this code will break when compiling for the ULP HAL
• How can I have 3 different SPI2 peripherals? What happens if I use all three simultaneously? As in spi2 spi22 and spi_v2? Would that even work?

[Vollbrecht]

yeah sorry to confuse you here. it was just a hack to outsource the current dev of the driver. after finishing the new driver api i will merge it into the spi.rs than this will be all gone. ( and you don't need to review a wall of code 😄 ). When i created the draft i started with a spi_v2.rs. This is now gone i should have removed it 😸 I split it now into two files because of two different feauteres: spi2.rs witch is mostly an update to the current spi.rs will bring Mutli Cs / Multi Device. spi2_pool is the new thing where you can share rtos device handles with multiple devices to allow more than the limit of 3 or 6 devices, or alternativly just to have an mechanism to seemingly switch already created device_configs on an device( one device needs different bus speeds like an sdcard on creation).

[ivmarkov]

Not OK. Use these from the alloc crate. For mutex - use the mutex supplied by esp-idf-hal itself. For now, the policy is to avoid STD types where possible. it is another question whether this will pay off in any way, but for now we are doing it.

Also - and as a matter of fact, - we also try to avoid the alloc module in esp-idf-hal, whenever possible.

[Vollbrecht]

yep it was laziness here( i had not the time to look at the non std variants so far but it was on the radar for the finalization)

[ivmarkov]

I have a general question as to the approach here: why are we creating a completely new implementation of SPI, in its own module? Given that the SPI impl in master is not compatible with the one in the release branch, I think we should just modify the existing implementation in a way that it has the extra capabilities you suggest. As long as it is still possible to implement the e-hal 1 and e-hal 0.2 traits on top of it, that is. This would also save me the effort of looking at a wall of code, which surely is 80% the same as the one inthe current driver, and trying to figure out where you've done changes?

[Vollbrecht]

yeah i was planing to exactly does this. to make my life easier i was just developing in a parallel file to fetch other potential upstream changes to spi.rs. sorry to confuse you here

[ivmarkov]

Formatting.

[Vollbrecht]

yeah, my code is still ugly 🙈 it was not 100 % ready for a review

[ivmarkov]

See above for STD usage.

[Vollbrecht]

yep

[ivmarkov]

HashMap is an overkill, unless you plan to use tens to hundreds of devices here. And requires STD. Why not a BTreeMap or even a simple array of (u32, spi_device_handle_t) here?

[Vollbrecht]

yeah, original the concept was kinda different, you had a central pool where all the devices would be subscribed and one device could yank another devices handle and get its own. the driver worked, but calling rtos spi_bus_remove_device & spi_bus_add_device will make the hardware managed cs go wild(pulling it low for a short amount )on transition from an spi_cs_pin to an normal pin again.
now the shared handles can possibly be <= ESP_MAX_SPI_DEVICES and not " all the subscribed devices", so using a simpler data struct now makes sense.
Users should be able to add or remove shared_handles at runtime. A fixed size array was a bit unergonomic. but certainly its possible now. but the EspPoolDevice has to search now every element of the array . alternatively one can make something like a mapping inside the EspPoolDevice that maps its ID to an array Index and now just store an shared_handles[spi_device_handle_t]; or a BTreeMap as you suggested. in the end i will hope to get rid of all the std

[ivmarkov]

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.

If somebody wants just one device (with or without a CS pin) and does not want to share, he/she should use the SpiBus trait on top of the SpiMasterDriver somehow. The current approach where one still has to create a EspSpiDevice - even if just to use the SpiBus trait - is still OK as long as one can create an EspSpiDevice without a CS pin.

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.

That's exactly what I meant and that would be great.

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

Why?

• 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 do not understand this. All spi_device_acquire_bus does is that it ensures, that a device acquiring the bus cannot get a transaction from another device in between its own transactions. Whether you do this using the spi_device_acquire_bus or your won lock - I guess it is up to you and what would be more efficient in the end. But this is orthogonal to whether the concrete SPI device relies on the ESP IDF driver to control the CS pin in hardware or not. Isn't it?

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.

OK but this is even more complex, and... would that even work?

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

I do not understand this. In my simple mind, the user will create a pool around a specific EspSpiDevice instance. For this, the user does not need to know the inner workings of the EspSpiDevice instance. The user only needs to raise the CS pin at the beginning of the wrapped transaction method of EspSpiDevice, and move down the signal at the end of the wrapping?

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.

If a custom, user-supplied wrapper on top of EspSpiDevice is so difficult, we can always provide one out of the box.

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 😆

I think we can somehow meet in the middle. I think a good path forward would be to:
a) Allow no-CS-pin operating mode on EspSpiDevice
b) Make sure that the user can wrap the EspSpiDevice and raise/lower its own "logical" CS pin
c) Optionally - provide a wrapper on top of EspSpiDevice that takes a logical CS pin and wraps the EspSpiDevice transaction method

[Dominaezzz]

let driver = SpiMasterDriver::new(.....)?;
let bus = EspSpiBus::new(&mut bus, &config)?; // No CS pin specified.

bus.transfer(...)?;
bus.write(...)?;

User can then use https://github.com/Rahix/shared-bus to share the bus and get unlimited devices with software/manual CS?

[Vollbrecht]

let driver = SpiMasterDriver::new(.....)?;
let bus = EspSpiBus::new(&mut bus, &config)?; // No CS pin specified.

bus.transfer(...)?;
bus.write(...)?;

User can then use https://github.com/Rahix/shared-bus to share the bus and get unlimited devices with software/manual CS?

works only for one thread on spi though ... my current solution is much slimmer and works everywhere and i dont think it works with multiple configs

[Dominaezzz]

I'm not suggesting a replacement to spi_pool. I'm suggesting an additional struct to implement the SpiBus trait, which will be needed for SD cards (or bitbanging other protocols). It also covers the case where user doesn't want to provide CS pin.

It just seems very strange to have a EspSpiDevice that doesn't have a CS pin in it, the SpiDevice trait kinda demands it.

[Vollbrecht]

If a custom, user-supplied wrapper on top of EspSpiDevice is so difficult, we can always provide one out of the box.

did you have a chance to look inside the spi_pool.rs ? thats essentially what the spi_pool is. its just 2 structs and 150 lines that does the wrapping without being a full wrapper rather an optimized impl and also provided the locking itself. i can now implement it very easily myself now though. i just think its a nice helper and other's might appreciate it + it can be only this way if its inside the hal but if my life depend on it i can live without it 😺 and try to wrap around it myself .

[ivmarkov]

It just seems very strange to have a EspSpiDevice that doesn't have a CS pin in it, the SpiDevice trait kinda demands it.

spi_device_handle_t can explicitly operate without a CS pin, as per the documentation, where spics_io_num can be -1.

And your SpiBus impl will anyway work with an spi_device_handle_t hidden inside it?
I'm by the way starting to think that ALL transaction methods and in fact ALL methods that in fact do multiple ESP IDF transactions under the hood for a single e-hal "trasaction" should in fact ensure minimal latency with spi_bus_acquire / spi_bus_release. Including perhaps the SpiBus impl. (UPDATE: I have forgotten the code, that's what Lock does anyway).

[ivmarkov]

If a custom, user-supplied wrapper on top of EspSpiDevice is so difficult, we can always provide one out of the box.

did you have a chance to look inside the spi_pool.rs ? thats essentially what the spi_pool is. its just 2 structs and 150 lines that does the wrapping without being a full wrapper rather an optimized impl and also provided the locking itself. i can now implement it very easily myself now though. i just think its a nice helper and other's might appreciate it + it can be only this way if its inside the hal but if my life depend on it i can live without it 😺 and try to wrap around it myself .

What I don't like about spi_pool is the "pooling" aspect: why does it take an array of configs? What is the point of pooling "configs"? This only brings complexities afterwards:

• You have to refer - in SpiPoolDevice to the "config" by typeless integer id. Which brings the possibility for panics
• You are repeating a lot of code from EspDeviceHandle into SpiPoolDevice and SpiConfigPool. And by the way, your SpiPoolDevice does not implement the e-hal traits, any reason for that? Once you do, these 150 lines will grow larger.

What I would do, if we really want this "logical CS" utility:

• Make the CS pin optional in EspSpiDevice
• Make the transaction method take &, not &mut. After all, you are internally synchronizing everything by one big critical section. (UPDATE: Might not be so simple after all; open for suggestions though how to do this without introducing a "pool") By the way, as per above, you should probably do acquire bus / release bus instead? I doubt your critical section is much lighter weight than the native ESP IDF methods? (UPDATE: This is already done anyway, seems the critical section is just to synchronize access from the "logical" devices that use a single spi_device_handle_t unstance; but isn't using a single critical section also a bit too heavy? In that you'll this way synchronize also logical devices that use a different spi_device_handle_t instance each?)
• Create an EspSpiLogicalDevice that takes EspSpiDevice via a Borrow + a Peripheral to a pin. The passed EspSpiDevice borrow should be created without a CS pin, but we should not bother enforcing that. We can (with a type-state), but I don't see a reason to
• The transaction method of EspSpiLogicalDevice should call the transaction method of EspSpiDevice via the borrow ref. However, it should wrap the user-defined closure with its own closure, that raises/lowers the logical CS pin before/after the transaction

[ivmarkov]

host and device methods seem to be removed now? Or did I misread? From my POV they were there so that in case you want to use the Rust SPI driver for the most part, but then just need "this little thing" to be done with an unsafe call to the actual esp-idf-sys raw bindings - you could still do this. I see less of a need to do this for the CS pin, as the CS pin is passed from user code, so I would retire cs_gpio_number and restore the other two methods.

[Vollbrecht]

included again

[ivmarkov]

@Vollbrecht I was hoping I can release the master HAL this weekend. However, I would really like to have the new SPI driver as part of the release. The one major thing standing is the controversy around the "pool". I can either release without the pool, or with changes along the lines of what I suggested (EspSpiLogicalDevice). I can try to do the changes myself but a) I don't have a use case, so testing would be very laborious and b) I don't want that we are stepping on each other's toes. So.. what are your plans on finishing this?

Btw, I would like to do a couple of renames once the "pool" problem is addressed, but that's the easy part, and can be done post-merge.

[Vollbrecht]

yeah, got you. will try to get an update later today

[ivmarkov]

I've left a ton of requests for minor changes in the form of renames for clarity.
Please go through those, as at one place, I noticed a buglet, and also - amongst the renames - there are a few requests to fix the 'd lifetimes (getting rid of _p at one place; replacing &'d () with &'d mut () as it should really be.

Once the renames and small fixes are in place, I think we should really merge.

@Dominaezzz - I really hope, that the SD card support can be modelled by something like an SpiSdCardMasterDriver which takes a mutable Borrow over SpiMasterDriver.

[ivmarkov]

Using ? here is NOK. You have to keep the Result without checking, then toggle the CS pin, and only then inspect the result with ?.

[ivmarkov]

Two additional options regarding the master/slave dichotomy:

• Option 1: Drop the Master word from everywhere, and move the current SPI driver into a master submodule . I think that might actually be OK, in that folks would rarely mix master and slave SPI drivers usages in the same piece of code. The slave code (when it lands) will live in its own slave submodule
• Option 2: Drop the Master word from everywhere. When we wrap the slave SPI drivers, they will have the "Slave" word (or something less offensive!) - yet - their "master" counterparts will not be explicitly named "master". And all would be (from user POV) in the same spi module.

Now that I think of it, I'm heavily leaning towards Option 2. It is just that most of the SPI/I2C use cases are targeting the master mode. So perhaps we can just skip "Master" from everywhere and assume it is "master" in that it does not have the "Slave" keyword? Last but not least, this is what ESP IDF does, in terms of naming conventions for SPI.

So if we go with Option 2:

• SpiMasterDriver becomes SpiDriver or SpiHostDriver (I'm heavily leaning towards the first)
• SpiBusMasterDriver becomes SpiBusDriver
• SpiDeviceMasterDriver becomes SpiDeviceDriver
• SpiSharedDeviceMasterDriver becomes SpiSharedDeviceDriver
• The "software CS" thing becomes SpiSoftCsDeviceDriver

Oh - and by the way - for SpiSharedDeviceDriver to stand on its own, it would be great if it has a public lock method, with signature pub lock<R>(&self, FnMut(&mut SpiDeviceDriver) -> R) -> R callback. And then you can express the implementation of SpiSoftCsDeviceDriver in terms of calling the above lock method of SpiSharedDeviceDriver. Note also that this would allow you to restore the transaction method of SpiDeviceDriver to take a &mut self, which is somewhat safer, IMO. And - I'm not sure that we need the "borrow" complications in terms of how SpiSharedDeviceDriver holds its reference to SpiDeviceDriver. It should just own the SpiDeviceDriver, just like EspWifi owns the WifiDriver.

[ivmarkov]

If it not clear, thus SpiSharedDeviceDriver becomes just a mutex (which it actually is), except already specialized for SpiDeviceDriver data.

Btw, to be able to implement the lock signature form above, you need to stuff the SpiDeviceDriver instance in UnsafeCell<SpiDeviceDriver>. And own it please, no need there for the borrow magic. Since SpiSharedDeviceDriver does not need a Drop impl, we can always implement release on it, if user would like to get back its SpiDeviceDriver instance.

[Dominaezzz]

Considering the placement of these delays, couldn't the user just add this delay themselves in the transaction function? I don't see why it has do be done by esp-idf-hal.

[Vollbrecht]

the point for this is, that if you are using an hardware library that expects an embedded_hal SpiDevice you just can give it this Device. The hardware driver manages the SpiDevice -> you have no way of hooking in the transaction yourself

[Vollbrecht]

question become here if one writes an hardware driver for x, should he assume that the embedded_hal device that he receive is an devices with managed cs or not. If he specify that he would like to have an managed cs device and it needed an delay between cs and clk than you would need this.

[ivmarkov]

The e-hal traits do not assume or specify in any way how the SPI protocol is implemented. So as long as it is behaving as expected - managed CS or not - user should not care. So I'm fine with these delays actually.

[Dominaezzz]

The hardware driver manages the SpiDevice -> you have no way of hooking in the transaction yourself

The nice thing about the SpiDevice trait is that it can be implemented by anyone. Which means anyone can simply make a wrapper to add this delay themselves and it's only a couple lines of code.

question become here if one writes an hardware driver for x, should he assume that the embedded_hal device that he receive is an devices with managed cs or not

Yes. The Spi traits explicitly state that SpiDevice implementations take care of toggling the CS pin. If the driver wants to manage the CS pin then the SpiBus trait should be used instead.

[ivmarkov]

question become here if one writes an hardware driver for x, should he assume that the embedded_hal device that he receive is an devices with managed cs or not

Yes. The Spi traits explicitly state that SpiDevice implementations take care of toggling the CS pin. If the driver wants to manage the CS pin then the SpiBus trait should be used instead.

This is not how I interpret the spec, sorry. SpiBus is here for bitbanging and all sorts of tricks on top of the SPI protocol that - besides - also require an exclusive access to the bus.

With SpiSoftCsDeviceDriver we are really only manually manipulating the CS signal and nothing else. And then again - given that from the user code POV, the behavior of SpiSoftCsDeviceDriver is indistinguishable from SpiDeviceDriver, then why not supporting it to the full extent?

I think removing the CS pin delays, yet keeping the "pool" (what is now the SpiSoftCsDeviceDriver + SpiSharedDeviceDriver combo) is probably the worst. Either we provide "software" CS support, or we don't. But no in-between.

[Dominaezzz]

This is not how I interpret the spec, sorry. SpiBus is here for bitbanging and all sorts of tricks on top of the SPI protocol that - besides - also require an exclusive access to the bus.

bitbanging is one use case. SD cards is another. Display+Touch screen combo drivers is yet another.
https://github.com/rust-embedded/embedded-hal/blob/master/embedded-hal/src/spi.rs#L96
It's all about whether exclusive access is required and it just so happens that software CS required exclusive access. (Hence the locking we have here and in esp-idf).
I think we're on the same page here but using different words.

I think removing the CS pin delays, yet keeping the "pool" (what is now the SpiSoftCsDeviceDriver + SpiSharedDeviceDriver combo) is probably the worst. Either we provide "software" CS support, or we don't. But no in-between.

Oh okay, I kinda see what you're getting at.
First I want to say, does software CS actually require delays? Or is this just a convenience thing?
I see it as a convenience which is why I don't like it but from what you're saying it sounds like it required?

Though, now that the CS pin has been made optional, I think "software" CS feature should be removed then 😅. It should be easy for users to DIY it now.
A mutex (Critical section) is used here but some users might actually want RefCell. At the end of the day, increasing the device limit isn't about multithreading, it's about sharing, which Rust itself has multiple tools for.
So the code here is quite an opinionated feature.

[Vollbrecht]

If it not clear, thus SpiSharedDeviceDriver becomes just a mutex (which it actually is), except already specialized for SpiDeviceDriver data.

Btw, to be able to implement the lock signature form above, you need to stuff the SpiDeviceDriver instance in UnsafeCell<SpiDeviceDriver>. And own it please, no need there for the borrow magic. Since SpiSharedDeviceDriver does not need a Drop impl, we can always implement release on it, if user would like to get back its SpiDeviceDriver instance.

@ivmarkov I got that part with the pub lock Fn. My problem is i already take it currently as an owned value. so i see no difference there. to not get confused, i need the generic because SpiDeviceDriver allows to get the SpiDriver by Borrow -> so if i want to safe it as i do it right now or if i save it in an UnsafeCell i still need to provide the generic right?

[ivmarkov]

Thanks to both of you for driving this to completion!
(Clippy currently does complain because of some of the latest changes, but I'll fix these post-merge.)