search

adafruit / Adafruit_Blinka_bleio

`_bleio` for Blinka based on `bleak`
58 stars 19 forks source link

Fixed FileNotFound and permissions exceptions on RasPi #20

Closed alexwhittemore closed 3 years ago

alexwhittemore commented 4 years ago

I'll admit I'm not SUUUUPER sure what's exactly going on here, but in light of (98) "Lots of things can go wrong:" I agree and it seems subprocess.SubprocessError is too restrictive.

I'm testing on Raspberry Pi (actually, my bad on the commit message, I'm confusing myself), and trying to go back and forth between using hcitool and not. For whatever reason, before I ever installed hcitool, I had no problems at all and the library worked without failure. Since installing hcitool and trying to disable it either by renaming or removing my user from the bluetooth group, I get either FileNotFoundException or a permissions exception, respectively. Both of these, being not equal to subprocess.SubprocessError crash the main script.

Generically catching all exceptions here fixes all of the exception type ambiguity and I think satisfies the spirit of the code without introducing edge cases? Though I'd like to understand better why it works just fine out of the box, and only breaks when I re-disable hcitool.

tannewt commented 4 years ago

except clauses should always have types listed. Otherwise, you will catch exceptions you didn't anticipate.

Instead, you can add additional clauses for the types of exceptions thrown on Mac. I'm not surprised that they differ from Linux.

alexwhittemore commented 4 years ago

I can go back and try to enumerate what they all are. I understand the theory in general, it just seems inapplicable here since the whole point is "see if this way of doing it fails, and if so, we'll do it the other way." Failing in some weird unexpected way would still seem to suggest "do it the other way."

Also it's not actually running on mac, I had that wrong in my head when I wrote the commit message - this is actually running on a Pi. So it DOES surprise me that it differs.

alexwhittemore commented 3 years ago

Ok I went back and explicitly added the two exceptions I've encountered (on RasPi - corrected the PR name too)

FileNotFoundException: When I explicitly rename /usr/bin/hcitool to /usr/bin/hcitool-disabled, or if I simply don't install it, this is what I get. I'm surprised I'm the only one, if that's true. Maybe people just generally install it and don't give a second thought?

PermissionsError: Another plausible way to "disable" using hcitool that I've used is a to just remove the execute permission. On the other hand, it'd be weird to have it installed yet not executable, so I'm open to the idea that a permissions error should actually be raised all the way up to the user to figure out. Given the opacity of the "hcitool if we can" strategy, "I have it installed, why isn't it getting used?" seems a likely outcome if "PermissionsError" is caught.

That itself raises the question of whether "hcitool or not" should be a more manual choice on the part of the user.

Also I'm just generally unclear on which failure modes "SubprocessError" is actually expected to catch, if not the specific ones I've found here.