Allow multiple read/write calls per transaction via SPI/I2C

[RandomInsano]

Both SPI and I2C have some concept of a transaction. SPI you assert an enable pin before sending data, I2C has start and end messages.

I'm finding that as I write and review more crates, the ergonomics of requiring single fixed sized buffers for communication are becoming cumbersome or require a lot of code duplication for variably large receivers. An example of the latter is the required code duplication in the eeprom24x.

After some research, it looks like the buffer copy for SPI and I2C in Linux-land might be unavoidable because the I2C_RDWR ioctl support isn't required. That means it would need to be wrapped somehow in the linux-embedded-hal crate.

Looking at Arduino, they do have the concept of protocol transactions. I'm not sure if I agree with the API entirely, but the concept is good. I'm sure Rust opens up some interesting design improvements there.

Can someone poke holes in this idea for me? I'd like some sort of solution to allocating these extra buffers on memory constrained devices, especially when it leads to reading and writing different sized data.

Here's a list of code that I find where buffer copying was required but didn't need to be:

• eeprom24x: For reading / writing arbitrarily large pages
• pscontroller: For changing one byte in a constant buffer

[RandomInsano]

Just some quick notes here, it looks like following is what Linux does with I2C_RDWR and might be a great way to go if we're okay implementing a fallback if the feature isn’t supported at runtime:

ioctl(file, I2C_RDWR, struct i2c_rdwr_ioctl_data *msgset)
  Do combined read/write transaction without stop in between.
  Only valid if the adapter has I2C_FUNC_I2C.  The argument is
  a pointer to a

  struct i2c_rdwr_ioctl_data {
      struct i2c_msg *msgs;  /* ptr to array of simple messages */
      int nmsgs;             /* number of messages to exchange */
  }

  The msgs[] themselves contain further pointers into data buffers.
  The function will write or read data to or from that buffers depending
  on whether the I2C_M_RD flag is set in a particular message or not.
  The slave address and whether to use ten bit address mode has to be
  set in each message, overriding the values set with the above ioctl's.

(from https://www.kernel.org/doc/Documentation/i2c/dev-interface)

Probably borrowing the buffers as they go in would be a good plan. Again, we can allow better ergonomics than C but it shows the basic way this could work.

Here's an example

[austinglaser]

The buffer-copy issue is one that I've run into time and again with embedded C code. It would be really cool if Rust provided a way to elegantly deal with complex transactions without unnecessary allocation/copying.

I think it will be important to provide access to a lower-level API, for use-cases where the concept of a transaction breaks down.

[RandomInsano]

Well, the i2c calls in Linux just take pointers to memory. I have a quick test I’m dabbling with to see. Because we need to stick to the common denominators I’m not sure what the final API should look like, but I’m not sure going to be faster for small buffers and instead it’ll just be more ergonomic.

[austinglaser]

Totally onboard with improved ergonomics as well.

[ryankurte]

Also totally onboard with improvements! For anyone else reading, there is related discussion in #64.

I've been wondering whether a builder-style API would work, something like SPI.start().write(&[W]).read(&mut[W]).end() could support transactions while allowing the underlying implementation to group operations if required/available?

[RandomInsano]

I’m glad you guys are keen to help here! Should be better for everyone once we’re done.

I like this builder pattern plan. I’ve pushed some (pretty broken) attempts to get an underlying Linux rdwr thing going just to start mocking things up and haven’t done well (the ioctl is saying I haven’t packed things well, feel free to school me).

The trick at the moment for linux-embedded-hal is that an array needs to be defined on that platform to pass into the kernel and it might be tricky to have a builder trait. Mainly, I’m not sure how to create a buffer at compile time based on it.

I was playing around with the idea of allocating a fixed buffer of X elements. Maybe the builder could pack elements of the array?

If you check my library I linked, you’ll see the idea was to build an array of actions which rolls pretty close to the builder pattern but it makes it the crate consumer’s responsibility to own the stack allocation. It looks something like this:

        let items = [
            Message::write(&[0u8; 12]),
            Message::read(&[0u8; 13]),
            Message::write(&[0u8; 14]),
        ];

I’m not sure which I prefer to be honest, so feel free to bikeshed a bit and we’ll see where folks lean. I’m going back to hacking on this to see if I can get the kernel to play nice or the ioctl! macro from the nix crate to properly import.

[RandomInsano]

A little update: After banging on it a few hours I’m stuck on getting the ioctl to play ball. I’ll reach out to the resident master of these things @posborne.

[RandomInsano]

I'm back after yet more head bashing. I got stuck on a weird drop problem, but my weird little POC is working and there are no memory copies in userland.

Currently at its lowest level I think the API could look like this:

        const ADDRESS = 0x53;
        const MESSAGE: &'static [u8] = &[0x01, 0x32, 0x04];
        let data = [0u8; 1];

        let bus = Bus::new(some_param_struct, ADDRESS);

        let mut items = [
            bus::write(&MESSAGE),
            bus::read(&data),
            bus::custom_thing(_, _, _),
        ];

        bus.transfer(&items);

Is that a pattern that works for folks? I want to discuss implementation of this with @posborne for inclusion into his i2cdev then port the idea up the HAL once that's done. Does anyone have a better idea for the structure here? I'm not sure what the lowest common denominators are for this interface.

[posborne]

Hey @RandomInsano; this would definitely be a fine feature to add to the i2cdev crate. If you have something I think we could probably move forward with a PR or proposal for an API on rust-embedded/rust-i2cdev#42.

[Rahix]

@RandomInsano: Your comment in #35 made me aware of this issue. I am strongly against breaking downstream drivers. One of the main reasons for the design of shared-bus was to keep API compatibility. What about changing your API to something like this?

let data = [0u8; 1];

let bus = Bus::new(some_param_struct);

bus.transfer(0x53, &mut [
    bus::write(&[0x01, 0x32, 0x04]),
    bus::read(&mut data),
    bus::custom_thing(_, _, _),
]);

This would work perfectly fine with the existing ecosystem and would not reduce flexibility. For example, some complex driver might need to talk to multiple devices. Such a driver would then need multiple i2c peripherals/proxies for no real benefit. I can see why you'd want to bind a bus to an address but I don't think the increase in usability would be big enough to warrant a breaking API change.

---

On a completely different note, I really like @ryankurte's API proposal so I gave it a shot. What I came up with looks like this:

let mut buf = [0x00; 8];
bus.transaction(0x44)                  // Transaction for address 0x44
    .write(&[0xde, 0xad, 0xbe, 0xef])  // Write step
    .read(&mut buf)                    // Read step
    .commit().unwrap();                // Actually perform the transaction

The full implementation can be found here.

I think from a user's perspective this API looks a lot nicer. Under the hood it is a lot more complex because of the use of a recursive type to circumvent the need for allocation but as this should not have too big of an effect on runtime/stacksize I would argue that it is ok. The complexity is also hidden from peripheral implementations which would look like this:

impl Transactor for Bus {
    fn commit<'a, S: Transact<'a>>(&'a mut self, steps: &'a mut S) -> Result<(), ()> {
        println!("  I2C: Start");
        // The closure is called for each step of the transaction
        steps.transact(|s| match s {
            Step::Write(d) => println!("  I2C: Write {:?}", d),
            Step::Read(buf) => {
                println!("  I2C: Read {:?} bytes", buf.len());
                for b in buf.iter_mut() {
                    *b = 0xff;
                }
            }
        });
        println!("  I2C: End");
        Ok(())
    }
}

One big downside I can see right away is that transactions whose steps are not known at compile time are not easily possible. I don't know how often they are needed though.

[ryankurte]

@Rahix I like your suggested alteration to @RandomInsano's suggestion, thought the address only makes sense for the i2c implementation anyway I think.

I think at the moment I am in favour of the array-of-parts approach for it's simplicity, as an additional trait. I wonder whether a default impl over other types would be viable, I think that raises the (SPI at least) question of from where and when the CS pin is controlled.

It should then be possible to experiment and implement a builder api on top as a separate crate if those ergonomics are preferred?

[RandomInsano]

Ooo! I like the idea of something that actually works, and I'm really comfortable with the look of option number 2 it as it maps to other builder interfaces I've used.

I'll try to counter argue how we might benefit more from having an abstraction that owns the bus address / select lines. I've been mulling it over, and we don't need to break existing drivers and the porting effort would (hopefully) be minimal and beneficial.

In the end, this is a good thought exercise and as long as the community benefits it's all fun for me! I don't mind throwing my work out if it's not the right path.

Let's say we have a few abstractions:

I2CProxy(u16)
SPIProxy(OutputPin)
OneWireProxy(u64)

LegacySPIProxy(OutputPin)
LegacyI2CProxy(u16)

We should be able to have the Legacy flavors provide the familiar interface but no-op the addresses that are sent by the address parameter of read() and write(). This would allow backward compatibility with a new system while others opt-in to the new interface. The back end of this new system would ideally start by consuming the existing APIs provided by the device crates, but overtime implementations that work closer to the metal would surface. For i2c, it also allows for devices with multiple addresses to be selected by the consumer.

In the end, i2c, SPI, serial and 1-wire are all transferring either by full or half duplex. I like the idea of having a consistent experience across bus types even if access patterns under the hood are different.

I'm not aware of any devices that require two distinct addresses / busses. If anyone has one, that'll be a great curve ball to think through.

//! Device / Driver consumer examples for the legacy case

// SPI Example
let bus_master = BusManager::new_spi(spi);
let select_expander = some_device::gimme_pin();
let bus_node = bus_master::legacy_spi(select_expander);
let stretch_armstrong = Mcp3008::new(bus_node, BusManager::dummy_pin());

// I2C Example
let bus_master = BusManager::new_i2c(i2c);
let bus_node = bus_master::legacy_i2c(Mag3110::ADDRESS);
let magna_doodle = Mag3110::new(bus_node);

// Multiple Light Sensors
let bus_master = BusManager::new_i2c(i2c);
let base_address = Tsl2561::BASE_ADDRESS;
let light_bright1 = Tsl2561::new(bus_master::legacy_i2c(base_address), 0x00);
let light_bright2 = Tsl2561::new(bus_master::legacy_i2c(base_address+0x10), 0x00);
let light_bright3 = Tsl2561::new(bus_master::legacy_i2c(base_address+0x20), 0x00);