search

solokeys / solo1-cli

Solo 1 library and CLI in Python
https://pypi.org/project/solo-python
Apache License 2.0
183 stars 69 forks source link

Bugfix: make monitor reconnect work #99

Closed enrikb closed 3 years ago

enrikb commented 3 years ago

On my Linux system (and maybe others as well), /dev/ttyACM0 is only created after the Solo key is inserted. If you try to 'monitor' before inserting, it will fail because the device node does not yet exist. Of course one can use debug-2 to stall the Solo key until monitor opens the serial device. However, this has the side effect of making the Solo key not work without a running monitor.

Second, if you disconnect the Solo key, 'monitor' tries to reconnect, but does not close the serial device before. The device node might be deleted, but the minor number is still in use. Therefore, on next connect e.g. /dev/ttyACM1 will be used and the reconnection will fail.

This change

  1. enters reconnect mode also when the device node does not exist initially

  2. closes the device before entering reconnect mode if it was opened before

(This has only been tested on Linux, namely Ubuntu 20.04.)

nickray commented 3 years ago

Thank you for looking into this! The serial interface, to me, is not exactly a nice experience...

I'd suggest we leave this open a little to give Windows or Mac users a chance to feedback (I'm on Linux too, and don't use this monitor interface), or perhaps @conorpp has input. If not, we merge (remind me if I forget please).

enrikb commented 3 years ago

Meanwhile I can confirm that the re-connect (still!) works on Windows 10, as well, with this change applied. On Windows, it also works without the patch. One drawback of the change as it is right now would be that if you call solo monitor <somdevice> with a typo in somdevice, there will be no error message. The script will just wait for somdevice to appear. So maybe the changed behavior on 'initial open' should depend on a command line flag. What do you think?

nickray commented 3 years ago

I'm not concerned about this. You might print f"Waiting for {serial_port} to appear..." or similar to be extra defensive?

enrikb commented 3 years ago

Thanks for that suggestion. Simple and effective!

enrikb commented 3 years ago

Thank you for looking into this! The serial interface, to me, is not exactly a nice experience...

I'd suggest we leave this open a little to give Windows or Mac users a chance to feedback (I'm on Linux too, and don't use this monitor interface), or perhaps @conorpp has input. If not, we merge (remind me if I forget please).

No feedback in >4 weeks? So either nobody has tested the change or it just works ;-)

nickray commented 3 years ago

I suspect the former :)