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

asyncio serial impl, optional config files, fix logging #38

Closed
wants to merge 25 commits into from

Conversation

csboling
Copy link
Contributor

Fixes #12 , fixes #14

I apologize that this is a lot of stuff at once, I can try and wrangle it into more digestible increments if desired. Tested on Windows and OSX, haven't had a chance to test Linux yet.

  • Adds a serial port + crow implementation using asyncio Protocols + pyserial_asyncio, which can detect connect/disconnect events and allows for nonblocking serial port operations. This allows the fix for When disconnected druid continually fills the console with messages #14. Some tweaks (in the form of overriding some methods) had to be made to get this cooperating cross-platform. Another class which keeps the polling implementation of the serial code is retained, this is what's used by the upload and download CLIs to avoid dealing with an event loop and stuff.
  • Maybe fixes Quitting druid results in traceback #12 by, I think, just not requiring the cancelled background task to run_until_complete? I don't know whether this is okay / safe but it seems to have a clean exit (for the time being)
  • Fixes some issues that were causing logging to write nothing useful, and to still write to the console.
  • Allows loading user configuration files for overriding various behavior. Defaults are all loaded from a hard-coded string in config.py. It will scan for druid.yml or .druid.yml in either your home directory or the current directory, and merge in configuration from there. For example, you can override the default shell colors:
    ui:
      style:
        'capture-field': '#747369'
        'output-field': '#d3d0c8'
        'input-field': '#f2f0ec'
        'line': '#747369'
    or customize how ^^stream, ^^change, or ^^ii.* events appear in the capture panels:
    captures:
      - on_inputs: [1]
        stream: 'input[{args[0]}] = {args[1]}'
        change: 'input[{args[0]}] = {args[1]}'
      - on_inputs: [2]
        stream: 'input[{args[0]}] = {args[1]}'
        change: 'input[{args[0]}] = {args[1]}'
      - ii: '{line}'
    There is a druid.yml checked in which just increases the default log verbosity to DEBUG on the assumption that if you have this file in your working directory, you're developing druid.
  • Reorganization to facilitate the above: move most stuff into some classes, decoupling the UI from the REPL functionality and from the serial port a bit.

@simonvanderveldt
Copy link
Member

simonvanderveldt commented Oct 15, 2019

Ohw, wow that's a lot of changes 😱

For starters: Thanks for the effort!

I apologize that this is a lot of stuff at once, I can try and wrangle it into more digestible increments if desired.

Yeah, it's definitely a good idea to split this into separate PRs :)

Did a quick scan, some high level feedback, might sound a bit harsh, but I do think these things need to addressed:

  • That's a lot/way too many classes. I really doubt we need them
  • There's no need for the nested package structure and large amount of modules
  • I don't really see a need for a config file, especially at the moment where there is barely any functionality and we should be focusing on fixing the basics. If/when we accumulate more features it might start to make sense but right now it just complicates the whole thing. If some colors are causing problems we should come up with some better defaults
  • I'm not sure about the whole custom reimplementation for the serial connection or what exactly is happening in druid/io/serial/device.py but what issue are we trying to solve? Is it really an issue and if it is is it worth taking on so much complexity/code to fix it?
  • A specific thing: get_distribution doesn't work for getting the version during development

I guess a more generic comment would be that if someone is planning or working on larger changes it's probably a good idea to create an issue first to discuss those changes.

@tehn
Copy link
Member

tehn commented Oct 15, 2019

wow! curious to look into the design ideas here. thanks for your efforts-- i'll be able to write more this evening.

@csboling
Copy link
Contributor Author

csboling commented Oct 15, 2019

TLDR: This is basically all to support more advanced use cases, the issues it addresses can probably be solved with more localized changes, and I don't want reviewing it to be a distraction from more pressing work. I'll try to split out some elements worth keeping into smaller patches.

That's a lot/way too many classes. I really doubt we need them
There's no need for the nested package structure and large amount of modules

Not the first time I've been told this! Mostly I get anxious when files get over a few hundred lines. Smaller files are often somewhat easier to merge I think, but it can definitely be more cognitive load to figure out what lives in which file. Classes here I'm mostly trying to use to decouple functionality but I'm willing to be convinced that the boundaries are too granular.

I don't really see a need for a config file, especially at the moment where there is barely any functionality and we should be focusing on fixing the basics.

I can see a couple sides to this. The principal advantage of runtime configuration is increased flexibility : I believe these initial options give the ability to customize aspects of the capture pane behavior that were being discussed recently on #13. This can also be empowering to users, since it's easier to make local changes to high level configuration than it is to modify source or put together a pull request. I included style options here because some forum discussion pointed out that the text is difficult to read on some terminal backgrounds, which may be a difficult problem to find good one-size-fits-all solutions for.

You can of course change your terminal background color instead, which avoids needing to support / document this configuration, though often that's a system-wide change and this is a simpler example of appearance customization. This seems like the main trade-off to me: flexibility of operation and promotion of more data-driven code structure vs documentation and validation burden. I guess my feeling is that every application wants for runtime configuration eventually, and this can be designed for early or painful to add later. Or not so bad to add later or totally unnecessary, also definite possibilities!

the whole custom reimplementation for the serial connection or what exactly is happening in druid/io/serial/device.py but what issue are we trying to solve? Is it really an issue and if it is is it worth taking on so much complexity/code to fix it?

These changes as I see them address a couple of concerns:

  • detecting connect / disconnect events so they can be reported once via a callback. This is probably achievable with some rework of the existing implementation or using serial.threaded via a similar approach to this patch
  • avoiding blocking on serial IO. This is probably a total non-issue as long as talking to a single crow is the only IO druid is trying to do, but becomes a performance bottleneck when trying to share the process with multiple devices. In particular the motivation for me came from performance issues I saw when trying to use pymonome to talk to a grid at the same time, using druid as a host to trigger Lua snippets on crow when a grid key is pressed. Using asyncio for both pyserial and pymonome, this seems to behave quite nicely. That's certainly off-roadmap usage of druid but is evidently where my mind raced off to shortly after I first used the program.
  • providing a common class abstraction over serial buffering strategies may make it easier to e.g. write integration tests or interact with other transports.

if someone is planning or working on larger changes it's probably a good idea to create an issue first to discuss those changes.

Totally fair! Often I find discussion of structural stuff like this hard to articulate without having some code in hand. This all basically grew out of my own harebrained explorations and I thought some of the structural changes might be nice to upstream in order to be working from the same codebase for both simpler and more complex use cases. With the benefit of sleeping on it I agree that this is probably overengineered for druid's primary functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When disconnected druid continually fills the console with messages Quitting druid results in traceback
3 participants