vlachoudis / bCNC

GRBL CNC command sender, autoleveler and g-code editor
GNU General Public License v2.0
1.57k stars 533 forks source link

Move serial port logic into controller classes. #1330

Open mrdunk opened 4 years ago

mrdunk commented 4 years ago

I just had a look at Sender.py. I last looked at this file a few years ago and thought at the time it would be good to subclass the different controller types. It's nice to see this has now been done with the controller plugins.

I think we could go a bit further though. Currently all supported controllers are driven using a serial port but i think this is somewhat limiting; If the serial port code moved to one of the plugins parent classes it would allow plugins which do not rely on a serial port to be written. Some useful plugins that spring to mind:

The work doesn't look that complicated. It's mostly just refactoring with very few changes to the logic. The plugin hierarchy would look something like this afterwards:

_GenericController
.   dummyController
.   _GenericSerialController
. .   SMOOTHIE
. .   _GenericGRBL
. . .   GRBL0
. . .   GRBL1

Is this something that is likely to get merged if i work on it?

Harvie commented 4 years ago

Actualy such functionality is mostly present. We use pyserial url handlers for this:

https://pyserial.readthedocs.io/en/latest/url_handlers.html

Talking about dummy controller for testing, there are several possibilities. You might use grbl-sim (compiles grbl to linux executable), or simavr (can run any atmega/arduino code on linux, including serial line). Or you might just go for dummy included in this repo: https://github.com/vlachoudis/bCNC/blob/master/tests/fake-grbl.sh

It creates fake serial port using socat (probably currently works only on linux, might need some tweaking on other platforms).

If you don't like idea of fake serial ports, it would help if pyserial fixed this: https://github.com/pyserial/pyserial/issues/393

that would allow to launch grbl simulator binary directly without need for intermediate fake serial port to connect bCNC to it.

Also note that if this abstraction gets improved, it will be usefull in more than just bCNC. Other projects using pyserial will benefit aswell.

mrdunk commented 4 years ago

So while there are solutions for most of the use cases i listed above, they all require hacking around outside bCNC. If dependence on a serial port was optional it would decouple the Sender class from the serial port and such hacks would no longer be necessary.

I guess i'm discussing code structure here rather than any particular feature. In my opinion, the refactoring i'm suggesting would make the existing code cleaner and future work easier.

It's your choice though; If you don't think it's beneficial then i'll drop it.

mrdunk commented 4 years ago

Some other random thoughts:

Pyserial URL handlers; I see you have discovered the joy of socat. Are you aware socat can do TCP as well as serial? With this you could have a small headless Linux machine with connected controller running socat then connect to it direct from bCNC. socat can do SOCKS, SSL, etc so you get some security for free.

Your pyserial suggestion at pyserial/pyserial#393; I'm not convinced adding exec type functionality to pyserial is the right approach here. Leaving discussion about the serious security implications to one side, executing arbitrary commands on a remote server is way outside the remit of what the pyserial project is trying to achieve. You could achieve similar with https://github.com/paramiko/paramiko . It's a Python ssh library which (among other things) allows execution of arbitrary commands on a remote server. Of course this puts us back to the original discussion of whether a local serial connection is needed... but you /could/ use paramiko to start socat in TCP mode on the remote server then pyserial to connect to it. I'd prefer to use paramiko to start a headless version of bCNC and use paramiko to communicate with it... without a local serial port... . This would make the remote machine OS agnostic. It would work for anything that has Python and SSHd.

grbl-sim looks interesting. I was wondering if this was a thing. I'll check it out. Thanks.

Harvie commented 4 years ago

I guess i'm discussing code structure here rather than any particular feature.

Well. I don't really see any benefits from doing that right now. To be honest, i was originaly aiming exactly for the approach that you suggest, but once i've found the features in pyserial are good enough, i just left this intention.

Don't get me wrong. I want to make sure that commonly available hardware is accessible from bCNC. I think currently this means two cases.

1.) GRBL port to ESP32 (wifi enabled hardware) 2.) GRBL plugged in console server (eg. of the shelf Mikrotik router with USB port)

Both of these setups are working and described in wiki: https://github.com/vlachoudis/bCNC/wiki/Connecting

I don't say your approach is wrong. Probably we can benefit from it in future. But right now i think it would make more sense to focus on bigger problems in bCNC. What really breaks my heart now, is how slow the g-code rendering is. Not to mention the lack of antialiasing. We need to figure out way to implement some faster graphical display (eg. opengl based) which will be fast and would allow instant rendering and free 3D rotation.

Harvie commented 4 years ago

I'm not convinced adding exec type functionality to pyserial is the right approach here. Leaving discussion about the serious security implications to one side, executing arbitrary commands on a remote server is way outside the remit of what the pyserial project is trying to achieve.

I am not talking about executing arbitrary commands on remote server. I am talking about executin arbitrary commands localy. I am very aware this is not exactly safe for everyone, so this feature would need to be explicitly enabled in code using pyserial.

I think problem lays in the fact that pyserial authors are very unresponsive. It would help if they at least allowed new maintainers into the project.

Are you aware socat can do TCP as well as serial?

I am. That is one of the currently possible bCNC setups. Run SOCAT on machine physicaly attached to GRBL and connect bCNC to it using pyserial over LAN.

mrdunk commented 4 years ago

But right now i think it would make more sense to focus on bigger problems in bCNC. Understood.

What really breaks my heart now, is how slow the g-code rendering is.

I just had a quick look at CNCCanvas and friends. Ideally a class like this would either be fed all updates and it would either cache or react to those synchronously or it would have access to a State Machine which has up to data about what is to be drawn and update it's self several times per second.

At first glance CNCCanvas mostly does the 2nd of those which i think fits the problem at hand best. A few references to self.canvas have leaked out into Application creating worrying circular dependencies. Maybe this is to do with the way TK handles the main loop? Are you forced to update in response to TK events or are you free to update the canvas according to your own schedule? I would be worried you are reacting to events more often than necessary and wasting CPU. An arguably better model is to set a flag in the state machine (Application) when a component needs redrawn and only redraw once per frame.

Not to mention the lack of antialiasing. We need to figure out way to implement some faster graphical display (eg. opengl based) which will be fast and would allow instant rendering and free 3D rotation.

What do you have in TK for this? I used Pygame ~12 years ago and found it fun and intuitive. It does not force you to use any particular main loop. Just draw to one of 2 canvases and flip them when you are ready. Maybe this could happen in a separate thread? I'm not aware of any other libraries that give a low hassle way of drawing vector art from Python.

Anyway, this is all a bit off topic. Thanks for taking the time to discuss and thanks for the useful software!