MrYsLab / pymata-aio

This is the second generation PyMata client.
https://github.com/MrYsLab/pymata-aio/wiki
GNU Affero General Public License v3.0
155 stars 51 forks source link

Throw exceptions instead of exiting #94

Closed DaAwesomeP closed 5 years ago

DaAwesomeP commented 5 years ago

I am attempting to use Pymata within a larger framework/project, but every connection error uses a sys.exit(0) that completely stops everything. It would be nice if there was a config option to instead throw an exception from a central exceptions file. This also makes it possible to programmatically handle errors differently.

DaAwesomeP commented 5 years ago

For example, I could do something like this:

try:
    await board.start_aio()
except PymataWrongPortException:
    # tell the user Pymata can't connect to this port
    pass

This is good for apps that don't run with the user necessarily watching the CLI (i.e. web UI or desktop UI). It also means that Pymata does not have to be a fatal dependency if the app does other things.

MrYsLab commented 5 years ago

If you are using pymata_core and not pymata3, I suggest you move to pymata-express. It will throw runtime exceptions without exiting. Also, it has superior port detection if you use the Firmata-Express sketch (downloadable from within the Arduino IDE). There is a handshake between pymata-express and Firmata-Express that guarantees that the correct port is found.

I could potentially provide an option that RuntimeErrors would be thrown instead of exiting, but for other than the initial port detection, it does make a lot of sense to do so. Once a serial port is found and successful communication is underway, if an error is detected, the Arduino will most likely need to be physically reset.

If I optionally raise an exception when port discovery fails, or retrieval of the analog map or Firmata version number fails at startup, would that meet your needs?

DaAwesomeP commented 5 years ago

If you are using pymata_core and not pymata3, I suggest you move to pymata-express.

Besides the port detection, is there another reason to move to pymata-express? Is it an even newer revision of Pymata? I'm making a new component for Home Assistant, so I will probably stick with the official/standard Firmata sketches.

If I optionally raise an exception when port discovery fails, or retrieval of the analog map or Firmata version number fails at startup, would that meet your needs?

Yeah that would be great!

MrYsLab commented 5 years ago

I added a parameter to both the pymata3 and pymata_core classes. This new parameter is "port_discovery_exceptions". Its default value is False which maintains the original behavior. If you set it to True, you should get RuntimeError exceptions instead of the sys.exit. Please let me know if there are any issues. The changes are in version 2.30.

And thanks for pull request for pymata-express.

DaAwesomeP commented 5 years ago

It works perfectly thanks!

MrYsLab commented 5 years ago

Great!

DaAwesomeP commented 5 years ago

Sorry to pull up an old issue, but I think there can be an improvement.

None of the raised RuntimeError() exceptions include messages. I'm currently seeing an issue where no errors are printed to the console and shortly later RuntimeError() is called, so I don't know what is going wrong.

I started to work on a pull, but I was confused by this recurring part:

# Above this snippet are some print or log statements about what went wrong.
# What TypeError is being caught? Is this catching an error in stopping the event loop?
try:
    loop = self.loop
    for t in asyncio.Task.all_tasks(loop):
        t.cancel()
    loop.run_until_complete(asyncio.sleep(.1))
    loop.stop()
    loop.close()
    if self.port_discovery_exceptions:
        raise RuntimeError
    else:
        sys.exit(0)
except RuntimeError:
    self.the_task.cancel()
    time.sleep(1)
     # this suppresses the Event Loop Is Running message,
     # which may be a bug in python 3.4.3
     if self.port_discovery_exceptions:
         raise
     else:
         sys.exit(0)
except TypeError:
    if self.port_discovery_exceptions:
        raise RuntimeError
    else:
        sys.exit(0)
MrYsLab commented 5 years ago

When you mention "recurring part" do you mean the raise of RuntimeError followed by the except RunitmeError. If that is what you mean, Python 3.4.3 exhibited some weird behavior that may no longer be exhibited in later versions of Python. When closing down the loop in 3.4.3, it generated a screen full of exceptions, and TypeError exception handler prohibited that from happening. This was a workaround for the issue.

Can you provide a code example that demonstrates the "issue where no errors are printed to the console and shortly later RuntimeError() is called"?

How shortly is shortly and is that an issue?

Also what operating system and Python version are you running?

Thanks.

DaAwesomeP commented 5 years ago

When you mention "recurring part" do you mean the raise of RuntimeError followed by the except RunitmeError. If that is what you mean, Python 3.4.3 exhibited some weird behavior that may no longer be exhibited in later versions of Python. When closing down the loop in 3.4.3, it generated a screen full of exceptions, and TypeError exception handler prohibited that from happening. This was a workaround for the issue.

I see, that explains that.

Can you provide a code example that demonstrates the "issue where no errors are printed to the console and shortly later RuntimeError() is called"?

It's hard to provide the code because it's baked into a Home Assistant integration. I was about to provide you some logs but I restarted my machine and the issue went away... very odd.

What I thought was causing this was that I am running two PyMata instances in the same thread, but I can't see what exactly is failing because the error isn't descriptive. But now it's working, so I don't know.

How shortly is shortly and is that an issue?

A few seconds after.

Python 3.5.3 Debian 9.9 Kernel 4.9.0-9-amd64

MrYsLab commented 5 years ago

Are you committed to Python 3.5.3 or can you upgrade to python 3.7.4? Asyncio has many improvements since 3.5.3, so things may clean up quicker, but without knowing the cause of the slow shutdown, I am not really sure if 3.7 is the cure.

DaAwesomeP commented 5 years ago

No, I'm not committed to Python 3.5 at all—that's just the Debian 9 version. I'll let you know if the issue returns!