Button with external resistor bug as for #2012 #2013

raffaeler · 2023-01-15T13:42:03Z

This PR implements solution 1 and 3 proposed in #2012

I ask the reviewers whether they prefer:

option 1 (adding a single parameter to the existing constructor)
option 3 (adding two ctor overloads)

In case of option 3, please tell me whether you prefer to:

mark the old ctors as obsolete
replace the two boolean values with an enum

Microsoft Reviewers: Open in CodeFlow

…tors and continue detecting correctly the gpio transitions in the events

…to button_resistor

raffaeler · 2023-01-16T20:38:40Z

After a rebase, this PR got all the doc changes that has already been merged in main. Don't know why.
The only changes of this PR are in the folder src/devices/Button (including its README.md)

src/devices/Button/GpioButton.cs

…now true. This PR introduces a source breaking-change as discussed and approved in the Triage.

raffaeler · 2023-01-19T21:10:44Z

Since this PR introduce a source and binary breaking change (the old two ctors were replaced with different parameters), I tried to run the command build -pack /p:GenerateCompatibilitySuppressionFile=true as discussed in the Triage.

Anyway the command failed with the following output:

H:\Samples\_forks\iot\src\devices\Arduino\samples\Monitor\Arduino.Monitor.cs(90,59): error CS1503: Argument 2: cannot convert from 'System.Device.Gpio.GpioController' to 'bool' [H:\Samples\_forks\iot
\src\devices\Arduino\samples\Monitor\Arduino.Monitor.csproj::TargetFramework=net6.0]
H:\Samples\_forks\iot\src\devices\Arduino\samples\Monitor\Arduino.Monitor.cs(90,82): error CS1503: Argument 4: cannot convert from 'System.Device.Gpio.PinMode' to 'System.Device.Gpio.GpioController?'
 [H:\Samples\_forks\iot\src\devices\Arduino\samples\Monitor\Arduino.Monitor.csproj::TargetFramework=net6.0]
H:\Samples\_forks\iot\src\devices\Arduino\samples\Monitor\Arduino.Monitor.cs(90,97): error CS1503: Argument 5: cannot convert from 'System.TimeSpan' to 'bool' [H:\Samples\_forks\iot\src\devices\Ardui
no\samples\Monitor\Arduino.Monitor.csproj::TargetFramework=net6.0]
    0 Warning(s)
    3 Error(s)

Time Elapsed 00:03:42.99
Build failed with exit code 1. Check errors above.

Apart the compatibility suppression xml file not being generated, the rest of the PR is good and ready for review/approval.

…ing changes

raffaeler · 2023-01-19T21:40:02Z

@pgrawehr I had to change the ctor call in Arduino.Monitor.cs.
Since previously the PinMode was Input, I don't know if it was pull-up or pull-down.
Please take a look at the current values and tell me whether I should change them (I don't know how the Arduino board is configured).

raffaeler · 2023-01-20T12:00:24Z

@joperezr I see helix failing with this error. Is there anything I can do?

    System.Device.Gpio.Tests.SysFsDriverTests.AddCallbackFallingEdgeNotDetectedTest [FAIL]
      Assert.False() Failure
      Expected: False
      Actual:   True
      Stack Trace:
        /_/src/System.Device.Gpio.Tests/GpioControllerTestBase.cs(246,0): at System.Device.Gpio.Tests.GpioControllerTestBase.AddCallbackFallingEdgeNotDetectedTest()

joperezr · 2023-01-20T20:54:50Z

I see helix failing with this error. Is there anything I can do?

Those are the hardware tests. Given their nature and what they test, they tend to be flaky sometimes. Usually we just re-run to make sure it was just a fluke. In this case we can be pretty sure about it since your PR isn't really touching S.D.G and the other Linux Debug build (which also runs the tests) passed.

raffaeler · 2023-01-20T21:02:11Z

Those are the hardware tests. Given their nature and what they test, they tend to be flaky sometimes. Usually we just re-run to make sure it was just a fluke. In this case we can be pretty sure about it since your PR isn't really touching S.D.G and the other Linux Debug build (which also runs the tests) passed.

Thanks, but recently I got merge permissions, but not the azp ones :-)

pgrawehr · 2023-01-21T12:41:31Z

@pgrawehr I had to change the ctor call in Arduino.Monitor.cs. Since previously the PinMode was Input, I don't know if it was pull-up or pull-down. Please take a look at the current values and tell me whether I should change them (I don't know how the Arduino board is configured).

The button is pulled down with an external resistor in the example. See src\devices\Arduino\ArduinoSample_Monitor.png for the schematic of the example.

raffaeler · 2023-01-21T13:30:54Z

The button is pulled down with an external resistor in the example. See src\devices\Arduino\ArduinoSample_Monitor.png for the schematic of the example.

@pgrawehr
I just commited and pushed the change with isPullUp=false and HasExternalResistor=true on the Arduino.Monitor.cs.

raffaeler · 2023-01-21T13:51:55Z

@pgrawehr do you have any idea why the Linux Build Build_Release failed twice in a row?
If you think that this is a random failure, please azp this PR again.
Thank you

pgrawehr · 2023-01-21T21:46:48Z

@pgrawehr do you have any idea why the Linux Build Build_Release failed twice in a row?
If you think that this is a random failure, please azp this PR again.
Thank you

Yea, that particular test is flaky. I'll check a bunch of the latest builds and disable it if it's really always the same.

src/devices/Button/GpioButton.cs

Ellerbach

PullUp/PullDown are not supported everywhere. So you can't create them just like this. You have to test and then keep the chosen mode to compare later on.

Ellerbach · 2023-01-22T17:15:58Z

src/devices/Button/GpioButton.cs

-            _debounceTime = debounceTime;
+            HasExternalResistor = hasExternalResistor;
+
+            _eventPinMode = isPullUp ? PinMode.InputPullUp : PinMode.InputPullDown;


This is dangerous! Because some board do not support PullUp/PullDown. It's the reason why originally you have to pass the pull type.
If you want to do something like this, you need first to check if the pin mode is supported, if not, then choose one that is a default one.
See here for the good way to do it: https://github.com/dotnet/iot/blob/main/src/devices/Dhtxx/DhtBase.cs#L192

@Ellerbach I am aware that not every pin can be internally pulled up or down.
I didn't change the logic of the original author, I just added support for the external pullup/down resistor.

I was not aware of IsPinModeSupported in GpioController. Good to know, Now I can throw an exception if it's not.

I revised two cases:

if the button can be configured as input (certain gpio can only be output)

if the button can be configured with the configuration deriving from isPullUp and HasExternalResistor

I think this is fine. If the mode is not supported, it will throw, which is expected, because it's an user error.

Ellerbach · 2023-01-22T17:16:29Z

src/devices/Button/GpioButton.cs

@@ -75,7 +101,7 @@ private void PinStateChanged(object sender, PinValueChangedEventArgs pinValueCha
            switch (pinValueChangedEventArgs.ChangeType)
            {
                case PinEventTypes.Falling:
-                    if (_pinMode == PinMode.InputPullUp)
+                    if (_eventPinMode == PinMode.InputPullUp)


same here, you can't really do it like this is pull up is not supported

The _eventPinMode is always good because it is used to keep track if it is either pulled-up internally or externally.
The validation only goes when I open the pin. But if the resistor is external, the raising/falling logic is basically the same.

To make this more clear: _eventPinMode is not used in OpenPin.
I just 'reused" the enumeration to keep track whether the button transition is high to low or vice-versa.

…l-up/down.

Ellerbach

Thanks a lot for the tweaks, looks good to me now.

raffaeler · 2023-01-25T11:23:12Z

Thanks a lot for the tweaks, looks good to me now.

Could you please you merge it?

raffaeler added 3 commits January 15, 2023 11:44

Added IsExternalResistor in the constructor to support external resis…

b6c418c

…tors and continue detecting correctly the gpio transitions in the events

fixing comment

52c9951

Adding two constructors to the button which avoids using PinMode

fec70ff

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

raffaeler added 8 commits January 15, 2023 19:28

ctor refactoring to avoid ambiguity

727a0c3

Cleaned up the old constructors

b1ab829

Added IsExternalResistor in the constructor to support external resis…

534cbf7

…tors and continue detecting correctly the gpio transitions in the events

fixing comment

67503da

Adding two constructors to the button which avoids using PinMode

26b3ee0

ctor refactoring to avoid ambiguity

8fdc172

Cleaned up the old constructors

7079db9

Merge branch 'button_resistor' of https://github.com/raffaeler/iot in…

91a8028

…to button_resistor

Merge branch 'dotnet:main' into button_resistor

6627323

pgrawehr reviewed Jan 18, 2023

View reviewed changes

src/devices/Button/GpioButton.cs Outdated Show resolved Hide resolved

src/devices/Button/GpioButton.cs Outdated Show resolved Hide resolved

raffaeler added 4 commits January 18, 2023 22:25

fixes as requested by the review

abfe6f6

Merge branch 'dotnet:main' into button_resistor

151996b

Merge branch 'dotnet:main' into button_resistor

ff28a2c

Removed the old two constructors. The default value of isPullUp is …

86b4e31

…now true. This PR introduces a source breaking-change as discussed and approved in the Triage.

Fixing Arduino Monitor example and compatibiltiy files for ctor break…

a1f3f9b

…ing changes

Merge branch 'dotnet:main' into button_resistor

29cfa87

Fixed Arduino.Monitor.cs as for request

f3abcdb

Merge branch 'dotnet:main' into button_resistor

025875c

pgrawehr reviewed Jan 22, 2023

View reviewed changes

src/devices/Button/GpioButton.cs Outdated Show resolved Hide resolved

src/devices/Button/GpioButton.cs Outdated Show resolved Hide resolved

dispose + error handling

39bd7b7

pgrawehr reviewed Jan 22, 2023

View reviewed changes

src/devices/Button/GpioButton.cs Outdated Show resolved Hide resolved

shouldDispose check

82461d4

pgrawehr approved these changes Jan 22, 2023

View reviewed changes

Ellerbach reviewed Jan 22, 2023

View reviewed changes

raffaeler added 3 commits January 22, 2023 18:39

Checking whether the pin support being configured as input and/or pul…

4c6a4fc

…l-up/down.

refactoring

bb676ec

Merge branch 'dotnet:main' into button_resistor

596c873

Ellerbach approved these changes Jan 25, 2023

View reviewed changes

Ellerbach merged commit bf03b0e into dotnet:main Jan 25, 2023

raffaeler deleted the button_resistor branch January 25, 2023 20:44

raffaeler mentioned this pull request Jan 26, 2023

Button transition is not detected correctly when an external resistor is used #2012

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Button with external resistor bug as for #2012 #2013

Button with external resistor bug as for #2012 #2013

raffaeler commented Jan 15, 2023 •

edited by ghost

Loading

raffaeler commented Jan 16, 2023

raffaeler commented Jan 19, 2023

raffaeler commented Jan 19, 2023

raffaeler commented Jan 20, 2023

joperezr commented Jan 20, 2023

raffaeler commented Jan 20, 2023

pgrawehr commented Jan 21, 2023

raffaeler commented Jan 21, 2023

raffaeler commented Jan 21, 2023

pgrawehr commented Jan 21, 2023

Ellerbach left a comment

Ellerbach Jan 22, 2023

raffaeler Jan 22, 2023

raffaeler Jan 22, 2023

pgrawehr Jan 22, 2023

Ellerbach Jan 22, 2023

raffaeler Jan 22, 2023

raffaeler Jan 22, 2023

Ellerbach left a comment

raffaeler commented Jan 25, 2023

Button with external resistor bug as for #2012 #2013

Button with external resistor bug as for #2012 #2013

Conversation

raffaeler commented Jan 15, 2023 • edited by ghost Loading

Microsoft Reviewers: Open in CodeFlow

raffaeler commented Jan 16, 2023

raffaeler commented Jan 19, 2023

raffaeler commented Jan 19, 2023

raffaeler commented Jan 20, 2023

joperezr commented Jan 20, 2023

raffaeler commented Jan 20, 2023

pgrawehr commented Jan 21, 2023

raffaeler commented Jan 21, 2023

raffaeler commented Jan 21, 2023

pgrawehr commented Jan 21, 2023

Ellerbach left a comment

Choose a reason for hiding this comment

Ellerbach Jan 22, 2023

Choose a reason for hiding this comment

raffaeler Jan 22, 2023

Choose a reason for hiding this comment

raffaeler Jan 22, 2023

Choose a reason for hiding this comment

pgrawehr Jan 22, 2023

Choose a reason for hiding this comment

Ellerbach Jan 22, 2023

Choose a reason for hiding this comment

raffaeler Jan 22, 2023

Choose a reason for hiding this comment

raffaeler Jan 22, 2023

Choose a reason for hiding this comment

Ellerbach left a comment

Choose a reason for hiding this comment

raffaeler commented Jan 25, 2023

raffaeler commented Jan 15, 2023 •

edited by ghost

Loading