Modern code review #1263
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,8 +47,8 @@ public class Ads1115 : IDisposable | |
| /// <param name="measuringRange">Programmable Gain Amplifier</param> | ||
| /// <param name="dataRate">Data Rate</param> | ||
| /// <param name="deviceMode">Initial device mode</param> | ||
| public Ads1115(I2cDevice i2cDevice, InputMultiplexer inputMultiplexer = InputMultiplexer.AIN0, MeasuringRange measuringRange = MeasuringRange.FS4096, | ||
| DataRate dataRate = DataRate.SPS128, DeviceMode deviceMode = DeviceMode.Continuous) | ||
| { | ||
| _i2cDevice = i2cDevice ?? throw new ArgumentNullException(nameof(i2cDevice)); | ||
| _inputMultiplexer = inputMultiplexer; | ||
|
|
@@ -61,7 +61,6 @@ public class Ads1115 : IDisposable | |
| _comparatorPolarity = ComparatorPolarity.Low; | ||
| _comparatorLatching = ComparatorLatching.NonLatching; | ||
| _comparatorQueue = ComparatorQueue.Disable; | ||
|
|
||
| SetConfig(); | ||
| DisableAlertReadyPin(); | ||
|
|
@@ -79,18 +78,18 @@ public class Ads1115 : IDisposable | |
| /// <param name="dataRate">Data Rate</param> | ||
| /// <param name="deviceMode">Initial device mode</param> | ||
| public Ads1115(I2cDevice i2cDevice, | ||
| GpioController? gpioController, int gpioInterruptPin, bool shouldDispose = true, InputMultiplexer inputMultiplexer = InputMultiplexer.AIN0, MeasuringRange measuringRange = MeasuringRange.FS4096, | ||
| DataRate dataRate = DataRate.SPS128, DeviceMode deviceMode = DeviceMode.Continuous) | ||
| : this(i2cDevice, inputMultiplexer, measuringRange, dataRate, deviceMode) | ||
| { | ||
| _gpioController = gpioController ?? new GpioController(); | ||
| if (gpioInterruptPin < 0 || gpioInterruptPin >= _gpioController.PinCount) | ||
| { | ||
| throw new ArgumentOutOfRangeException(nameof(gpioInterruptPin), $"The given GPIO Controller has no pin number {gpioInterruptPin}"); | ||
| } | ||
|
|
||
| _gpioInterruptPin = gpioInterruptPin; | ||
| _shouldDispose = shouldDispose || gpioController is null; | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -164,11 +163,7 @@ public DeviceMode DeviceMode | |
| /// Comparator mode. | ||
| /// Only relevant if the comparator trigger event is set up and is changed by <see cref="EnableComparator(short, short, ComparatorMode, ComparatorQueue)"/>. | ||
| /// </summary> | ||
| public ComparatorMode ComparatorMode { get; private set; } | ||
|
|
||
| /// <summary> | ||
| /// Comparator polarity. Indicates whether the rising or the falling edge of the ALRT/RDY Pin is relevant. | ||
|
|
@@ -208,13 +203,7 @@ public ComparatorLatching ComparatorLatching | |
| /// Minimum number of samples exceeding the lower/upper threshold before the ALRT pin is asserted. | ||
| /// This can only be set with <see cref="EnableComparator(short, short, ComparatorMode, ComparatorQueue)"/>. | ||
| /// </summary> | ||
| public ComparatorQueue ComparatorQueue => _comparatorQueue; | ||
|
|
||
| /// <summary> | ||
| /// This event fires when a new value is available (in conversion ready mode) or the comparator threshold is exceeded. | ||
|
|
@@ -285,7 +274,7 @@ private void DisableAlertReadyPin() | |
| (byte)Register.ADC_CONFIG_REG_HI_THRESH, 0x7F, 0xFF | ||
| }; | ||
| _i2cDevice.Write(writeBuff); | ||
| if (_gpioController is object) | ||
|
There was a problem hiding this comment. at the risk of starting a war, I think Adding @jaredpar to make sure dissenting opinions way in. There was a problem hiding this comment. I sorta figured it isn't using There was a problem hiding this comment. I'm guessing that people are going to have to get used to seeing it both ways. People that are super in the weeds of C# like the language team might decide that There was a problem hiding this comment. no reason to new set the lang version explicitly imho: <PropertyGroup>
<LangVersion>8.0</LangVersion>
</PropertyGroup> |
||
| { | ||
| _gpioController.UnregisterCallbackForPinValueChangedEvent(_gpioInterruptPin, ConversionReadyCallback); | ||
| _gpioController.ClosePin(_gpioInterruptPin); | ||
|
|
@@ -319,7 +308,7 @@ private void WriteComparatorRegisters(short loThreshold, short hiThreshold) | |
| /// for interrupt handling.</exception> | ||
| public void EnableConversionReady() | ||
| { | ||
| if (_gpioController is null) | ||
| { | ||
| throw new InvalidOperationException("Must have provided a GPIO Controller for interrupt handling."); | ||
| } | ||
|
|
@@ -349,7 +338,7 @@ public void EnableConversionReady() | |
|
|
||
| private void ConversionReadyCallback(object sender, PinValueChangedEventArgs pinValueChangedEventArgs) | ||
| { | ||
| if (AlertReadyAsserted is object) | ||
| { | ||
| AlertReadyAsserted(); | ||
| } | ||
|
|
@@ -368,10 +357,7 @@ private void ConversionReadyCallback(object sender, PinValueChangedEventArgs pin | |
| /// <param name="queueLength">Minimum number of samples that must exceed the threshold to trigger the event</param> | ||
| /// <exception cref="InvalidOperationException">The GPIO Controller for the interrupt handler has not been set up</exception> | ||
| public void EnableComparator(ElectricPotential lowerValue, ElectricPotential upperValue, ComparatorMode mode, | ||
| ComparatorQueue queueLength) => EnableComparator(VoltageToRaw(lowerValue), VoltageToRaw(upperValue), mode, queueLength); | ||
|
|
||
| /// <summary> | ||
| /// Enable comparator callback mode. | ||
|
|
@@ -388,7 +374,7 @@ private void ConversionReadyCallback(object sender, PinValueChangedEventArgs pin | |
| public void EnableComparator(short lowerValue, short upperValue, ComparatorMode mode, | ||
| ComparatorQueue queueLength) | ||
| { | ||
| if (_gpioController is null) | ||
| { | ||
| throw new InvalidOperationException("GPIO Controller must have been provided in constructor for this operation to work"); | ||
| } | ||
|
|
@@ -444,7 +430,7 @@ private ushort ReadConfigRegister() | |
| /// This method must only be called in powerdown mode, otherwise it would timeout, since the busy bit never changes. | ||
| /// Due to that, we always write the configuration first in power down mode and then enable the continuous bit. | ||
| /// </summary> | ||
| /// <exception cref="TimeoutException">A timeout occurred waiting for the ADC to finish the conversion</exception> | ||
| private void WaitWhileBusy() | ||
| { | ||
| // In powerdown-mode, wait until the busy bit goes high | ||
|
|
@@ -502,10 +488,7 @@ private short ReadRawInternal() | |
| /// For performance reasons, it is advised to use this method if quick readings with different input channels are required, | ||
| /// instead of setting all the properties first and then calling <see cref="ReadRaw()"/>. | ||
| /// </remarks> | ||
| public short ReadRaw(InputMultiplexer inputMultiplexer) => ReadRaw(inputMultiplexer, MeasuringRange, DataRate); | ||
|
|
||
| /// <summary> | ||
| /// Reads the next raw value, first switching to the given input and ranges. | ||
|
|
@@ -590,30 +573,16 @@ public short VoltageToRaw(ElectricPotential voltage) | |
| /// <returns>An electric potential (voltage).</returns> | ||
| public ElectricPotential MaxVoltageFromMeasuringRange(MeasuringRange measuringRange) | ||
| { | ||
| double voltage = measuringRange switch | ||
| { | ||
| MeasuringRange.FS6144 => 6.144, | ||
| MeasuringRange.FS4096 => 4.096, | ||
| MeasuringRange.FS2048 => 2.048, | ||
| MeasuringRange.FS1024 => 1.024, | ||
| MeasuringRange.FS0512 => 0.512, | ||
| MeasuringRange.FS0256 => 0.256, | ||
| _ => throw new ArgumentOutOfRangeException(nameof(measuringRange), "Unknown measuring range used") | ||
| }; | ||
|
|
||
| return ElectricPotential.FromVolts(voltage); | ||
| } | ||
|
|
@@ -623,50 +592,37 @@ public ElectricPotential MaxVoltageFromMeasuringRange(MeasuringRange measuringRa | |
| /// </summary> | ||
| /// <param name="dataRate">One of the <see cref="DataRate"/> enumeration members.</param> | ||
| /// <returns>A frequency, in Hertz</returns> | ||
| public double FrequencyFromDataRate(DataRate dataRate) => dataRate switch | ||
| { | ||
| DataRate.SPS008 => 8.0, | ||
| DataRate.SPS016 => 16.0, | ||
| DataRate.SPS032 => 32.0, | ||
| DataRate.SPS064 => 64.0, | ||
| DataRate.SPS128 => 128.0, | ||
| DataRate.SPS250 => 250.0, | ||
| DataRate.SPS475 => 475.0, | ||
| DataRate.SPS860 => 860.0, | ||
| _ => throw new ArgumentOutOfRangeException(nameof(dataRate), "Unknown data rate used") | ||
| }; | ||
|
|
||
| /// <summary> | ||
| /// Cleanup. | ||
| /// Failing to dispose this class, especially when callbacks are active, may lead to undefined behavior. | ||
| /// </summary> | ||
| public void Dispose() | ||
| { | ||
| if (_i2cDevice is object) | ||
| { | ||
| DisableAlertReadyPin(); | ||
| _i2cDevice.Dispose(); | ||
| _i2cDevice = null!; | ||
| } | ||
|
|
||
| if (_shouldDispose) | ||
| { | ||
| _gpioController?.Dispose(); | ||
| _gpioController = null; | ||
|
There was a problem hiding this comment. Moving the nulling of There was a problem hiding this comment. You are right. It would be better the old way. I was conflating the call to dispose and the assignment to Filed an issue on that: #1272 |
||
| } | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be cleaner to have an overload that didn't take a controller, and remove the shouldDispose from there? It seems weird this parameter is only used if a value is provided. Similar example is the HttpClient that takes a handler had the dispose boolean but the handler isn't nullable - only if you use the overload that doesn't take a handler is one instantiated and shouldDispose set to true:
https://github.com/dotnet/runtime/blob/master/src/libraries/System.Net.Http/src/System/Net/Http/HttpMessageInvoker.cs#L24-L28
https://github.com/dotnet/runtime/blob/master/src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs#L130
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good feedback. That's something we could consider, but it gets complicated really quickly because of the interplay of GPIO and I2C or SPI. Some bindings have an overload for just GPIO and another for GPIO and I2C (or SPI) where GPIO is optional. It seems like optional parameters are a perfect match for this complexity. Also, this is really the only extra line that has to exist to allow it.