Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
raffaeler opened this issue Jan 15, 2023 · 1 comment
Closed

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

raffaeler opened this issue Jan 15, 2023 · 1 comment
Assignees
@raffaeler
Labels
bug Something isn't working Priority:2 Work that is important, but not critical for the release

Comments

@raffaeler
Copy link
Contributor

If an external pull up resistor is wired to the pin button and the user creates the button with PinMode.Input (without internal resistors), the transition in PinStateChanged is not detected correctly.
See: https://github.com/dotnet/iot/blob/main/src/devices/Button/GpioButton.cs#L78

Steps to reproduce

  1. Use an external pull-up resistor
  2. Configure the button with PinMode.Input

The gpio button always need either a pull-up or pull-down. It then can be internal (provided by the board) and configured by PinMode or external.

In order to fix this problem, we may use one of the following strategies:

  1. add a boolean IsExternalResistor (defaulting to false) parameter specifying whether the the PinMode is either InputPullUp or InputPullDown. This is a binary breaking change, but does not break the compilation.
  2. completely remove the the PinMode parameter and add two boolean values:
  • IsPullUp (false means pull-down)
  • IsExternalResistor (false means configured through PinMode)
    This requires the user to make changes in her code.
  1. Add a new constructor overload (eventuallty deprecating the previous ones). This does not break any code (not even binary) but still exhibit the bug when using the old constructors.

With regards to option 2, instead of the two booleans we may introduce an enum with the following fields:

  • PullUpInternal
  • PullUpExternal
  • PullDownInternal
  • PullDownExternal
@raffaeler raffaeler added the bug Something isn't working label Jan 15, 2023
@raffaeler raffaeler self-assigned this Jan 15, 2023
@raffaeler raffaeler mentioned this issue Jan 15, 2023
Merged
@krwq krwq added the Priority:2 Work that is important, but not critical for the release label Jan 19, 2023
Ellerbach pushed a commit that referenced this issue Jan 25, 2023
@raffaeler
Button with external resistor bug as for #2012 (#2013)
bf03b0e 
* Added IsExternalResistor in the constructor to support external resistors and continue detecting correctly the gpio transitions in the events

* fixing comment

* Adding two constructors to the button which avoids using PinMode

* ctor refactoring to avoid ambiguity

* Cleaned up the old constructors

* Added IsExternalResistor in the constructor to support external resistors and continue detecting correctly the gpio transitions in the events

* fixing comment

* Adding two constructors to the button which avoids using PinMode

* ctor refactoring to avoid ambiguity

* Cleaned up the old constructors

* fixes as requested by the review

* Removed the old two constructors. The default value of `isPullUp` is 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 breaking changes

* Fixed Arduino.Monitor.cs as for request

* dispose + error handling

* shouldDispose check

* Checking whether the pin support being configured as input and/or pull-up/down.

* refactoring
@raffaeler
Copy link
Contributor Author

Fixed with #2013

@ghost ghost locked as resolved and limited conversation to collaborators Feb 26, 2023
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@raffaeler raffaeler

Labels
bug Something isn't working Priority:2 Work that is important, but not critical for the release
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@krwq @raffaeler