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

Refactor REPL and crowlib code #60

Open
simonvanderveldt opened this issue Oct 25, 2019 · 0 comments
Open

Refactor REPL and crowlib code #60

simonvanderveldt opened this issue Oct 25, 2019 · 0 comments
Milestone

Comments

@simonvanderveldt
Copy link
Member

simonvanderveldt commented Oct 25, 2019

There are a couple of issues with the current REPL and crowlib code that we should address:

  • REPL: Use of globals/no proper place to store state
  • Communication internals are implemented in both the REPL and the CLI as well as in crowlib, mainly because we're directly exposing the serial port object (ie we need to do .write(bytes()) or .write(something.encode()).
  • Where to write output to from crowlib is passed in from the REPL. Ideally the output from crowlib wouldn't care about this and use normal logging which would just bubble up to consumers of crowlib like the REPL that could then show the logged output.
  • Things like uploads are implemented multiple times (once in crowlib and once more with a bit more logic in the CLI) and their behavior is different

I've been trying to get something working for a bit (for example in this branch https://github.com/simonvanderveldt/druid/tree/cleanup-crowlib where I started with refactoring crowlib) but it's been difficult to keep the changes small, mainly because everything is a bit entangled with eachother. It tends to end up in basically changing everything 😬
I don't really know how to solve that yet, but I would like to suggest to start with adding a class for the REPL. This should contain the REPLs state and hopefully be a decent start for the other changes as well.

This might need a separate issue but putting it here for now:
We might also need to change the serial implementation a bit, especially the reading of the serial port. Unfortunately asyncio's add_reader won't work cross platform (mainly issues on Windows, serial isn't an fd on Windows which means it won't work with SelectorEventLoop and ProactorEventLoop doesn't support add_reader at all. See https://docs.python.org/3/library/asyncio-platforms.html#windows for more detail.
So we'll either need to fall back to using pyserial-asyncio, which also has issues on Windows which for now have been solved by polling (see pyserial/pyserial-asyncio#8) or we could also simply split the serial reading off into a thread.

Also see #38

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

No branches or pull requests

1 participant