I2C APIs should take address on Read/Write

[shaggygi]

I started a binding for the At24Cxx Serial EEPROM and wanted to ping for thoughts before proceeding.

The problem I noticed while reading the datasheets is the I2C addresses vary based on the size of memory. For example, the AT24C01/AT24C02 uses the hardware pins for addressing A0-A2 and the page addresses are in the first byte read/written (as in most cases).

However, as you work your way up to the AT24C16, it begins using the address bits A0-A2 instead to address the pages. Figure 7. Device Address in datasheet link above explains. This is somewhat of a hindrance in the I2cDevice implementation as you must assign the I2C address to the I2cConnectionSettings and then instantiate the I2cDevice. The I2cDevice object is assigned the I2C address for the lifetime of the object.

I'm hesitantly okay with this as you could design the binding where certain devices would have to use multiple I2cDevice objects to access all available memory. Basically, the one device would look like multiple devices on the bus. For example, the AT24C16 would need 8 I2cDevice objects to address all memory using addresses 0x50 - 0x57 where each object could access 16 pages.

The problem I see with this approach, is there will be some users that want to interact with the device/memory as a whole. I've personally seen where an IoT device has a 4K size of configuration data that is loaded into EEPROM. Ideally, I would use a method call that provides starting address and bytes of data to write. In general use, the binding API would need some internal logic to determine the paging (no matter what AT24C device) and would offer a little more appeasing API surface for Reads/Writes. However, I'm not sure of a good solution being forced to instantiate multiple I2cDevice objects to accomplish this.

/cc @joperezr @JeremyKuhne @krwq I'm copying you as you might have some thoughts how to get around this with current implementation or ideas on how to improve the II2cController that is in the works.

Since most of our current bindings pass in an I2cDevice object to instantiate the binding, I'm not sure how we could recreate another I2cDevice object with the new address to use when attempting to access related pages.

Would it be beneficial to add the following methods where it would override the initial address and use one provided? I could see this confusing some users as they'll see this as an option and think they have to use instead of how the current method includes the address.

public abstract byte ReadByte(int deviceAddress);
public abstract void Read(int deviceAddress, Span<byte> buffer);
public abstract void WriteByte(int deviceAddress, byte data);
public abstract void Write(int deviceAddress, Span<byte> data);

One other tidbit, I personally don't want the bindings to provide me with "best" I2cDevice for platform (similar to GpioController) as I might want to use my own (e.g. GpioI2cDevice, etc.) in particular cases.

[shaggygi]

Came across this Python library where they use the address in all methods.

#!/usr/bin/python

import smbus

bus = smbus.SMBus(1)    # 0 = /dev/i2c-0 (port I2C0), 1 = /dev/i2c-1 (port I2C1)

DEVICE_ADDRESS = 0x15      #7 bit address (will be left shifted to add the read write bit)
DEVICE_REG_MODE1 = 0x00
DEVICE_REG_LEDOUT0 = 0x1d

#Write a single register
bus.write_byte_data(DEVICE_ADDRESS, DEVICE_REG_MODE1, 0x80)

#Write an array of registers
ledout_values = [0xff, 0xff, 0xff, 0xff, 0xff, 0xff]
bus.write_i2c_block_data(DEVICE_ADDRESS, DEVICE_REG_LEDOUT0, ledout_values)

This is a little redundant, but approach would fix this thread's issue. It also allows you to only have to create one object to communicate with multiple devices on the bus. Of course, this is a big difference than our current implementation (since it's 1-to-1) so it seems like there would be an alternative addition to not break things.

[krwq]

@shaggygi I think for now my recommendation would be to expose the address field in the I2C through property and for now set it before the write. As we add more devices let's comment on this issue every time we have to do it (this should be cumbersome enough that someone will find this issue or create a similar one). Depending on how many times we have to do it we will or not add this API, sounds fair?

As for the auto-selecting I2C device, please create seperate issue since it will get lost quickly

[shaggygi]

I'm not sure, but I think this could cause issues with the Windows I2cDevice. See code link below where it is initialized using the DeviceAddress. I guess it could execute the code again to verify and store _winI2cDevice again when the DeviceAddress is changed.

iot/src/System.Device.Gpio/System/Device/I2c/Drivers/Windows10I2cDevice.Windows.cs

Line 22 in 91c0471

public Windows10I2cDevice(I2cConnectionSettings settings)

As for the auto-selecting I2C device, please create seperate issue since it will get lost quickly

This is not an issue today (that I'm aware of), more of a comment if there was thinking to code for I2cController similar to the GpioController is today. I'll keep watch and possible add issue if it comes up.

Thx

[JeremyKuhne]

To make sure I'm clear- you're suggesting this?

namespace System.Device.I2c
{
    public abstract class I2cDevice : IDisposable
    {
        // Existing API  ...
       
        // New API
        public abstract byte ReadByte(int deviceAddress);
        public abstract void Read(int deviceAddress, Span<byte> buffer);
        public abstract void WriteByte(int deviceAddress, byte data);
        public abstract void Write(int deviceAddress, Span<byte> data);
    }
}

My initial reaction is that it would be a bad idea as it isn't broadly applicable and, as you point out, likely to confuse people. If I were presented with this problem I'd start exploring from this idea:

namespace System.Device
{
    // Abstracts treating the device like a storage object- with a goal of easily being able to
    // create helpful things like System.IO.Stream wrappers, etc. Device binding would implement,
    // handling address switching to hit different banks, etc.
    public interface IStorageDevice
    {
         // Total capacity in bytes
         long Capacity { get; }

         // "position" would be the byte position from the start of the device's address space (i.e. Stream.Position)
         byte ReadByte(int position);
         byte Read(Span<byte> buffer, int position);
         // etc.
    }
}

namespace Iot.Device.AT24Cxx
{
    public abstract class AT24Cxx : IStorageDevice
    {
         public ReadOnlySpan<IStorageDevice> Banks { get; }

         // One possible way to provide direct bank access
         private struct BankWrapper : IStorageDevice
         {
               private int _myAddress;
               private AT24Cxx _device;
         }
    }
}

Take all of this with a grain of "I just started looking at this". :)

[krwq]

@JeremyKuhne this would also align with IGpioController since it also has addresses and Read/Write 😄 We could unify a lot of things this way

[shaggygi]

To make sure I'm clear- you're suggesting this?

No, I wouldn't like this approach since we already have to use address when creating settings and device. It was mainly for ideas and show it is similar to other GPIO libraries.

[JeremyKuhne]

I only did a brief look at https://github.com/dotnet/iot/blob/master/src/System.Device.Gpio/System/Device/I2c/Drivers/UnixI2cDevice.Linux.cs.

There is value in having I2cDevice be analogous to SpiDevice. I've got concerns about I2cDevice actually conceptually being I2cBus, which is what it would be if you could change the address on the fly. We could change I2cDevice to I2cBus that just contains the file descriptor / id for the bus. Or perhaps keep I2cDevice and have it take an optional I2cBus (that it creates for you). Some notes:

1. There is a cost (trivial?) to opening the file descriptor on every "device" even though I believe we only need to do it for the bus
2. There are limited numbers of active process file descriptors (1024 on my Pi)
3. I'm not clear on threading implications- does it matter if ioctl calls are made to the same descriptor on different threads if they aren't referencing the same device address?
4. Spi doesn't work the same way- each chip select gets it's own file descriptor
5. I haven't had a chance to look at the Windows implementations yet

[shaggygi]

I've got concerns about I2cDevice actually conceptually being I2cBus, which is what it would be if you could change the address on the fly.

IMHO, I still think it should be I2cDriver. For a few and some mentioned before...

• I2cDevice is technically what the binding would be. Like an Mcp23xxx, so it doesn't make sense to pass it an I2cDevice object. An I2cController or I2cDriver (and SpiController/SpiDriver) naming seems better.
• I2cDriver can be compared to GpioDriver where an I2C Read/Write(int address...) is similar to GPIO Read/Write(int pinNumber...)
• Related to GpioController, GpioDriver and the recent work for IGpioController. I think we can all agree naming is difficult. I actually first thought the Mcp23xxx should have implemented an IGpioDriver. Mainly, because you created the driver and pass it to the controller similar to how we create a RaspberryPi driver and use it with the controller. However, this gets a little complicated and @JeremyKuhne @joperezr work having the binding implement the IGpioController makes its easier to use. With that said, it is an I2C "driver" that is driving the I/Os for this binding.
• As mentioned above, I think it would confuse users if we added the Read/Write overloads with address parameter in addition to how the I2cDevice is constructed currently. If we change I2cDevice I2cDriver to only use the descriptor/bus, then it makes more sense to have the Read/Writes with the address parameter. This would enable both scenarios to work:
  1. When using directly as an I2cController/I2cDriver (similar to GpioController/GpioDriver), you could instantiate and call whatever address you want. Meaning, you could communicate with multiple I2C devices using the same object. This would be helpful for utilities like DeviceApiTester CLI as you have to create a new I2cDevice object for every address to check in the i2c-detect command. This would allow the one object to check all addresses.
  2. When using as a binding with the I2cController/I2cDriver being passed in and used behind the scenes, you could have Read/Write without the address as the binding would have the address stored when instantiating. Or in the case of the At24Cxx where multiple addresses are needed, the correct address would be used (behind the scenes) based on the memory page needing to access.

I believe this would work for the Unix flavor today (with some of the lower-level code changed). It wouldn't throw an error until the Read/Write occurred as those checks are not used until the Initialize() method is called. I'm not quite sure about the Windows flavor as some of the checks are configured in the constructor.

Sorry to sound so passionate about this topic 😄 I'm honestly open to what makes sense. Just want this to be a good story in the end. Now I understand how you Microsofties feel trying to get something somewhat difficult right. Keep the dialog going 👍

[shaggygi]

@joperezr this one might need a milestone assignment as decision could change the API surface. Thoughts?

[krwq]

After having worked with a few I2C devices over the weekend I thing tying to specific address is better and doing I2CBus as @JeremyKuhne has suggested for that specific use makes sense.
Also other option is to create multiple I2c devices for each address and use them as such (in your case you will have just 8 devices which doesn't seem particularly bad considering it is somewhat special anyway)

I had worked with 1 device which used 2 different addresses so far and in this particular case it was used to communicate with 2 different sensors on the same device so I also treat this device as 2 - for one or two devices which behave differently I think it makes sense to create I2cBus - I2cDevice in that case would implement on top of the bus and they could easily co-exist.

IMO next steps:

• open issue proposing I2cBus (and perhaps SpiBus)
• close this issue as won't fix

[shaggygi]

for one or two devices which behave differently I think it makes sense to create I2cBus - I2cDevice in that case would implement on top of the bus and they could easily co-exist.

This comment somewhat clicked just now. As mentioned before, I don't particular care for the name I2cDevice or I2cBus and for passing to a binding that is basically the "device". However, this might work for I2cController (and is where we're headed with other pin types - GpioController, AdcController, PwmController, etc.).

Therefore, we could pass in the I2cController to the binding as such:

var i2cController = new I2cController();  // More details below.

using(var myBinding = new MyBinding(i2cController))
{
    // Do stuff.
}

Like new GpioController(), new I2cController() could read/write to devices just like it does with _openPins.

int deviceAddress = 0x20;
i2cController.Write(deviceAddress, data);

This wouldn't be redundant in most cases as this would be hidden behind the surface APIs where the binding would determine which address to call... usually a specific one, but 2 in your case and 8 for the AT24Cxx.

I still like the name "Driver" (instead of UnixI2cDevice, etc.) as that is consistent with other pin Driver types, but don't see us changing at this point. But to point out, you could pass it to the controller like the other drivers.

new GpioController(PinNumberingScheme.Logical, gpioDriver);
// similar where this has the I2cConnectionSettings which as the busId and deviceAddress.
new I2cController(i2cDriver);

Using new I2cController(i2cDriver) or new I2cController(busId, deviceAddress) would be used in most cases where there is only 1 address needed to control. However, what are your thoughts on needing multiples and could they mutate using the same except address?

Meaning, the following would add a new driver/device under the controller, but with a different address.

int deviceAddress1 = 0x20;
int deviceAddress2 = deviceAddress1 + 1;
var i2cController = new I2cController(busId, deviceAddress1);
i2cController.Add(deviceAddress2);
// Now write to each device.
i2cController.Write(deviceAddress1, data);
i2cController.Write(deviceAddress2, data);

Thoughts?

[krwq]

@shaggygi I feel like 1 and multiple are different level of abstractions, so my thinking was more like (I think this is what @JeremyKuhne had in mind):

int deviceAddress1 = 0x20;
int deviceAddress2 = deviceAddress1 + 1;
var i2cBus = new I2cBus(busId);
i2cBus.Write(deviceAddress1, data);
i2cBus.Write(deviceAddress2, data);

var i2c = new I2cDevice(busId, deviceAddress1);
i2c.Write(data);

note even variable naming suggest it should be bus and device - Controller doesn't mean anything in context of software - almost every class controls something