search

jetperch / pyjoulescope

Joulescope driver and utilities
https://www.joulescope.com
Apache License 2.0
37 stars 11 forks source link

Scanning for devices catches too many exceptions #20

Closed imbuedhope closed 3 years ago

imbuedhope commented 3 years ago

The exception handler in joulescope.driver.scan() is excessive and causes issues in code that depends on exceptions. This should really be narrowed to the bare minimum.

https://github.com/jetperch/pyjoulescope/blob/6b92e38c7ede9a0514d03d19dbb67c8d6b2490be/joulescope/driver.py#L1330-L1339

I have a script that uses a KeyboardInterrupt to halt the script safely. This is completely broken if scan() is being called when the interrupt occurs. And since the script calls scan_for_changes() repeatedly it happens consistently. This results in non-standard behavior for shell scripts.

In specific, the following continues to be stuck in a loop when a KeyboardInterrupt occurs.

try:
    while True:
        joulescope.scan()
except KeyboardInterrupt:
    pass

Unless I'm missing something, it looks like catching RuntimeError explicitly in L1337 should be good enough for this.

I can make a PR if needed, lmk!

mliberty1 commented 3 years ago

Hi @imbuedHope - I understand the issue. I view scan as NoRaise: it has no reason to throw an exception, just return an empty list. However, it should not handle the exiting exceptions KeyboardInterrupt and SystemExit. So, I think that it should safely be able to catch Exception rather than RuntimeError. Does this work for you?

imbuedhope commented 3 years ago

Yup that should work! I updated my PR.

mliberty1 commented 3 years ago

Merged into develop. It will make its way to main (soon to be renamed from master) with the next release. Thanks!