Modern code review #1263

richlander · 2020-11-07T23:19:59Z

I did a code review of the repo, moving to more modern patterns, including C# 9 syntax.

Lots of switch expressions added.
Updated null checks, inspired by 'Generate Equals' should use is object rather than != null roslyn#39253.
Applied language guidance for "master" usage, per https://www.oshwa.org/a-resolution-to-redefine-spi-signal-names. More still needs to be done on the repo for that.
Consistency and spelling mistakes.

Filed dotnet/roslyn#49230 as part of this cleanup. Not important, but I surely won't be the last person to be unsure of is checks on Span<T>.

This PR removes about 1k LOC from the repo. A previous PR (#1243) removed a similar number of LOC. Next step is to start applying records throughout the codebase (#1258).

Ellerbach

Nice clean up!

dotMorten · 2020-11-08T21:52:37Z

src/devices/Ads1115/Ads1115.cs

        {
-            _i2cDevice = i2cDevice ?? throw new ArgumentNullException(nameof(i2cDevice));
+            _i2cDevice = i2cDevice ?? throw new ArgumentNullException($"{nameof(i2cDevice)} cannot be null");


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_

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.

I added another commit with many instances of ArgumentException fixed, well beyond the initial scope of the PR. Good call.

dotMorten · 2020-11-08T21:57:34Z

src/devices/Ads1115/Ads1115.cs

            {
                throw new ArgumentOutOfRangeException(nameof(gpioInterruptPin), $"The given GPIO Controller has no pin number {gpioInterruptPin}");
            }

            _gpioInterruptPin = gpioInterruptPin;
-            _shouldDispose = shouldDispose;
+            _shouldDispose = shouldDispose || gpioController is null;


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

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.

ericsampson · 2020-11-08T23:28:46Z

src/devices/Bno055/samples/Program.cs

@@ -9,7 +9,7 @@

 Console.WriteLine("Hello BNO055!");
 using I2cDevice i2cDevice = I2cDevice.Create(new I2cConnectionSettings(1, Bno055Sensor.DefaultI2cAddress));
-using Bno055Sensor bno055Sensor = new Bno055Sensor(i2cDevice);
+using Bno055Sensor bno055Sensor = new (i2cDevice);


I'm personally not a big fan in this situation, using var seems a lot easier to parse to me. But I guess that's a style choice.
There's also not a lot of consistency in the pattern that's used in this, PR, some are using explicit types, some are using var, and this one is using target-typed new.

I used to be in the var camp. I switched as part of the last couple PRs I've done on this codebase. I've never spent so much time in a codebase in which I was totally unfamiliar. It's not a problem for construction, which this specific comment is about, but is a problem for readability for everything else. I've decided I'm not going to use var any more.

You are right that the PR isn't consistent on on this aspect. I'm trying some things. I still haven't figured out the right pattern. Fortunately, find and replace makes it somewhat straightforward to make changes.

I've decided I'm not going to use var any more.
😲🤯

jmarolf · 2020-11-09T05:33:45Z

src/devices/Ads1115/Ads1115.cs

@@ -285,7 +274,7 @@ private void DisableAlertReadyPin()
                (byte)Register.ADC_CONFIG_REG_HI_THRESH, 0x7F, 0xFF
            };
            _i2cDevice.Write(writeBuff);
-            if (_gpioController != null)
+            if (_gpioController is object)


at the risk of starting a war, I think is not null is more clear than is object. They generate the same code (is not null vs is object).

Adding @jaredpar to make sure dissenting opinions way in.

I sorta figured it isn't using not null due to language version requirements for the repo.
But in general re is object vs is not null, this feels like something that is going to keep coming up as people are opinionated on this; there's no technical advantage either way, it's purely preference driven.
So tl;dr, 🍿 (Hi Jared :)

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 is object is extremely clear to them and choose it in general. Other people working on their company LOB codebases might decide on is not null because it seems easier for Joe/Jane Developer to understand what's going on. Each team/codebase will choose what they feel is clear and understandable to them (and readers of the code), though what that means will vary in context.

no reason to new set the lang version explicitly imho:

<PropertyGroup> <LangVersion>8.0</LangVersion> </PropertyGroup>

IDisposable · 2020-11-09T17:52:30Z

src/devices/Ads1115/Ads1115.cs

            {
-                _gpioController.Dispose();
+                _gpioController?.Dispose();
+                _gpioController = null;


Moving the nulling of _gpioController here means that if we called the constructor like
new Ads1115(i2cDevice, gpioController, 1, false, ...) then we would still keep the reference to the pass-in gpioController unlike the old code. Is this an intended change?

You are right. It would be better the old way. I was conflating the call to dispose and the assignment to null as related operations. They are not. Thanks for catching this.

Filed an issue on that: #1272

IDisposable · 2020-11-09T17:55:49Z

src/devices/Ahtxx/AhtBase.cs


-        /// <inheritdoc cref="IDisposable" />
-        public void Dispose() => Dispose(true);


So happy to see trivial Dispose being handled trivially ;)

IDisposable · 2020-11-09T17:58:29Z

src/devices/Bmp180/Bmp180.cs

-        {
-            return Temperature.FromDegreesCelsius((CalculateTrueTemperature() + 8) / 160.0);
-        }
+        public Temperature ReadTemperature() => Temperature.FromDegreesCelsius((CalculateTrueTemperature() + 8) / 160.0);


Would love to see these magic numbers be appropriately named const values

src/devices/Bno055/Bno055Sensor.cs

IDisposable · 2020-11-09T18:04:30Z

src/devices/Bno055/Bno055Sensor.cs

@@ -548,17 +536,14 @@ private void SetConfigMode(bool mode)
        /// <inheritdoc/>
        public void Dispose()
        {
-            if (_autoDispoable)
+            if (_shouldDispose)
            {
                _i2cDevice?.Dispose();
                _i2cDevice = null!;


Like other places, just because we're not _shouldDispose does that mean we should still keep the ref live in _i2cDevice or should we be nulling it outside the brace?

src/devices/BrickPi3/Extensions/EnumExtension.cs

IDisposable · 2020-11-09T18:10:18Z

src/devices/BrickPi3/Movement/Vehicle.cs

@@ -171,7 +171,7 @@ private void RunMotorSyncTime(BrickPortMotor[] ports, int[] speeds, int timeout)
            }

            // create a timer for the needed time to run
-            if (_timer == null)
+            if (_timer is null)
            {
                _timer = new Timer(RunUntil, ports, TimeSpan.FromMilliseconds(timeout), Timeout.InfiniteTimeSpan);


Is _timer ever disposed/nulled? It is an IDisposable object and should be handled (which means this class should also be IDisposable

IDisposable · 2020-11-09T18:13:31Z

src/devices/Buzzer/samples/MelodyPlayer.cs

            {
                return result;
            }

            return 0;
        }
+
+        public void Dispose() => _buzzer?.Dispose();


Should we be nulling _buzzer here?

src/devices/Ccs811/Ccs811Sensor.cs

IDisposable · 2020-11-09T18:18:19Z

src/devices/CharacterLcd/Hd44780.cs

-                _disposed = true;
-            }
-        }
+        public virtual void Dispose() => _lcdInterface?.Dispose();


Should we be nulling the _lcdInterface here?

src/devices/GoPiGo3/GoPiGo3.cs

src/devices/GrovePi/Sensors/LedBar.cs

Co-authored-by: Marc Brooks <[email protected]>

richlander · 2020-11-10T15:15:33Z

Thanks for all your help @IDisposable! Much appreciated. Thanks to everyone else for the feedback.

* Code cleanup * Update projects * Project updates * Fix breaks * Update ExplorerHat project * Update Ft4222 project * Update GoPiGo3 * Update GrovePi project * Update a few more projects * More updates * Update null checks * Fix ArgumentException usage * Apply suggestions from code review Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/GoPiGo3/GoPiGo3.cs * Update src/devices/Ina219/Ina219.cs Co-authored-by: Marc Brooks <[email protected]> * Switch exception type * Switch exception type * Update src/devices/Mcp3428/Mcp342x.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Mcp3xxx/Mcp3xxx.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Mcp9808/Mcp9808.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Mlx90614/Mlx90614.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Ssd13xx/Ssd13xx.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Tcs3472x/Tcs3472x.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Vl53L0X/Vl53L0X.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Ws28xx/Ws28xx.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Hmc5883l/Hmc5883l.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Mpr121/Mpr121.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Mpu9250/Mpu6500.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Nrf24l01/Nrf24l01.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Pn5180/Pn5180.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Pn532/Pn532.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Pn532/Pn532.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/RadioTransmitter/Devices/Kt0803/Kt0803.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/RadioReceiver/Devices/Tea5767/Tea5767.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Rtc/Devices/Ds1307/Ds1307.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Rtc/Devices/Ds3231/Ds3231.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Rtc/Devices/Pcf8563/Pcf8563.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Seesaw/SeesawBase.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/ShiftRegister/ShiftRegister.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Shtc3/Shtc3.cs Co-authored-by: Marc Brooks <[email protected]> * Update src/devices/Si7021/Si7021.cs Co-authored-by: Marc Brooks <[email protected]> * Update GoPiGo3.cs * Update per feedback * Add type in place of var * Revert one switch expression to switch statement * Add pragma for []? Co-authored-by: Marc Brooks <[email protected]>

richlander added 3 commits November 7, 2020 14:11

Code cleanup

654dd9c

Update projects

e737d2a

Project updates

6391e20

ghost added the area-device-bindings Device Bindings for audio, sensor, motor, and display hardware that can used with System.Device.Gpio label Nov 7, 2020

richlander added 6 commits November 7, 2020 15:28

Fix breaks

d67de4e

Update ExplorerHat project

0f4b3be

Update Ft4222 project

91bb426

Update GoPiGo3

230e6f8

Update GrovePi project

4a78e44

Update a few more projects

4215804

Ellerbach approved these changes Nov 8, 2020

View reviewed changes

richlander added 2 commits November 8, 2020 11:24

More updates

1b2f27d

Update null checks

09751da

dotMorten reviewed Nov 8, 2020

View reviewed changes

ericsampson reviewed Nov 8, 2020

View reviewed changes

Fix ArgumentException usage

2b17f8e

jmarolf reviewed Nov 9, 2020

View reviewed changes