Modern code review #1263
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,10 +47,10 @@ 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)} cannot be null"); | ||
| _inputMultiplexer = inputMultiplexer; | ||
| _measuringRange = measuringRange; | ||
| _dataRate = dataRate; | ||
|
|
@@ -61,7 +61,6 @@ public Ads1115(I2cDevice i2cDevice, InputMultiplexer inputMultiplexer = InputMul | |
| _comparatorPolarity = ComparatorPolarity.Low; | ||
| _comparatorLatching = ComparatorLatching.NonLatching; | ||
| _comparatorQueue = ComparatorQueue.Disable; | ||
|
|
||
| SetConfig(); | ||
| DisableAlertReadyPin(); | ||
|
|
@@ -79,18 +78,18 @@ public Ads1115(I2cDevice i2cDevice, InputMultiplexer inputMultiplexer = InputMul | |
| /// <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; | ||
|
There was a problem hiding this comment. 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: There was a problem hiding this comment. 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. |
||
| } | ||
|
|
||
| /// <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 @@ public void EnableComparator(ElectricPotential lowerValue, ElectricPotential upp | |
| 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 |
||
| } | ||
| } | ||
| } | ||
| } | ||
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.
You're only supposed to provide the name of the argument.
See https://docs.microsoft.com/en-us/dotnet/api/system.argumentnullexception.-ctor?view=netcore-3.1#System_ArgumentNullException__ctor_System_String_
Otherwise if you want to add a message use the second parameter, but the above message would pretty much already be generated (and probably localized too). See
https://docs.microsoft.com/en-us/dotnet/api/system.argumentnullexception.-ctor?view=netcore-3.1#System_ArgumentNullException__ctor_System_String_System_String_
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.
Got it. I noticed that there are two patterns that are pervasive in the codebase. I apparently picked the wrong one. I didn't Have this background. I'll fix them all on a second PR.
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.
I added another commit with many instances of
ArgumentExceptionfixed, well beyond the initial scope of the PR. Good call.