Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 25 commits into from
Feb 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
11b5d35
I2cBusExtensions.PerformBusScan() cleanup
swharden Feb 7, 2023
5d1f7e5
Revert "I2cBusExtensions.PerformBusScan() cleanup"
swharden Feb 9, 2023
a3d98bb
Ft232HI2c: dispose without nulling the I²C bus
swharden Feb 9, 2023
85856b7
Ft232HI2c: simplify code comment
swharden Feb 9, 2023
7b4f08e
Ft232H sample: I2C bus scan
swharden Feb 9, 2023
cb1c639
Ft232HI2c: null settings in destructor
swharden Feb 9, 2023
7157c5b
Revert "Ft232H sample: I2C bus scan"
swharden Feb 11, 2023
0432fe5
Merge remote-tracking branch 'upstream/main' into bugfix-i2c-scan
swharden Feb 11, 2023
a507d62
Ft232HI2cBus: add/remove device NoCheck
swharden Feb 12, 2023
0f39615
Ft232hI2cDevice: set bus to null in destructor
swharden Feb 12, 2023
72e2ea0
Ft232h sample: opening and closing i2c devices
swharden Feb 12, 2023
e35b187
ProtocolTests: reorder usings
swharden Feb 12, 2023
e38addd
ProtocolTests: repeat creation of the same I2C device
swharden Feb 12, 2023
7d435a1
ProtocolTests: explicitly remove I2C device
swharden Feb 12, 2023
0abc40e
ProtocolTests: scan for I2C devices
swharden Feb 12, 2023
3e4d18f
BoardTest: remove and recreate I2C device
swharden Feb 12, 2023
6cef0f2
BoardTest: dispose and recreate I2C device
swharden Feb 12, 2023
ef2c62f
Revert "Ft232h sample: opening and closing i2c devices"
swharden Feb 12, 2023
4c679a0
disable Board I2cBusManager device caching
swharden Feb 12, 2023
3dfa427
Ft232HI2c → Ft232HI2cDevice
swharden Feb 12, 2023
ad7bc86
mark Ft232HI2c obsolete
swharden Feb 12, 2023
a1a30d3
Ft232HI2c: inherit from I2cDevice
swharden Feb 12, 2023
c28125c
delete Ft232HI2c
swharden Feb 14, 2023
8cb4e2c
I2cBusManager: CreateDevice() remembers newest device
swharden Feb 14, 2023
a22bd02
Update CompatibilitySuppressions
pgrawehr Feb 15, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/Iot.Device.Bindings/CompatibilitySuppressions.xml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,13 @@
<Right>lib/net6.0/Iot.Device.Bindings.dll</Right>
<IsBaselineSuppression>true</IsBaselineSuppression>
</Suppression>
<Suppression>
<DiagnosticId>CP0001</DiagnosticId>
<Target>T:Iot.Device.Ft232H.Ft232HI2c</Target>
<Left>lib/net6.0/Iot.Device.Bindings.dll</Left>
<Right>lib/net6.0/Iot.Device.Bindings.dll</Right>
<IsBaselineSuppression>true</IsBaselineSuppression>
</Suppression>
<Suppression>
<DiagnosticId>CP0001</DiagnosticId>
<Target>T:IoT.Device.Pn532.ErrorCode</Target>
Expand Down Expand Up @@ -147,6 +154,13 @@
<Right>lib/netstandard2.0/Iot.Device.Bindings.dll</Right>
<IsBaselineSuppression>true</IsBaselineSuppression>
</Suppression>
<Suppression>
<DiagnosticId>CP0001</DiagnosticId>
<Target>T:Iot.Device.Ft232H.Ft232HI2c</Target>
<Left>lib/netcoreapp3.1/Iot.Device.Bindings.dll</Left>
<Right>lib/netstandard2.0/Iot.Device.Bindings.dll</Right>
<IsBaselineSuppression>true</IsBaselineSuppression>
</Suppression>
<Suppression>
<DiagnosticId>CP0001</DiagnosticId>
<Target>T:Iot.Device.Common.ValueArray`1</Target>
Expand All @@ -161,6 +175,13 @@
<Right>lib/netstandard2.0/Iot.Device.Bindings.dll</Right>
<IsBaselineSuppression>true</IsBaselineSuppression>
</Suppression>
<Suppression>
<DiagnosticId>CP0001</DiagnosticId>
<Target>T:Iot.Device.Ft232H.Ft232HI2c</Target>
<Left>lib/netstandard2.0/Iot.Device.Bindings.dll</Left>
<Right>lib/netstandard2.0/Iot.Device.Bindings.dll</Right>
<IsBaselineSuppression>true</IsBaselineSuppression>
</Suppression>
<Suppression>
<DiagnosticId>CP0001</DiagnosticId>
<Target>T:IoT.Device.Pn532.ErrorCode</Target>
Expand Down
67 changes: 65 additions & 2 deletions src/System.Device.Gpio.Tests/ProtocolTests.cs
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Threading;
using System.Collections.Generic;
using System.Device.I2c;
using System.Device.Pwm;
using System.Threading;
using Iot.Device.Adc;
using Iot.Device.Bmxx80;
using Xunit;
using Iot.Device.Board;
using UnitsNet;
using Xunit;
using static System.Device.Gpio.Tests.SetupHelpers;

namespace System.Device.Gpio.Tests;
Expand Down Expand Up @@ -100,6 +102,67 @@ public void I2C_I2cBus_MultipleDispose()
bme280.Dispose();
}

[Fact]
[Trait("feature", "i2c")]
public void I2C_I2cBus_MultipleCreate()
{
I2cBus i2cBus = CreateI2cBusForBme280();

I2cDevice device1 = i2cBus.CreateDevice(Bmp280.DefaultI2cAddress);
device1.ReadByte();
i2cBus.RemoveDevice(Bmp280.DefaultI2cAddress);

I2cDevice device2 = i2cBus.CreateDevice(Bmp280.DefaultI2cAddress);
device2.ReadByte();
}

[Fact]
[Trait("feature", "i2c")]
public void I2C_I2cBus_MultipleCreateAndDispose()
{
I2cBus i2cBus = CreateI2cBusForBme280();

I2cDevice device1 = i2cBus.CreateDevice(Bmp280.DefaultI2cAddress);
device1.ReadByte();
device1.Dispose();

I2cDevice device2 = i2cBus.CreateDevice(Bmp280.DefaultI2cAddress);
device2.ReadByte();
device2.Dispose();
}

[Fact]
[Trait("feature", "i2c")]
public void I2C_I2cBus_Scan()
{
I2cBus i2cBus = CreateI2cBusForBme280();
List<int> addresses = i2cBus.PerformBusScan();
Assert.NotNull(addresses);
}

[Fact]
[Trait("feature", "i2c")]
public void I2C_I2cBus_ScanMultipleTimes()
{
I2cBus i2cBus = CreateI2cBusForBme280();

List<int> addresses1 = i2cBus.PerformBusScan();
Assert.NotNull(addresses1);

List<int> addresses2 = i2cBus.PerformBusScan();
Assert.NotNull(addresses2);
}

[Fact]
[Trait("feature", "i2c")]
public void I2C_I2cBus_HasBmp280Present()
{
I2cBus i2cBus = CreateI2cBusForBme280();
List<int> addresses = i2cBus.PerformBusScan();
Assert.NotEmpty(addresses);
Assert.Contains(Bmp280.DefaultI2cAddress, addresses);
}

[Fact]
[Trait("feature", "pwm")]
[Trait("feature", "spi")]
Expand Down
7 changes: 1 addition & 6 deletions src/devices/Board/I2cBusManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,8 @@ public I2cBusManager(Board board, int bus, int[]? pins, I2cBus busInstance)
/// <remarks>No test is performed whether the given device exists and is usable</remarks>
public override I2cDevice CreateDevice(int deviceAddress)
{
if (_devices.TryGetValue(deviceAddress, out I2cDevice? device))
{
return device;
}

I2cDevice newDevice = _busInstance.CreateDevice(deviceAddress);
_devices.Add(deviceAddress, newDevice);
_devices[deviceAddress] = newDevice;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return newDevice;
}

Expand Down
28 changes: 28 additions & 0 deletions src/devices/Board/tests/BoardTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,34 @@ public void TwoI2cDevicesCanSharePins()
ctrl.OpenPin(0);
}

[Fact]
public void CreateAndRemoveI2cDevice()
{
using Board board = CreateBoard();
I2cBus bus = board.CreateOrGetI2cBus(0);

I2cDevice device1 = bus.CreateDevice(0x55);
device1.ReadByte();
bus.RemoveDevice(0x55);

I2cDevice device2 = bus.CreateDevice(0x55);
device2.ReadByte();
}

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

[Fact]
public void CreateSpiDeviceDefault()
{
Expand Down
2 changes: 1 addition & 1 deletion src/devices/Ft232H/Ft232H.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
<Compile Include="Ft2232HDevice.cs" />
<Compile Include="Ft232HDevice.cs" />
<Compile Include="Ft232HGpio.cs" />
<Compile Include="Ft232HI2c.cs" />
<Compile Include="Ft232HI2cBus.cs" />
<Compile Include="Ft232HI2cDevice.cs" />
<Compile Include="Ft232HSpi.cs" />
<Compile Include="Ft4232HDevice.cs" />
<Compile Include="Ftx232HDevice.cs" />
Expand Down
17 changes: 14 additions & 3 deletions src/devices/Ft232H/Ft232HI2cBus.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ namespace Iot.Device.Ft232H
/// </summary>
internal class Ft232HI2cBus : I2cBus
{
private HashSet<int> _usedAddresses = new HashSet<int>();
private HashSet<int>? _usedAddresses = null;

/// <summary>
/// Store the FTDI Device Information
Expand All @@ -35,23 +35,34 @@ public Ft232HI2cBus(Ftx232HDevice deviceInformation)
/// <inheritdoc/>
public override I2cDevice CreateDevice(int deviceAddress)
{
_usedAddresses ??= new HashSet<int>();
if (!_usedAddresses.Add(deviceAddress))
{
throw new ArgumentException($"Device with address 0x{deviceAddress,0X2} is already open.", nameof(deviceAddress));
}

return new Ft232HI2c(this, deviceAddress);
return CreateDeviceNoCheck(deviceAddress);
}

internal I2cDevice CreateDeviceNoCheck(int deviceAddress)
{
return new Ft232HI2cDevice(this, deviceAddress);
}

/// <inheritdoc/>
public override void RemoveDevice(int deviceAddress)
{
if (!_usedAddresses.Remove(deviceAddress))
if (!RemoveDeviceNoCheck(deviceAddress))
{
throw new ArgumentException($"Device with address 0x{deviceAddress,0X2} was not open.", nameof(deviceAddress));
}
}

internal bool RemoveDeviceNoCheck(int deviceAddress)
{
return _usedAddresses?.Remove(deviceAddress) ?? false;
}

internal void Read(int deviceAddress, Span<byte> buffer)
{
DeviceInformation.I2cStart();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,19 @@ namespace Iot.Device.Ft232H
/// <summary>
/// I2C Device for FT232H
/// </summary>
public class Ft232HI2c : I2cDevice
public class Ft232HI2cDevice : I2cDevice
{
private Ft232HI2cBus _i2cBus;
private int _deviceAddress;
private I2cConnectionSettings _settings;
private bool _shouldDisposeBus;

internal Ft232HI2c(Ft232HI2cBus i2cBus, int deviceAddress)
internal Ft232HI2cDevice(Ft232HI2cBus i2cBus, int deviceAddress, bool shouldDisposeBus = false)
{
_i2cBus = i2cBus;
_deviceAddress = deviceAddress;
_settings = new I2cConnectionSettings((int)i2cBus.DeviceInformation.LocId, deviceAddress);
_shouldDisposeBus = shouldDisposeBus;
}

/// <inheritdoc/>
Expand Down Expand Up @@ -47,9 +49,22 @@ public override void WriteRead(ReadOnlySpan<byte> writeBuffer, Span<byte> readBu
/// <inheritdoc/>
protected override void Dispose(bool disposing)
{
_i2cBus?.RemoveDevice(_deviceAddress);
_i2cBus = null!;
if (_i2cBus != null)
{
if (_shouldDisposeBus)
{
_i2cBus.Dispose();
}
else
{
_i2cBus.RemoveDeviceNoCheck(_deviceAddress);
}

_i2cBus = null!;
}

_settings = null!;

base.Dispose(disposing);
}

Expand Down