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.

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

Add checking if the new node is not the same as original mode in the setmode function #47

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vava24680
Copy link

I added the checking of whether the new mode and the original mode are the same or not in the setmode function. If both of the modes are the same, it's regarded as a normal behavior.
I have checked the original RPi.GPIO source code, there is similar checking mechanism in the setmode function.

@theyoyojo
Copy link
Contributor

theyoyojo commented Jul 26, 2020

Hey, thanks for the suggestion @vava24680

I took a look at the original RPi.GPIO code and while you are correct that there is a condition that resembles this, it seems to me that the condition is logically redundant in the use cases actually supported by RPi.GPIO.

Do you know of a use case where this condition would be relevant, and if so could you add a small test for it in the tests/test_gpio.py file?

@vava24680
Copy link
Author

Hi @theyoyojo, could you give me some time to answer ? I have something more important to do now, sorry for the delay.

@vava24680
Copy link
Author

@theyoyojo I found this use case when I use the Adafruit_CircuitPython_DHT library to get the temperature data from a DHT11 sensor. In that package, it uses setmode function twice.

I think since this package tries to replace to the origin RPi.GPIO, so it's reasonable to maintain the original behavior as the original RPi.GPIO, that's why I added this check in the setmode function.

I already added a test for this change.

Thank you!

@theyoyojo
Copy link
Contributor

theyoyojo commented Jul 30, 2020

Hello @vava24680,

Thank you for your test case. If we cannot find a use case for the extra logical condition, then I will at the very least merge your test case.

However, I took a look at https://github.com/adafruit/Adafruit_CircuitPython_DHT and I was unable to find any references to RPi.GPIO in that library. I noticed that it depends on https://github.com/adafruit/Adafruit_Blinka, so I checked that codebase as well, and though it does indeed use RPi.GPIO, I could not find an instance where setmode() is called twice in a context where it would be executed on a Raspberry Pi. All I found was this: https://github.com/adafruit/Adafruit_Blinka/blob/1c7e43354667e4268f8d806d9ee1bd3d41272310/src/adafruit_blinka/microcontroller/bcm283x/pin.py#L4

Can you point me to the specific codepath you found that requires this logical condition?

@vava24680
Copy link
Author

  1. In the adafruit_dht.py, at line 33, it imports DigitalInOut, Pull, Direction from the digitalio.py, and In the digitalio.py, it further imports the pin module. In the corresponding pin module, the setmode is called first time.
  2. Also in the adafruit_dht.py, at line 37, it imports the PulseIn from the pluseio.py and in the pluseio.py, the PulseIn.py and PWMOut.py are imported, the setmode is called second time in the PWMOut.py. That's why I need this logical condition.

@theyoyojo
Copy link
Contributor

theyoyojo commented Aug 14, 2020

I am still having trouble reproducing this on my RPi 3 B v1.2, but I will try this out on my RPi 4B+ as well. Once I can reproduce the issue and confirm that your patch fixes it, I will merge the pull request.

As noted in #28, we don't have a reliable way to detect RPi version info in a distro-generic manner, and as you can see by the current values in the RPI_INFO dictionary, the data is hardcoded for the RPi 3 B v1.2 at the moment, though we plan to change this in the near future. One main goals of this project is to provide support for the RPi.GPIO API on as many distributions as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants