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

Running SysFsDriverTests or RaspberryPiDriverTests causes PWM to be unusable #874

Closed
krwq opened this issue Dec 1, 2019 · 12 comments
Closed
Labels
bug Something isn't working

Comments

@krwq
Copy link
Member

krwq commented Dec 1, 2019

After running SysFsDriverTests or RaspberryPiDriverTests PWM no longer works.

I have 100% repro: If I run PWM tests in a loop or run LibGpiodDriverTests everything works without any issues.
If I run SysFS or RPI3 GPIO tests once after that PWM becomes completely unusable and stops reacting to any changes (regardless if using PWM manually through /sys/class/pwm/ or our APIs) - PI has to be rebooted.

Found during testing #758

I'm currently on latest from master and my PI3 is running Raspbian after sudo apt-get update and sudo apt-get upgrade (was reproing before updates as well).

I'll try to further isolate the problem if I have time.

@krwq krwq added the bug Something isn't working label Dec 1, 2019
@krwq krwq added this to the vNext milestone Dec 1, 2019
@pgrawehr
Copy link
Contributor

pgrawehr commented Dec 1, 2019

The reason for your problem is #857. You need to reset the pin to PWM mode after some other test has set it to in or out.

@krwq
Copy link
Member Author

krwq commented Dec 1, 2019

@pgrawehr possible but I don't believe any of these drivers should be even touching PWM pin...

@krwq
Copy link
Member Author

krwq commented Dec 2, 2019

actually seems they do touch it...

@krwq
Copy link
Member Author

krwq commented Dec 2, 2019

Hmm.. I'm kinda leaning toward not allowing to open any pins which have mode set to something which we do not understand to avoid similar problems in the future... @pgrawehr - that should at least prevent the problems like this since they are really weird to debug

@pgrawehr
Copy link
Contributor

pgrawehr commented Dec 2, 2019

@krwq I wouldn't completely disallow it, but make sure they are not opened in different modes at the same time. Otherwise, you would still not be able to open a pin in PWM mode if another application has set it to Output.
In either case, I think that comes down to the "Board" approach you mentioned in #809 (comment), since only then we have a common board-wide controller that can know which pins are open for what and which pins have which special function(s).

@krwq
Copy link
Member Author

krwq commented Dec 2, 2019

Yes, other thing I'm considering is perhaps let user decide what to do on dispose, i.e.:

gpioController.OpenPin(18, PinMode.Output, XYZ.RestorePreviousMode); // XYZ.DoNotClose, XYZ.DontCare

The DoNotClose is related to #347

@pgrawehr
Copy link
Contributor

pgrawehr commented Dec 3, 2019

I didn't encounter this scenario, but I think you're right. Not sure whether the XYZ parameter should be given on Open() or Close() though.

@krwq
Copy link
Member Author

krwq commented Dec 4, 2019

@pgrawehr IMO Open because:

  • Dispose auto-calls Close which would make it easier to use without having to close pins manually
  • Implementation has less work in cases where it doesn't have to do anything (doesn't have to read back the mode etc)

@pgrawehr
Copy link
Contributor

pgrawehr commented Dec 5, 2019

That sounds reasonable. Wouldn't prevent to have it on Close(), too (to explicitly do something else).

@krwq
Copy link
Member Author

krwq commented Dec 5, 2019

@pgrawehr not sure how would that work/give if we don't add other modes that you suggested in the other issues (perhaps we should consider that as an option). We won't be able to restore to previous unless we explicitly store it before

@pgrawehr
Copy link
Contributor

pgrawehr commented Dec 5, 2019

@krwq True, but if we offer to restore the previous mode, we need to be able to store it anyway. (unless we just throw if the previous mode was unknown and RestoreToPrevious was selected, but that's pretty useless then) Without adding the list of modes, we would only be able to force it into a specific one, but not necessarily restore to something unknown.

This doesn't necessarily mean that the extra modes will be visible from outside, but hiding that would make the implementation ugly, I think (and prevent extensions like the pigpio from using it).

@pgrawehr
Copy link
Contributor

Fixed with #1128.

@ghost ghost locked as resolved and limited conversation to collaborators Jan 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants