Board: Improve I²C device management to avoid null reference exceptions

[swharden]

Remove devices after creating them while scanning the I2C bus.

Thanks to @Ellerbach for providing feedback in the Discord 🚀

Resolves #2031

Microsoft Reviewers: Open in CodeFlow

[Ellerbach]

Looks good to me. thanks. I let @pgrawehr to have a look.

[raffaeler]

Looks good!

[krwq]

see @pgrawehr's comment

[swharden]

Thanks for your input everyone! I agree that the destructor should be cleaning things up, and it remains unclear to me why it is not. For reference here is the test code I am using to evaluate this issue. The code change in this PR causes this failing test to pass.

I'll take a closer look to figure out exactly what is going wrong, and whether a more targeted code change could similarly resolve this issue.

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Device.I2c;
using System.Linq;
using Iot.Device.Board;
using Iot.Device.Ft232H;
using Iot.Device.FtCommon;
using Xunit;

namespace System.Device.Gpio.Tests;

[Trait("feature", "ft232h")]
public class Ft232hTests
{
    [Fact]
    public void DevicesReleaseAfterScanning()
    {
        // I have one device connected (a FT232H)
        using Ft232HDevice f232h = new(FtCommon.GetDevices().First());
        I2cBus bus = f232h.CreateOrGetI2cBus(0);

        // First scan succeeds
        foreach (int address in bus.PerformBusScan())
        {
            Console.WriteLine($"Found address {address:X2} ({address})");
        }

        // Second scan throws a null reference exception
        foreach (int address in bus.PerformBusScan())
        {
            Console.WriteLine($"Found address {address:X2} ({address})");
        }
    }
}

[swharden]

I haven't figured everything out yet (it's taking me a long time to debug this) but I have a better understanding of the core issue now.

Here's a failing test:

[Fact]
public void PerformBusScan2()
{
    using Ft232HDevice f232h = new(FtCommon.FtCommon.GetDevices()[0]);
    I2cBus bus = f232h.CreateOrGetI2cBus(0);

    // open a device then dispose of it
    I2cDevice device1 = bus.CreateDevice(0x48);
    device1.ReadByte();
    device1.Dispose(); // this sets _i2cBus to null ⚠️

    // open the same device a second time
    I2cDevice device2 = bus.CreateDevice(0x48); // this succeeds
    device2.ReadByte(); // this throws a null reference exception ☠️
}

Disposing the I2C device sets the bus to null ... then it fails later when it tries to read from that null bus

[swharden]

@krwq, thanks for your useful comments above. @Ellerbach I'm aware you're familiar with FT232H (#2019) so I'll pose this to both of you. I think I arrived at a fix, but this has potential for unexpected behavior so I really value your input.

Currently the FT232H's I2C device destructor sets the bus to null which is what causes these problems later (see my last comment).

iot/src/devices/Ft232H/Ft232HI2c.cs

Lines 48 to 54 in 301f795

	protected override void Dispose(bool disposing)
	{
	_i2cBus?.RemoveDevice(_deviceAddress);
	_i2cBus = null!;
	_settings = null!;
	base.Dispose(disposing);
	}

This code fixes the issue, but it's not clear to me what the intent was of setting those variables to null inside the destructor in the first place. I'll update my PR to use this solution, but I want to check to make sure I'm not missing some intended functionality here. Thanks for your input!

protected override void Dispose(bool disposing)
{
    try
    {
        _i2cBus.RemoveDevice(_deviceAddress);
    }
    catch (ArgumentException)
    {
        // dont worry if the device has already been removed
    }

    base.Dispose(disposing);
}

[krwq]

Btw might be worth sharing some code with https://github.com/dotnet/iot/blob/main/src/devices/Ft4222/Ft4222I2cBus.cs so that we don't have same mistakes in multiple places. Possibly some common base class or something

[swharden]

sharing some code ... Possibly some common base class or something

I like this idea, but it seems out of scope for this PR and may cause merge issues with #2019 which appears to be actively worked on. Do you recommend I attempt these changes in this PR, or would it be a better idea to open an issue for it and work on it in a new PR after #2032 and #2019 are successfully merged?

3 failing and 8 successful checks

I see the CI release build has failed multiple times with errors that appear unrelated to the content of this PR. Can someone clarify whether this indicates there's a problem with my code modification, or if it's just an issue with the CI system? Thanks!

[pgrawehr]

@swharden #2019 is merged now, so you should be able to rebase and resolve the conflicts.

[swharden]

Testing on actual hardware

Testing on actual hardware

The tests that are in the System.Device.Gpio.Tests assembly are run on actual hardware (Raspberry Pis) with the Unix driver during CI. So if you add some test there, you would see whether it works on Linux.

Wow, that's fantastic! Thanks for pointing me in this direction. In addition to just being a RasPI, it looks like there is expected to be a BME280 I2C pressure sensor at address 0x77 (119). I'll add some tests here to demonstrate this issue and clarify when it is resolved.

iot/src/System.Device.Gpio.Tests/ProtocolTests.cs

Lines 40 to 48 in 58c8b14

	[Fact]
	[Trait("feature", "i2c")]
	public void I2C_Bme280CanRead()
	{
	using (Bme280 bme280 = CreateBme280())
	{
	TestBme280Reading(bme280);
	}
	}

Note to self: the raspberry pi tests say that they should pass without requiring raspberry pi hardware, so don't put i2c tests in there

Testing on mock hardware

I added this failing test to demonstrate the issue without requiring special gear 6cef0f2

iot/src/devices/Board/tests/BoardTests.cs

Lines 202 to 214 in 6cef0f2

	[Fact]
	public void CreateAndDisposeI2cDevice()
	{
	using Board board = CreateBoard();
	I2cBus bus = board.CreateOrGetI2cBus(0);
	I2cDevice device1 = bus.CreateDevice(0x55);
	device1.ReadByte();
	device1.Dispose();
	I2cDevice device2 = bus.CreateDevice(0x55);
	device2.ReadByte();
	}

Solution: don't store devices in the bus manager

all bus instances keep a list of attached devices, so this isn't necessary on the bus manager.

The tests pass for me after I removed all references to the dictionary from I2cBusManager.

I'll fix this issue by removing the dictionary in the bus manager never returning cached devices.

EDIT: I feel like the dictionary should stay because without it RemoveDevice() can't function, but it's required because the I2C bus manager inherits from I2cBus which has that function marked abstract.

Checking for null

Additionally, it might have reduced the confusion if Ft232HI2c.Write/Read would throw an ObjectDisposedException instead of an NRE if _i2cBus is null (and similarly in other places).

This is the line that throws the null reference exception, but in theory every function in this class that interacts with the bus could throw for the same reason.

iot/src/devices/Ft232H/Ft232HI2c.cs

Lines 29 to 32 in 58c8b14

	public override void Read(Span<byte> buffer)
	{
	_i2cBus.Read(_deviceAddress, buffer);
	}

Performing a null check on every read or write seems a bit costly. I agree ObjectDisposedException would be more informative than a NullReferenceException, but _i2cBus isn't nullable so I think the best way to proceed is to leave it how it is and assume it will never be null.

If you disagree let me know and I'll make it nullable and add null checks in all the functions of this class that reference the bus. I guess I2C is a pretty slow protocol anyway, so in context adding a bunch of null checks is probably a negligible hit to overall performance.

Renaming

Feel free to rename Ft232HI2c to Ft232HI2cDevice. This is clearly a naming error, and in such cases we're not very peculiar about breaking changes

Sounds good! Will do.

[swharden]

@pgrawehr I believe I made all the recommended changes and this is ready to merge now. Let me know what you think!

TLDR:

• I added this failing test 6cef0f2
• I got it to pass by ensuring I2cBusManager.CreateDevice() always instantiates and returns a new device 4c679a0
• Note that I2cBusManager still uses a dictionary to store created devices so I2cBusManager.RemoveDevice() can locate and dispose of them if users call that function instead of disposing of the device directly.

EDIT: The I2C bus manager's RemoveDevice() method could be marked obsolete to indicate that callers should dispose of devices they create themselves and not rely on the bus manager's dictionary-based memory system.

[Ellerbach]

I'm ok to rename the FT232HI2c yo FT232HI2cDevice. So don't keep the old one, I think it's confusing. And then please also adjust the sample and the read me

[pgrawehr]

Looks almost good, just a minor finding.

[swharden]

I'm ok to rename the FT232HI2c yo FT232HI2cDevice. So don't keep the old one

Done!

And then please also adjust the sample and the read me

Supporting the previous point, I think both are okay as they are because they use var

iot/src/devices/Ft232H/samples/Program.cs

Line 121 in 58c8b14

var i2cDevice = ftI2cBus.CreateDevice(Bmp280.DefaultI2cAddress);

iot/src/devices/Ft232H/README.md

Line 76 in 58c8b14

var i2cDevice = ftI2cBus.CreateDevice(Bmp280.SecondaryI2cAddress);

Use _devices[deviceAddress] = newDevice, since you want to always update. With this change, the oldest entry will persist, but you probably want to keep the newest.

Done!

---

I think this PR is ready to merge now, but let me know if you have any additional suggestions. Thanks for all your input along the way! 🚀

[Ellerbach]

@krwq is all ok for you now here?
@swharden thanks for the persistence and making all the changes!

[pgrawehr]

We need to update the compatibilitysuppressions still.

[krwq]

at this point we should consider making _devices either HashSet<I2cDevice> or Dictionary<int, HashSet<I2cDevice>> - perhaps even change design so that RemoveDevice(int) is replaced with RemoveDevice(I2cDevice). Alternatively we should throw on CreateDevice if it was created again (we should also check if it was already disposed though). My personal favor is we should change the design.

@pgrawehr thoughts?

[pgrawehr]

That would probably be better, but I'm not sure it's worth the effort. Would need to make the changes and then check whether they show any benefit.

[krwq]

let's do it separately if needed in that case - current state is still better than it was - that's why I approved in the first place

[krwq]

Let's make iterative changes from now on. My comment is still valid though

[pgrawehr]

@swharden Thanks again for figuring this out.