Adding GrovePi support #246

Ellerbach · 2019-02-18T07:08:58Z

Adding supportfor GrovePi from Dexter Industries
Adding support for all core GrovePi functions
Adding support for native GrovePi sensors like Ultrasound, Led Bar, DHT
Adding support for generic Analogic Input, Digital Input and Output, PWM Output
Adding support for high level sensors like buzzer, led, relay
Adding documentation
Adding sample with usage of multiple sensors, all present in the Grove starter kit

…ordingly, added SPI bus Id and chip select line

krwq · 2019-02-19T00:50:09Z

src/devices/GrovePi/GrovePi.cs

+        /// The default GrovePi I2C address is 0x04
+        /// Other addresses can be use, see GrovePi documentation
+        /// </summary>
+        public static byte GrovePiSefaultI2cAddress => 0x04;


I2cAddress should be good enough (also make it const)

krwq · 2019-02-19T00:51:14Z

src/devices/GrovePi/GrovePi.cs

+        public static byte GrovePiSefaultI2cAddress => 0x04;
+
+        /// <summary>
+        /// The maximum ADC, 12 bit so 2013 on GrovePi


confused by the comment, since value is 1023

typo, it's 1023

isn't that 10bit though?

yes, it is. copy/paste from GoPiGo with mistakes. correcting.

krwq · 2019-02-19T00:51:40Z

src/devices/GrovePi/GrovePi.cs

+        /// <summary>
+        /// Contains the GrovePi key information
+        /// </summary>
+        public GrovePiInfo GrovePiInfo { get; internal set; }


Info is sufficient since this is on GrovePi class

krwq · 2019-02-19T00:53:04Z

src/devices/GrovePi/GrovePi.cs

+
+        public void Dispose()
+        {
+            if(_autoDispose)


space before (

krwq · 2019-02-19T00:53:20Z

src/devices/GrovePi/GrovePi.cs

+        {
+            if(_autoDispose)
+            {
+                _i2cDevice.Dispose();


_i2cDevice?.Dispose(); _i2cDevice = null;

krwq · 2019-02-19T00:54:38Z

src/devices/GrovePi/GrovePi.cs

+        public Version GetFirmwareVerion()
+        {
+            WriteCommand(GrovePiCommands.Version, 0, 0, 0);
+            Thread.Sleep(10);


Why is sleep and 3 zeros needed needed?

The write command is taking 4 bytes. The version does not require the last 3 bytes. So they have to be set to 0 and sent.
For sleep, I can try to remove it and run more tests as I I implemented a second retry. In tests versions, asking too many I2C commands would raise exceptions.

krwq · 2019-02-19T00:55:06Z

src/devices/GrovePi/GrovePi.cs

+            WriteCommand(GrovePiCommands.Version, 0, 0, 0);
+            Thread.Sleep(10);
+            var inArray = ReadCommand(GrovePiCommands.Version, 0);
+            return new Version(inArray[1], inArray[2], inArray[2]);


inArray[1], inArray[2], inArray[3]?

the first inArray is always 0. And it is not part of the version. Only the last 3 bytes are

@Ellerbach What I meant is that you got [2] twice

hooooooooooooooo

krwq · 2019-02-19T00:56:11Z

src/devices/GrovePi/GrovePi.cs

+        /// <param name="param2">Second parameter</param>
+        public void WriteCommand(GrovePiCommands commands, GrovePort pin, byte param1, byte param2)
+        {
+            byte[] outArray = new byte[4] { (byte)commands, (byte)(pin), param1, param2 };


Span<byte> outArray = stackalloc byte[4]

krwq · 2019-02-19T00:56:32Z

src/devices/GrovePi/GrovePi.cs

+            byte[] outArray = new byte[4] { (byte)commands, (byte)(pin), param1, param2 };
+            try
+            {
+                _i2cDevice.Write(outArray.AsSpan());


AsSpan is not needed since byte[] is also a Span

krwq · 2019-02-19T00:56:56Z

src/devices/GrovePi/GrovePi.cs

+            {
+                _i2cDevice.Write(outArray.AsSpan());
+            }
+            catch (Exception ex) when (ex is IOException)


catch (IOException ex)

krwq · 2019-02-19T00:58:31Z

src/devices/GrovePi/GrovePi.cs

+                default:
+                    return null;
+            }
+            Span<byte> inArray = new Span<byte>(new byte[numberBytesToRead]);


This should be:

byte[] outArray = new byte[numberBytesToRead]; _i2cDevice.Read(outArray); return outArray;

krwq · 2019-02-19T00:58:56Z

src/devices/GrovePi/GrovePi.cs

+            {
+                return (PinLevel)_i2cDevice.ReadByte();
+            }
+            catch (Exception ex) when (ex is IOException)


exception directly in catch

krwq · 2019-02-19T00:59:20Z

src/devices/GrovePi/GrovePi.cs

+        public PinLevel DigitalRead(GrovePort pin)
+        {
+            WriteCommand(GrovePiCommands.DigitalRead, pin, 0, 0);
+            Thread.Sleep(10);


why is the sleep needed? if datasheet requires it - should it be part of WriteCommand instead?

Yes, it's needed to have the operation done correctly on the GrovePi side. Now, As implemented a retry method in the write command, I can try without and run tests like this.

perhaps add this delay before doing the retry

Yes, exactly what I was trying to test. I did a stress test wihout any wait. And after 15 minutes it failed. Will run a new one with the wait before the retry

I ran more tests and still failing after 30 minutes with older firmware (1.2.2). I'll implement multiple retries with the wait pattern. Let's see.

krwq · 2019-02-19T01:00:25Z

src/devices/GrovePi/Models/GrovePiCommands.cs

+    /// GroovePi commands to read, write, setup pins and access special sensors
+    /// Note that those commands are supports in most of the recent firmware with version higher than 1.2.1
+    /// </summary>
+    public enum GrovePiCommands


should this be private?

No as if you want to implement your own sensors, you may need it

krwq · 2019-02-19T01:00:33Z

src/devices/GrovePi/Models/GrovePiCommands.cs

@@ -0,0 +1,64 @@
+using System;


copyright header

same comment for other files where it is missing

pfff, totally forgot them… Will add

krwq · 2019-02-19T01:01:43Z

src/devices/GrovePi/Models/PinMode.cs

+    /// <summary>
+    /// Defines the pin level, either high, either low
+    /// </summary>
+    public enum PinLevel


why not use PinValue?

Right, I was hesitating. I can rename it to PinValue

I meant that we already have it in Gpio namespace

yes, I got it. I used the Gpio ones.

krwq · 2019-02-19T01:02:21Z

src/devices/GrovePi/Models/PinMode.cs

+    /// <summary>
+    /// Defines the pin mode, either input, either output
+    /// </summary>
+    public enum PinMode


why not use PinMode from Gpio?

Yes, same for PinLevel (PinValue). They are actually the same values in the enum. So all will work the same way

krwq · 2019-02-19T01:08:55Z

src/devices/GrovePi/Sensors/AnalogSensor.cs

+                throw new ArgumentException($"Error: grove Port not supported");
+            _grovePi = grovePi;
+            Port = port;
+            _grovePi.PinMode(Port, PinMode.Input);


I feel like GrovePi could probably implement IGpioController - might be worth taking a look

It actually can but the IGpioController is only for Digital Pins and not for analogic pins. And most people who are using Grove sensors which are not implemented will look at the Arduino code. And in the Arduino code, they'll find analogRead, digitalWrite, etc. And not the ones from the IGpioController. So in this case, I think it will bring more confusion than Clarity. If I would implement the IGpioController, I would anyway keep the DigitalWrite and DigitalRead functions.
GrovePi is here to bring analogic sensors to the RaspberyPi world and makes things easy. So I'll keep the philosophie of GrovePi.

@Ellerbach I'd suggest to create an issue that we should have support for analog pins in Gpio

I will do. also the IGpioInterface is not present in the nuget 0.1.0-prerelease.19078.2. Would be great to have a new nuget release that would include it.

Issue for analogic support: #255

krwq · 2019-02-23T07:37:16Z

src/devices/GrovePi/GrovePi.cs

+                    _i2cDevice.Write(outArray);
+                    return;
+                }
+                catch (IOException ex)


you can do catch (IOException ex) when (tries < MaxRetries)
and then while (true) - it will just throw on the last try - fine to leave as is too

I4ll leave it like this so far. But I keep the pattern in mind for other cases where it may Apply better.

Can we write something here describing why we're retrying? Ideally we don't swallow IOException, of course, but if there is a valid reason we should clearly state why.

krwq · 2019-02-23T07:49:06Z

src/devices/GrovePi/GrovePi.cs

+        {
+            WriteCommand(GrovePiCommands.DigitalRead, pin, 0, 0);
+            byte tries = 0;
+            IOException innerEx = new IOException();


Why not IOException innerEx = null or if compiler allows just declaration? I don't think this value is ever used

yes, it does accept null. The simple declaration was not enough. So I jsut created a new exception which is not needed as null is working.

krwq

LGTM, Thanks @Ellerbach - we can fix up anything remaining here as more people have a chance to play with GrovePi

JeremyKuhne · 2019-03-02T00:49:19Z

FYI: I just got the GrovePi+ starter kit. I'm going through the RGB LCD and revamping it a bit. I've found a datasheet for what I believe the IC is copying. It should be a clone of the NXP PCF2119x, which was introduced back in 1997. (AIP31068L is the clone part number, but I cannot find the datasheet for it.)

JeremyKuhne · 2019-03-02T00:59:35Z

src/devices/GrovePi/Sensors/DigitalOutput.cs

+        /// <param name="port">The grove Port, need to be in the list of SupportedPorts</param>
+        public DigitalOutput(GrovePi grovePi, GrovePort port)
+        {
+            if (!SupportedPorts.Contains(port))


The name of the argument should be included in the exception and you're not actually using the interpolated string.

throw new ArgumentException($"Error: grove port {port} not supported.", nameof(port));

if we are changing it then I'd get rid of the Error: prefix since that is implied by throwing an exception

… PR feedbacks

krwq · 2019-03-07T06:04:43Z

@JeremyKuhne - any chance you could take another look?

JeremyKuhne · 2019-03-07T17:29:27Z

I'll review shortly

JeremyKuhne

Appreciate your effort here! I have the GrovePi+ starter kit myself and I'm excited to use/build on this.

There are a number of nits and a few larger items called out. Notably ISensor is something we probably shouldn't do. Usage of the model would also be simpler if we can separate the analog pin and digital pin identifiers.

JeremyKuhne · 2019-03-07T17:44:53Z

src/devices/GrovePi/GrovePi.cs

+        /// The maximum ADC, 10 bit so 1023 on GrovePi
+        /// </summary>
+        public int MaxAdc => 1023;
+        /// <summary>


nit: no space

JeremyKuhne · 2019-03-07T17:49:40Z

src/devices/GrovePi/GrovePi.cs

+                }
+            }
+
+            throw new IOException($"{nameof(WriteCommand)} error writting command", innerEx);


I can see throwing another error here, but only if we're providing more context. We're not in this case, and there is a spelling problem too. A better string would be $"Failed to write command {commands}".

JeremyKuhne · 2019-03-07T17:50:27Z

src/devices/GrovePi/GrovePi.cs

+        {
+            Span<byte> outArray = stackalloc byte[4] { (byte)commands, (byte)(pin), param1, param2 };
+            byte tries = 0;
+            IOException innerEx = new IOException();


You shouldn't allocate an exception, just set it to null.

JeremyKuhne · 2019-03-07T17:51:36Z

src/devices/GrovePi/Models/GrovePiCommands.cs

+    /// </summary>
+    public enum GrovePiCommands
+    {
+        // Digital read a pin, equivalent of digitalRead on Arduino


These should be xml comments. /// <summary>

JeremyKuhne · 2019-03-07T17:52:55Z

src/devices/GrovePi/Models/GrovePiCommands.cs

+    /// GroovePi commands to read, write, setup pins and access special sensors
+    /// Note that those commands are supports in most of the recent firmware with version higher than 1.2.1
+    /// </summary>
+    public enum GrovePiCommands


This should be singular as it is not a flags enum. GrovePiCommand.

See the design guidelines.

JeremyKuhne · 2019-03-07T19:22:17Z

src/devices/GrovePi/Sensors/Relay.cs

+            set
+            {
+                _value = value;
+                _grovePi.DigitalWrite(Port, ((_value == PinValue.High) && !IsInverted) ? PinValue.High : PinValue.Low);


If we're exposing the raw value we should also set the raw value. This probably isn't even needed- you can infer the PinValue if you need to from IsOn/IsEnabled and IsInverted. I presume needing to know this would be very rare and having just one get/set would make it easier to use this type correctly.

I've added an IsOn property. Will keep this Value for consistency with the rest of the sensors.

JeremyKuhne · 2019-03-07T19:23:31Z

src/devices/GrovePi/Sensors/Relay.cs

+        /// <summary>
+        /// Get "On" when relay if on and "Off" when relay is off
+        /// </summary>
+        public override string ToString() => (_value == PinValue.High) ? "On" : "Off";


This isn't right as it doesn't take inverted into account.

JeremyKuhne · 2019-03-07T19:26:52Z

src/devices/GrovePi/Sensors/DhtSensor.cs

+    /// <summary>
+    /// Type of DHT sensors
+    /// </summary>
+    public enum DhtType


Nit: This should be in a separate file.

JeremyKuhne · 2019-03-07T19:27:14Z

src/devices/GrovePi/GrovePi.cs

+        public Version GetFirmwareVerion()
+        {
+            WriteCommand(GrovePiCommands.Version, 0, 0, 0);
+            var inArray = ReadCommand(GrovePiCommands.Version, 0);


Per our coding styles, don't use var here.

JeremyKuhne · 2019-03-07T19:29:37Z

src/devices/GrovePi/GrovePi.cs

+        /// <param name="autoDispose">True to dispose the I2C device when disposing GrovePi</param>
+        public GrovePi(I2cDevice i2cDevice, bool autoDispose = true)
+        {
+            _i2cDevice = i2cDevice ?? throw new ArgumentException("I2C device can't be null", nameof(i2cDevice));


Nit- the description isn't needed. The argument name and the exception tell you everything you need to know.

qwertoyo · 2019-04-11T12:54:38Z

🐱‍🐉 I'd like to see this merged, I'm using python with a weird dockerfile to use these sensors at the moment, but my ideal solution would be to use dotnet.
Is the PR opener (kudos) working on the comments? If not, may I help?

Ellerbach · 2019-04-11T14:37:38Z

🐱‍🐉 I'd like to see this merged, I'm using python with a weird dockerfile to use these sensors at the moment, but my ideal solution would be to use dotnet.
Is the PR opener (kudos) working on the comments? If not, may I help?

I need to find the time to work on this PR. In the mean time, you can use the PR by checking out the branch of this PR and work on it. Looking at the feedbacks, not everything will change. You may have later to adjust some of the elements but not everything. And your feedback will be very valuable.

Ellerbach · 2019-04-11T15:57:12Z

🐱‍🐉 I'd like to see this merged, I'm using python with a weird dockerfile to use these sensors at the moment, but my ideal solution would be to use dotnet.
Is the PR opener (kudos) working on the comments? If not, may I help?

@qwertoyo, other option is that you can take this PR, look at feedbacks from Jeremy and work on the PR ,as well. As I wrote in the previous comment, you can take the PR as a branch and work on it.

krwq · 2019-04-17T21:30:01Z

src/devices/GrovePi/samples/Program.cs

@@ -10,6 +10,7 @@
 using System.Device.I2c;
 using System.Device.I2c.Drivers;
 using System.Device.Gpio;
+using Iot.Device.GroovePiDevice.Models;


arg, will fix this one.

krwq · 2019-04-17T21:31:54Z

@Ellerbach except for one missed comment from previous and renaming Groove -> Grove looks like this should be ready. @JeremyKuhne, do you want to take another look?

joperezr · 2019-04-18T16:15:46Z

src/devices/GrovePi/GrovePiDevice.csproj

+  </PropertyGroup>
+
+  <ItemGroup>
+    <PackageReference Include="System.Device.Gpio" Version="0.1.0-prerelease.19078.2" />


This is not right. Can you please instead use version $(SystemDeviceGpioPackageVersion) ? That is what we are doing in the rest of the repo

will adjust, the PR is so old that it was not the case originally. Thanks for checking! Will adjust as well

joperezr · 2019-04-18T16:17:39Z

src/devices/GrovePi/Sensors/DhtSensor.cs

+// See the LICENSE file in the project root for more information.
+
+using Iot.Device.GrovePiDevice.Models;
+using Iot.Device.GrovePiDevice.Models;


Build is failing because you have this using namespace twice.

yes, correcting it

krwq · 2019-04-24T16:38:51Z

@joperezr want to review? I think this has been open long enough and is in reasonably good state - we can fix any remaining feedback separately

joperezr · 2019-04-24T18:42:50Z

src/devices/GrovePi/GrovePi.cs

+
+        /// <summary>
+        /// The default GrovePi I2C address is 0x04
+        /// Other addresses can be use, see GrovePi documentation


NIT: I wonder if it would be worth it to add here the alternative I2c addresses as well. It might be nice so that users don't have to look into datasheet in case they want to use an alternate address. Anyways, you don't have to fix it, just thinking out loud.

There is a way to change the address programmatically thru some scripts. To implement that, it would need to add SPI support and basically do what the script is doing. I Don't see much interest. Usually what people do is changing it before they start using it. If they want to have multiple GrovePi, they need anyway to change one by one with the script.

joperezr · 2019-04-24T18:47:05Z

src/devices/GrovePi/GrovePi.cs

+        /// <param name="pin">The pin to write the command</param>
+        /// <param name="param1">First parameter</param>
+        /// <param name="param2">Second parameter</param>
+        public void WriteCommand(GrovePiCommand command, GrovePort pin, byte param1, byte param2)


Should we consider making these lower-level APIs internal instead? seems like you have a wrapper API for all commands, so perhaps we should hide Write and Read from the public surface area just to make API easier to use.

As I have implemented some sensors only, I would live this part public so anyone who want to implement their own sensor can really use all low level as well. Now Read and Write will make it in 90% of the cases. Just living the door open for the 10%

joperezr · 2019-04-24T18:54:23Z

Left small comments but this looks good otherwise. Main thing for me is that I would try to simplify the surface area with GroovePi, maybe the best way would be to simply have a WriteCommand and ReadCommand, and don't have those DigitalReads and instead simply force people to manually call the WriteCommand and specify what is it that they want to read.

joperezr · 2019-04-25T17:41:38Z

I think this is good to go now. Thanks for all the great work here @Ellerbach!

Ellerbach added 12 commits February 3, 2019 11:19

Adding BrickPi3 device

a973dfa

fixing names for Linux builds

73fc8c3

fixing name of SPIExceptions.cs in csproj file

a1eb5ce

fiex typos in README, ,improved code based on GitHub reviews

e823c34

remove specific exceptions to use generic ones, adjusted the code acc…

ba8fdc4

…ordingly, added SPI bus Id and chip select line

Merge remote-tracking branch 'upstream/master'

6c78a36

Adding support for GoPiGo3

d960aca

Incorportated feedback following the PR

9e4a54d

Adjusting code based on last commit

7cedd7f

Fixed code based on latest feedbacks. Fixed typos in comments

270f45d

Merge remote-tracking branch 'upstream/master'

98075b9

Adding GrovePi support

cb5842d

krwq reviewed Feb 19, 2019

View reviewed changes

src/devices/GrovePi/GrovePi.cs Outdated

public void Dispose()

{

if(_autoDispose)

Copy link

Member

krwq Feb 19, 2019

Choose a reason for hiding this comment

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

space before (

krwq reviewed Feb 19, 2019

View reviewed changes

krwq reviewed Feb 23, 2019

View reviewed changes

Ellerbach added 2 commits February 27, 2019 12:13

fixed code following PR feedbacks

652783d

Fixed comment issue

60f802e

krwq approved these changes Mar 1, 2019

View reviewed changes

JeremyKuhne reviewed Mar 2, 2019

View reviewed changes

Adjusting exception for arguments as well as adding comments based on…

43d809a

… PR feedbacks

JeremyKuhne requested changes Mar 7, 2019

View reviewed changes

Updating code based on latest PR comments

271f11e

krwq reviewed Apr 17, 2019

View reviewed changes

fixed typo and feedback in last PR comments

f6e5a35

joperezr reviewed Apr 18, 2019

View reviewed changes

updating csproj files and fixing double namespace

880b04f

krwq approved these changes Apr 18, 2019

View reviewed changes

joperezr reviewed Apr 24, 2019

View reviewed changes

joperezr approved these changes Apr 25, 2019

View reviewed changes

joperezr merged commit 434a15c into dotnet:master Apr 25, 2019

github-actions bot locked and limited conversation to collaborators Dec 14, 2023

Adding GrovePi support #246

Adding GrovePi support #246

Conversation

Ellerbach commented Feb 18, 2019

krwq Feb 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq Feb 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq left a comment

Choose a reason for hiding this comment

JeremyKuhne commented Mar 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq commented Mar 7, 2019

JeremyKuhne commented Mar 7, 2019

JeremyKuhne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qwertoyo commented Apr 11, 2019 • edited Loading

Ellerbach commented Apr 11, 2019

Ellerbach commented Apr 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq commented Apr 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq Feb 19, 2019 •

edited

Loading

krwq Feb 19, 2019 •

edited

Loading

qwertoyo commented Apr 11, 2019 •

edited

Loading